Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

Lightweight module system with config manager #54

Merged
merged 19 commits into from
Jan 12, 2017
Merged

Lightweight module system with config manager #54

merged 19 commits into from
Jan 12, 2017

Conversation

geerteltink
Copy link
Member

@geerteltink geerteltink commented Jan 7, 2016

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.

@geerteltink geerteltink added this to the 1.1.0 milestone Jan 7, 2016
$cachedConfigFile = 'data/cache/app_config.php';
$configManager = new ConfigManager([
// Load config from all modules first
new PhpFileProvider('src/*/Config/{{,*.}global,{,*.}local}.php'),
Copy link

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 in config/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).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xtreamwayz

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?

Copy link
Member Author

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?

Copy link

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.

Copy link

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.

@geerteltink
Copy link
Member Author

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).

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 config/config.php.

@samsonasik
Copy link

need rebase

@RalfEggert
Copy link

@weierophinney @xtreamwayz
What is the status of this?

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

@geerteltink
Copy link
Member Author

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.

@samsonasik
Copy link

@xtreamwayz another rebase needed

@geerteltink
Copy link
Member Author

I had to do a git push --force. I hope it didn't mess up the history.

@gabbydgab
Copy link

Suggestions:

see related changes and implementation in https://github.com/gabbydgab/ZFExpressiveModularApp

@weierophinney
Copy link
Member

I like the direction of this, but have a few comments; requests.

  • I've ported @mtymek's component to zendframework/zend-config-aggregator; you can use that now, specifying ^0.1.0 for the constraint. This will also require updating config/config.php to use the new namespace (Zend\ConfigAggregator) and class name (ConfigAggregator).

  • If we do not change the structure of the src/App/ directory (e.g., to add src/, config/, and templates directories), I'd recommend that instead of dropping in configuration files, we generate a ConfigProvider class to drop into it. This will be closer to what folks will see when installing ZF modules, and will be immediately autoloadable due to the composer autoload rules we have in place already; the config/config.php file would then reference App\ConfigProvider::class.

  • Alternately, we should demonstrate what a module looks like within an Expressive application, and move the templates under the src/App directory, create a src/ subdirectory, and potentially a src/config/ subdirectory (though that may not be required if we go the ConfigProvider route).

    This approach would also require that we add a tool for injecting an application-defined module in the configuration, something along the lines of ./vendor/bin/expressive-register "ModuleName\ConfigProvider", which would add the specified provider to the config/config.php file.

    The big caveat with this approach is that this will mean that we will require different documentation based on 1.0 versus 1.1 versions of Expressive, as the structure will vary considerably.

  • Since we're now recommending programmatic pipelines and routing, I'm not sure what this will mean for routing configuration. I'm thinking we may want to create delegator factories on the Application instance, which would then inject routes. Thoughts?

  • I agree with @gabbydgab regarding including zf-development-mode, and a related application-level dist config file for purposes of enabling/disabling the selected development settings (vs automatically writing the local.php file).

Thoughts, @xtreamwayz ?

@danizord
Copy link

danizord commented Dec 9, 2016

Since we're now recommending programmatic pipelines and routing, I'm not sure what this will mean for routing configuration. I'm thinking we may want to create delegator factories on the Application instance, which would then inject routes. Thoughts?

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.

@geerteltink
Copy link
Member Author

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:

  • Install module x?
  • Enable routes for module x?
  • Enable pipeline for module x?

That brings me to a challenge I encountered when writing tests. How can you pipe middleware at a specific position?

  • Before pipeRoutingMiddleware()
  • After pipeRoutingMiddleware()
  • Before pipeDispatchMiddleware()
  • After pipeDispatchMiddleware()

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.

@geerteltink geerteltink changed the base branch from master to develop December 10, 2016 19:20
@weierophinney
Copy link
Member

Notes based on the feedback/questions from @danizord and @xtreamwayz.

First, addressing @danizord:

Since we're now recommending programmatic pipelines and routing, I'm not sure what this will mean for routing configuration. I'm thinking we may want to create delegator factories on the Application instance, which would then inject routes. Thoughts?

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.

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 ConfigProvider, so that's easy enough.

The second aspect, though, is providing information on:

  • Pipeline middleware, and where in the pipeline it should exist. For instance, does it operate as a fall-through/failsafe (e.g., 404 handler)? Should it run on every request, or only when a routed request has succeeded? (e.g., adding an authorization or rate-limit check to a given routed request.) These will likely just require documentation, but we may want to provide a standard way of documenting these details. (I'm not 100% convinced we would need to come up with this now, however.)

  • Routed middleware often expects specific parameters in the routing expression: /user/:user_id/address/:address_id. As such, this is definitely a candidate for having some form of configuration or functionality for adding routes, to prevent copy-paste errors.

Regarding this statement by @xtreamwayz:

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 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 src/ directory, it would be useful to be able to add these to the configuration, and that's where the proposed expressive-register tool comes into play. If we also added zfcampus/zf-composer-autoloading to the skeleton as a dev requirement, the workflow might be something like this:

  • Create a new module by creating a new src/ subdirectory, a ConfigProvider class within it, using the subdirectory as the namespace.
  • Call ./vendor/bin/autoload-module-via-composer {ModuleName}
  • Call ./vendor/bin/expressive-register {ModuleName}

We could even create a binary in the skeleton or within zend-expressive-tooling that performs all three steps.

Regarding this:

I think modules should be able to work out of the box. Or at least in a few easy steps with questions:

  • Install module x?
  • Enable routes for module x?
  • Enable pipeline for module x?

I agree to an extent. The first question is already taken care of via zend-component-installer, and potentially via the expressive-register command for local modules. The second goes back to the discussion above about routed middleware; however, there are good reasons not to do this automatically (routing conflicts, need to segregate by subpath, etc.). The third I do not think we should address out-of-the-box; pipeline middleware really should be added manually, as we cannot accurately guess where in the pipeline it should fit, particularly with a mix of third-party and application-specific middleware.

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:

  • Should the App namespace be exposed as a module? If so, what does this mean, exactly, in terms of the skeleton structure?

  • How should we handle exposing routed middleware from a module?

Proposed App module structure

I propose that App become a module. In this particular case, that will mean the following:

src/ layout

The src/ directory will now have the following layout (minus the ExpressiveInstaller subdirectory, which is removed following initial install):

src/
    App/
        src/
            Action/
                HomePageAction.php
                HomePageFactory.php
                PingAction.php
            ConfigProvider.php
        templates/
            home-page.{phtml,html.twig}

ConfigProvider

ConfigProvider will have the following:

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 changes

This will require that composer.json be updated to specify that the App\\ PSR-4 namespace maps to src/App/src/.

Templates changes

Additionally, the root-level templates/ directory will be updated to remove the app/ subdirectory.

Installer changes

The installer will need to be updated to:

  • Create the new directory structure for the App module.
  • Create the ConfigProvider. In should be the same across all sets of containers, routers, and/or template providers.
  • Install the home page template to the new location.
  • Register the App module in the config/config.php file; this may be possible using the zend-component-installer functionality.
  • Update the application-level configuration for templates to remove information on the app templates.

Module-specific routing

With programmatic pipelines and routing being the new recommendation, how can modules provide routing? There are definitely huge reasons to do so:

  • Routes typically have dynamic parameters which routed middleware will depend upon.
  • Routes may also provide constraints for those dynamic parameters, ensuring that the parameters are valid once the middleware receives them.

That said, there are problems with providing them automatically:

  • The module author cannot know which router implementation is in use. Since routing syntax varies from implementation to implementation, consuming routes blindly could lead to application issues.
  • The application developer may need to segregate the routes under a different path segment.
  • The application developer may need to rewrite the routes to prevent conflicts with other routes in the application.
  • The application developer may only want to use a portion of the routes.

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:

  • The original container
  • The service name
  • A callback by which the service will be generated

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 ConfigProvider, this delegator factory should not be included in the dependency configuration returned by that class. That choice should be left to the application developer, so that if there are routing conflicts, or if the functionality needs to be segregated under a subpath, they can make that decision.

At the application level, if the application developer wants to include the routes verbatim, they can do the following in config/autoload/dependencies.global.php:

return [
    'dependencies' => [
        // ...
        'delegators' => [
            Application::class => [
                \Acme\Foo\ApplicationRoutesDelegatorFactory::class,
                // additional delegators as needed
            ],
        ],
    ],
];

Developer experience

We will eventually need to provide a way for third-party module authors to notify users during installation about these delegator factories, and potentially provide a console-based solution to wire them; e.g.:

$ ./vendor/bin/expressive-wire-delegator "Zend\\Expressive\\Application" "Acme\\Foo\\ApplicationRoutesDelegatorFactory"

Benefits

This approach has several benefits:

  • It re-uses existing functionality. ConfigProviders continue to provide the module configuration and dependencies. An optional delegator factory allows the application developer to opt-in to also wiring the module-specific routing.

  • These delegators can also be called manually for containers that do not support them (assuming they do not implement the zend-servicemanager-specific DelegatorFactoryInterface):

    // in public/index.php, before or after the call to require 'config/routes.php`:
    (new \Acme\Foo\ApplicationRoutesDelegatorFactory())(
        $container,
        \Zend\Expressive\Application::class,
        function () use ($app) {
            return $app;
        }
    );
  • Module developers can choose what routers they want to support, including alternate third-party router implementations, and how to handle inability to support the current router.

Problems

The primary problems are as follows:

  • We cannot restrict what the delegator factory performs. It could also register pipeline middleware, or call methods like raiseThrowables(). As such, it would be up to the application developer to audit the implementation and ensure it does nothing untoward.

  • We cannot prevent module authors from auto-registering the delegator factory. Again, this then becomes the onus of the application developer to audit the ConfigProvider and ensure it does not automatically register delegator factories that provide routing.

  • Getting a full list of registered routes becomes more difficult. Since not all routes will now be in config/routes.php, we will likely need to provide some way of introspecting a router to get routing information for either a CLI tool or for solutions such as Z-Ray or phpdebugbar. (This is already a problem, as routing can be segregated across multiple config files; as such, this is a problem we need to solve regardless of what we do. Credits to @ezimuel for pointing this out.)

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 Application class, getRoutes(), which would return the value of the $routes property. Developers could then iterate through this and examine the Route instances it contains to get information such as the path, name, allowed methods, etc.

@geerteltink
Copy link
Member Author

That's a lot of text. I need to read a bit more thoroughly tomorrow. My first reaction:
I agree with making App a module. I'll work on the required changes tomorrow.

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.

@weierophinney
Copy link
Member

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 ConfigProvider class in your module, and that can be as straight-forward as:

namespace Acme;

class ConfigProvider
{
    public function __invoke()
    {
        return [
            'dependencies' => [ /* ... */ ],
            'acme' => [ /* ... */ ],
        ];
    }
}

For registering middleware, we likely wouldn't have wanted modules to auto-register middleware_pipeline configuration anyways, as that's something to configure on the application level; however, the middleware services can and should be registered. That means the developer only needs to do the following:

  • Open config/pipeline.php
  • Add an appropriate statement at the appropriate location (e.g., $app->pipe(\Some\ThirdParty\Middleware::class);)

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:

  • Open config/autoload/dependencies.global.php
  • Add a delegators section under dependencies if none exists.
  • Add an entry for Application::class under dependencies.delegators, if none exists.
  • Add an array item indicating the class name of the delegator factory to that entry.

The original comment provides an example. We could even provide a script to make this simpler for end-users.

tl;dr

What follows is a tl;dr of the original comment, as actionable steps.

  • Make App a module, with the recommended structure as detailed above.
  • Add a ConfigProvider to the App module, with its list of dependencies and the module-specific templates settings.
  • Update the installer to support the above.
  • Recommend modules for Expressive provide a ConfigProvider class, registered in their composer.json, so that they can provide dependencies and other configuration to applications.
  • Recommend modules for Expressive define a delegator factory that will inject the Application instance with any routes, but also recommend that this factory not be wired in the ConfigProvider; it will be up to the application developer to determine if they want to wire it in.

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'),

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.

Copy link
Member Author

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

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

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,

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?

Copy link

@MichaelGooden MichaelGooden Dec 14, 2016

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.

Copy link
Member Author

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`.

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.

Copy link
Member Author

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.

@geerteltink
Copy link
Member Author

geerteltink commented Dec 14, 2016

@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.

Copy link
Member

@weierophinney weierophinney left a 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"],
Copy link
Member

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!

Copy link
Member Author

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,
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member

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,
Copy link
Member

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...

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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;
},
Copy link
Member

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...)

Copy link
Member Author

@geerteltink geerteltink Jan 10, 2017

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.

Copy link
Member

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.

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.

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?

Copy link
Member

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 ?

Copy link
Member

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.

Copy link
Member

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.

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.

Copy link
Member

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.

@weierophinney
Copy link
Member

@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!

@weierophinney
Copy link
Member

@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.

weierophinney added a commit to weierophinney/zend-expressive-skeleton that referenced this pull request Jan 12, 2017
…nager

Lightweight module system with config manager
weierophinney added a commit to weierophinney/zend-expressive-skeleton that referenced this pull request Jan 12, 2017
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`.
@weierophinney weierophinney merged commit 117f2b1 into zendframework:develop Jan 12, 2017
weierophinney added a commit that referenced this pull request Jan 12, 2017
weierophinney added a commit that referenced this pull request Jan 12, 2017
@weierophinney
Copy link
Member

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!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants