-
Notifications
You must be signed in to change notification settings - Fork 90
Lightweight module system with config manager #54
Lightweight module system with config manager #54
Conversation
$cachedConfigFile = 'data/cache/app_config.php'; | ||
$configManager = new ConfigManager([ | ||
// Load config from all modules first | ||
new PhpFileProvider('src/*/Config/{{,*.}global,{,*.}local}.php'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance this looks like a great idea (no need to register each module separately), but after giving it some thoughts I see a few potential problems:
- all modules would have to follow the same convention (configuration in PHP file, under
src/ModuleName/Config/
directory). You also have to be aware of your modules' directory structure. I suggest using config provider classes instead (like in my initial example). - local overrides (
*.local.php
) should be placed only inconfig/autoload/
directory, otherwise you could end up with multiple local files in different locations. - using wildcard doesn't let you specify order of modules to be loaded.
- finally, with wildcard it is not explicit which modules are actually loaded (I guess this is matter of personal preference though).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct that you want to place config files in the /src/
dir of each module? And that it is possible to have global.php
and local.php
within the modules?
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct that you want to place config files in the /src/ dir of each module?
Not sure what you mean. The idea behind this config manager is to make it easier to create modules and load their config. So yes, the config files are placed inside the modules. If you mean they are placed at the same level as ModuleName/Action etc, then yes, since there is no ModuleName/src/Action etc. But that's for @weierophinney to decide if we change the structure. I wanted to keep it as close as possible to your tutorial.
And that it is possible to have global.php and local.php within the modules?
I agree, local.php should be in config/autoload/.
But what is the best to do? Only allow global.php or just .php?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO with natural structure for module is this:
MyModule/
config/
src/
templates/
Every related file (classes, templates, configs) is grouped together under common directory. This is something people are already familiar with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what is the best to do? Only allow global.php or just .php?
If you don't want to use provider classes, then just go for *.php
within some directory - it's pretty straight forward.
I feel it's a bit too much to use config provider classes for a simple project. Personally I prefer to keep it as simple as possible. I write the modules in the src dir so I can set my own convention. But I know this is a skeleton for projects and it should be using best practices. However for 3rd party modules that needs to be loaded from the vendor dir, I can imagine you want some sort of convention. If a config provider class is the way to go than I'll change it. But you need to keep in mind that the idea behind zend-expressive is to use middleware from other projects as well. So if you decide on a convention, this doesn't mean that if you import some auth middleware from another project it has that config class. But that's the good thing about this config manager, it can load anything from anywhere. You just need to add it to |
need rebase |
@weierophinney @xtreamwayz I really would like to get this PR accepted. That will be the base to add the zend-component-installer as well and get a real nice and flexible set up. I just stumbled about a problem described here. zendframework/zend-component-installer#6 If the Config Manager is added then no one should have the same issue that I solved myself |
First the config-manager has to be moved into a zend repo. Matthew has been very busy with preparing the ZF3 release. I'm pretty sure this is on his todo list after that. |
@xtreamwayz another rebase needed |
I had to do a |
Suggestions:
see related changes and implementation in https://github.com/gabbydgab/ZFExpressiveModularApp |
I like the direction of this, but have a few comments; requests.
Thoughts, @xtreamwayz ? |
I think a good rule of thumb would be that third party code should not pipe middlewares or register routes automatically, but expose middlewares that should be explicity registered by the developer in proper ordering and path. |
About the structure: This has been discussed a long time ago but there was never a final decision made. This would be a good time. If the zend-config-aggregator is standard added to the skeleton, we should show a good example of a modular structure. It's been a while since I made this PR, but I included zend-component-installer and I think it actually worked. So we wouldn't need the ./vendor/bin/expressive-register. I need to check this again though, otherwise the ./vendor/bin/expressive-register is required. I think modules should be able to work out of the box. Or at least in a few easy steps with questions:
That brings me to a challenge I encountered when writing tests. How can you pipe middleware at a specific position?
I guess routes can be added at any point so those should be fine. However the order might matter for routers. Including zf-development-mode would be most welcome but it needs some adjustments to make it work with expressive. |
Notes based on the feedback/questions from @danizord and @xtreamwayz. First, addressing @danizord:
Agreed, to an extent. There's a few things at play here that we may need to address. First, a module should likely expose, at the very least, the services it provides. These would be via a The second aspect, though, is providing information on:
Regarding this statement by @xtreamwayz:
I should have been more specific in my notes. We'd still want zend-component-installer in place. However, for application-specific modules, i.e., those under the
We could even create a binary in the skeleton or within zend-expressive-tooling that performs all three steps. Regarding this:
I agree to an extent. The first question is already taken care of via zend-component-installer, and potentially via the What we could do, however, is list which middleware a package exposes, along with descriptions, as well as any functionality for injecting routes (see more on this below). This is something that could go into zend-component-installer; it could read from additional package metadata to do so. (As noted above with regards to standardizing a documentation format for pipeline middleware, I do not think we need to address this right now.) So, in the end, this all comes down to two items, as far as I'm concerned:
Proposed App module structureI propose that src/ layoutThe
ConfigProvider
namespace App;
class ConfigProvider
{
public function __invoke()
{
return [
'dependencies' => $this->getDependencies(),
'templates' => $this->getTemplates(),
];
}
public function getDependencies()
{
return [
'invokables' => [
Action\PingAction::class => Action\PingAction::class,
],
'factories' => [
Action\HomePageAction::class => Action\HomePageFactory::class,
],
];
}
public function getTemplates()
{
return [
'paths' => [
'app' => [__DIR__ . '/../templates'],
],
];
}
} composer.json changesThis will require that Templates changesAdditionally, the root-level Installer changesThe installer will need to be updated to:
Module-specific routingWith programmatic pipelines and routing being the new recommendation, how can modules provide routing? There are definitely huge reasons to do so:
That said, there are problems with providing them automatically:
One approach would be to have third-party module writers document the routes expected and/or supported, and have application developers copy/paste these into their application. This is problematic: later updates to the module may result in additional or altered routes, which would then need to be updated in the application manually. This is something that can easily be missed. Fortunately, we already have a mechanism for this in place: delegator factories. These work already for both zend-servicemanager (where they are used verbatim) and Pimple (where they are used as Pimple extensions. Delegators only need to be callable, and accept three arguments:
As such, you can write delegators that work across container implementations. Since the container is provided to the delegator, we can also retrieve the router, allowing the delegator to determine how to inject routes based on the router type. As an example: namespace Acme\Foo;
use Interop\Container\ContainerInterface;
use Zend\Expressive\Router\AuraRouter;
use Zend\Expressive\Router\FastRouteRouter;
use Zend\Expressive\Router\RouterInterface;
use Zend\Expressive\Router\ZendRouter;
class ApplicationRoutesDelegatorFactory
{
/**
* @param ContainerInterface $container
* @param string $serviceName
* @param callable $callback
* @return \Zend\Expressive\Application
*/
public function __invoke(ContainerInterface $container, $serviceName, callable $callback)
{
$app = $callback();
$app->get('/foo', Action\Collection::class, 'acme.foo');
$app->post('/foo', Action\ResourceCreate::class);
$router = $container->get(RouterInterface::class);
switch (true) {
case ($router instanceof AuraRouter):
$app->get('/foo/{id}', Action\Resource::class, 'acme.foo.resource')
->setOptions(['tokens' => [
'id' => '\d+',
]]);
break;
case ($router instanceof FastRouteRouter):
$app->get('/foo/{id:\d+}', Action\Resource::class, 'acme.foo.resource');
break;
case ($router instanceof ZendRouter):
$app->get('/foo/:id', Action\Resource::class, 'acme.foo.resource')
->setOptions(['constraints' => [
'id' => '\d+',
]]);
break;
default:
// unsupported; throw an exception? ignore entirely?
// up to module developer
break;
}
return $app;
}
} This delegator factory should not be registered by default. In other words, if the module defines a At the application level, if the application developer wants to include the routes verbatim, they can do the following in return [
'dependencies' => [
// ...
'delegators' => [
Application::class => [
\Acme\Foo\ApplicationRoutesDelegatorFactory::class,
// additional delegators as needed
],
],
],
];
BenefitsThis approach has several benefits:
ProblemsThe primary problems are as follows:
The first two we can only resolve via recommendations, and by documenting the potential problems within the manual. I do not think there is a foolproof way to address this, without adding complexity. The latter could be resolved by adding an accessor to the |
That's a lot of text. I need to read a bit more thoroughly tomorrow. My first reaction: As the programmatic way makes it easier to understand middleware, it looks like it actually makes it harder to configure the application, especially when modules are involved. |
Yes and no. Normal configuration (providing dependencies, providing module-specific configuration, providing configuration overrides) is quite easy, actually, as you only need to provide the namespace Acme;
class ConfigProvider
{
public function __invoke()
{
return [
'dependencies' => [ /* ... */ ],
'acme' => [ /* ... */ ],
];
}
} For registering middleware, we likely wouldn't have wanted modules to auto-register
Do this once for each middleware you want to use from the given package. Its routes that are more problematic, and, like middleware, they really shouldn't be auto-registered anyways. As such, like regular middleware, we should encourage package developers to setup the dependencies and any related configuration, but not routing configuration, to ensure that the application developer can easily override it. However, we do likely want them to provide routing out-of-the-box if they provide routable middleware. So, the tl;dr on the above comment is that this is provided by delegator factories, which means the application developer needs only:
The original comment provides an example. We could even provide a script to make this simpler for end-users. tl;drWhat follows is a tl;dr of the original comment, as actionable steps.
The latter two are targets for the documentation, and, as such, would be part of the zend-expressive package. You can read the longer post if you need more detail on the "why". 😄 |
$aggregator = new ConfigAggregator( | ||
[ | ||
// Load module config | ||
new PhpFileProvider('src/App/Config/{{,*.}global}.php'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be something like App\ConfigProvider::class
(to be created) per @weierophinney's suggestion above, instead of looking for individual config files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
return [ | ||
// Cache the configuration: Recommended for production |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wording could be improved:
Recommended to be enabled for production
I don't think it is clear enough that this is the development config.
return [ | ||
// Cache the configuration: Recommended for production | ||
// Closures used somewhere in the configuration make caching impossible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Closures are used anywhere in the merged config caching is impossible.
return [ | ||
// Cache the configuration: Recommended for production | ||
// Closures used somewhere in the configuration make caching impossible | ||
ConfigAggregator::ENABLE_CACHE => false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this in local.php.dist? I would think development.config.php.dist is the right place for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it is in development.config.php.dist. You should probably decide where it should live and remove the other entry. Having it specified twice will confuse new users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want this in local.php since you only want this enabled on your production server and an user should set it in that file. Also you don't want caching enabled by default since that really confuses people during development. I've seen a lot of issues with this for ZF3 in #zftalk.
It's also in development.config.php to make sure it is set false in development mode (in case it's enabled in local.php). Since development.config.php is loaded last, it will overwrite all previous states for this setting.
It shouldn't confuse new users since development.config.php.dist should not be edited and is meant for the zf-dev-mode tool only. Maybe a warning on top of that file might make this clear.
* Configuration file for the zf-development-tool. | ||
* | ||
* Do not edit this file, or the tool will not work properly!!! | ||
* Instead change the settings in `config/autoload/local.php`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is wrong with editing this file? This file represents configuration that you always want in your development environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, you mean extra dev settings as well. You got a good point there.
@weierophinney I think I've made all the changes you proposed. Except for the error and layout templates. I've placed these inside the App module as well since I see this as the default module. Just as with the ZF3 skeleton. If needed I can move those back. |
as discussed with @MichaelGooden
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I see a few more changes and/or questions to answer, though, as noted.
@@ -48,6 +51,10 @@ | |||
"scripts": { | |||
"pre-install-cmd": "ExpressiveInstaller\\OptionalPackages::install", | |||
"pre-update-cmd": "ExpressiveInstaller\\OptionalPackages::install", | |||
"post-create-project-cmd": ["@development-enable"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! hadn't considered we could do that, but of course we can!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Credits to @MichaelGooden !!!
return [ | ||
// Configuration cache: disabled by default. Recommended to be enabled for production. | ||
// If Closures are used anywhere in the merged config, caching is impossible. | ||
ConfigAggregator::ENABLE_CACHE => false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be in config/development.config.php.dist
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is there as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should have it in both places, basically. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm removing this entry within my local merge branch.
@@ -3,9 +3,6 @@ | |||
return [ | |||
'debug' => false, | |||
|
|||
// Cache the configuration. Recommended for production. | |||
'config_cache_enabled' => false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we'll be putting the application in development mode on initial install, I'm wondering if we should have this enabled by default? Particularly if the development configuration disables it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering the issues we had on #zftalk when ZF3 was released, I prefer to disable caching by default. There were a lot of questions and issues where the config was cached (enabled by default) and changes that were made were not applied to the app since the cache was loaded.
I try to prevent that by having the cache disabled by default and they need to read what it does to enable it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm torn on this. One of the key reasons for having development mode was to solve the problem of ensuring it's optimized by default, but can be development ready otherwise. Having to add configuration to make it production ready makes for educational overhead.
One possibility would be to have the setting commented out:
// Uncomment the below to enable configuration caching.
// You may then use development-enable/development-disable
// to toggle caching.
// 'config_cache_enabled' => true,
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the key reasons for having development mode was to solve the problem of ensuring it's optimized by default, but can be development ready otherwise.
Makes sense. So where to set that boolean to true? In config\autoload\zend-expressive.global.php?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enabling this in zend-expressive.global.php in my local merge branch.
//Include cache config for zf-development-mode | ||
function () use ($cacheConfig) { | ||
return $cacheConfig; | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels convoluted. Doesn't ConfigAggregator
just look at the second argument to the constructor to determine where the configuration cache is, if any? Why would we have this as part of the configuration? (I feel like I'm missing something...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. It was merged from f83cf75, but I can't remember if I asked @MichaelGooden why it's there. We might as well revert that commit and use 'data/config-cache.php'
as it was before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's omit it for now, and we can revisit later if we are given more information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is required to get the config cache path into zf-development-mode. I cannot find a cleaner way to pass the path to zf-dev-mode and I did not want to hardcode the path either. (https://github.com/zfcampus/zf-development-mode/blob/master/src/ConfigDiscoveryTrait.php#L60-L62)
This closure hack is because ConfigAggregator
does not accept a plain array, it requires a closure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More specifically, ConfigAggregator
accepts a string to be instantiated as a class, or a closure. (https://github.com/zendframework/zend-config-aggregator/blob/master/src/ConfigAggregator.php#L80-L96)
I personally propose allowing ConfigAggregator
to accept plain arrays in the constructor, I cannot see any downside to it. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason only callables or classes evaluating to callables are allowed is to make disambiguation easier, since an array could also be callable. Secondarily, it's a performance optimization, as you can specify class names throughout, and thus have zero additional overhead in the case of a cache hit.
That said, we could have logic like this:
if (! is_callable($provider) && is_string($provider) && class_exists($provider)) {
$provider = new $provider();
}
if (! is_callable($provider) && is_array($provider)) {
return $provider;
}
if (! is_callable($provider)) {
throw new InvalidProviderException();
}
$config = $provider();
if (! is_array($config)) {
throw new InvalidConfigFromProviderException();
}
return $config;
(pseudo-code; I'm not looking at the actual logic right now)
It adds some complexity, and also means there's more possible types allowable in the value, but, as you note, also means we don't have to perform any weird hacks to make it happen.
Would you be willing to open an issue and/or pull request for this against zend-config-aggregator, @MichaelGooden ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created zendframework/zend-config-aggregator#1 to track this. I think we can solve this using the current architecture via a new ArrayProvider
class:
$configCache = ['config_cache_path' => 'data/config-cache.php'];
$aggregator = new ConfigAggregator([
new \Zend\ConfigAggregator\ArrayProvider($configCache),
/* ... */
], $configCache['config_cache_path']);
It's a little extra overhead, but keeps the current semantics, and prevents code complexity from growing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And now I have zendframework/zend-config-aggregator#2 to resolve it. I'll merge it tomorrow, and tag 0.2.0 of that library to contain the new feature; we can then use that version for the skeleton.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With these changes I believe the config/config.php
file will be opcode cache-able again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in my local merge branch.
@xtreamwayz I'm incorporating feedback in a local merge branch, but will need to complete the work tomorrow; at that time I'll push to develop, and start work on the installer changes as discussed in earlier comments. Thanks for the great work on making this happen! |
@xtreamwayz In prepping to run tests, discovered I'd not yet bumped the zend-expressive-router version to 2.0 in the develop branch of zend-expressive. I've done that now. However, tons of test failures now, which I need to resolve. I'll let you know what I find. |
…nager Lightweight module system with config manager
Per conversation on zendframework#54, we want to make production settings the default, which means enabling the configuration cache. However, we also need to make it easy for developers to clear the configuration cache. This patch adds a script, `bin/clear-config-cache.php`, which can be used to remove the configured configuration cache file, if defined. It is aliased in `composer.json` as `clear-config-cache`, allowing usage as `composer clear-config-cache`.
Merged to develop; I'll start work on the installer changes shortly, and submit a new pull request when ready. In the meantime, all tests now run correctly, and I identified some issues with zend-expressive-helpers and zend-expressive-zendviewrenderer in the process. Thanks, @xtreamwayz, for getting this started! |
As discussed in #31.
This is how the config manager can be added. This should be updated once zendframework/zend-expressive-config-manager is created and is probably for version 1.1.