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

Allow injecting ConfigAggregator-based config files #31

Merged
merged 5 commits into from
Jan 9, 2017

Conversation

weierophinney
Copy link
Member

We recently added zend-config-aggregator, which will supercede zend-config-manager (developed by a third-party contributor as a stepping stone). This patch adds support for ConfigAggregator-based configuration files, while maintaining support for ConfigManager-based files.

In order to make this work, we needed separate discovery and injector files. Since both identify the same filesystem location, this also means an injector chain - and those are intended to run each injector. Because of that, the existing ExpressiveConfigInjector and the new ConfigAggregatorInjector now use a new trait, ConditionalDiscoveryTrait, which makes use of discovery classes in order to determine if injection/removal is necessary or possible before performing operations.

Fixes #30

We recently added zend-config-aggregator, which will supercede
zend-config-manager (developed by a third-party contributor as a
stepping stone). This patch adds support for ConfigAggregator-based
configuration files, while maintaining support for ConfigManager-based
files.

In order to make this work, we needed separate discovery and injector
files. Since both identify the same filesystem location, this also means
an injector chain - and those are intended to run _each_ injector.
Because of that, the existing ExpressiveConfigInjector and the new
ConfigAggregatorInjector now use a new trait, ConditionalDiscoveryTrait,
which makes use of discovery classes in order to determine if
injection/removal is necessary or possible before performing operations.
@weierophinney weierophinney added this to the 0.6.0 milestone Dec 20, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 91.205% when pulling 17a2798 on weierophinney:hotfix/30 into e6ae441 on zendframework:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 91.205% when pulling dff4166 on weierophinney:hotfix/30 into e6ae441 on zendframework:master.

@weierophinney
Copy link
Member Author

@gabbydgab Would you have time to test this in the next couple of weeks? You can do so by adding a repository to your application composer.json:

"repositories": [
    {"type": "vcs", "url": "https://github.com/weierophinney/zend-component-installer.git"}
],

and then using an alias constraint:

"zendframework/zend-component-installer": "dev-hotfix/30 as 0.6.0"

and finally running:

$ composer update zendframework/zend-component-installer

Thanks!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 91.205% when pulling 7576839 on weierophinney:hotfix/30 into e6ae441 on zendframework:master.

@gabbydgab
Copy link

@weierophinney I've setup custom modular expressive repo for this. I'll be setting the scenarios by branch (zend-config-aggregator and zend-config-manager).

Following are the scenarios, I can think of, for validation:

  1. Switching from Zend\Expressive\ConfigManager to Zend\ConfigAggregator
    Initially test Zend\ConfigAggregator as default then do the BC test for Zend\Expressive\ConfigManager
  2. Checking the order (array stack) of the injected configuration
    I'll setup at most two repository with extra.zf.component entry listing. Probably a mix of ZF component and 3rd-party library.
    I'm not sure yet how will it prioritized the configuration, is it FIFO? or a scenario that you would merge extra.zf.component configuration on a composer-metapackage type repository.
  3. Removing the installed dependency
    This should also remove the injected configuration in config/config.php file
  4. If caching is enabled while removing the installed dependency
    This should also remove the injected configuration from the cached config (data/config/app_config.php). I hope this will be easy to test. :)
  5. Test should not affect ZendSkeletonApp (MVC)
    Probably out of scope for this test but needs to re-validate.

I haven't started yet but let me know what other scenarios I need to re-validate before I can continue. Thanks for the update.

@gabbydgab
Copy link

My initial test confirms that it is now forward compatible for Zend\ConfigAggregator. Yey!! 😄 👍

I know that scenarios 2 - 5 is covered in the unit test but I want to explore in experimenting composer-metapackage type repository.

Will provide in-depth scenario as a consumer of the package on how would I apply it on my use-case and hopefully would be part of the how-to-tutorial. Hopefully I can finish it after Christmas break. :)

@gabbydgab
Copy link

gabbydgab commented Dec 21, 2016

Probably minor (code sniffer) PSR-2 issue after injection. 🤔

It needs additional indentation to make it cleaner - that the first argument is an array and the second is a string.

use Zend\ConfigAggregator\ConfigAggregator;
use Zend\ConfigAggregator\PhpFileProvider;

$aggregator = new ConfigAggregator(
    [
    \Zend\Filter\ConfigProvider::class,
    \Zend\I18n\ConfigProvider::class,
    \Zend\Router\ConfigProvider::class,
    \Zend\Validator\ConfigProvider::class,
    \Zend\Navigation\ConfigProvider::class,
        new PhpFileProvider('config/autoload/{{,*.}global,{,*.}local}.php'),
        new PhpFileProvider('config/development.config.php') // only override if development mode is ENABLED
    ],
    __DIR__ . '/../data/cache/application.config.php'
);

return $aggregator->getMergedConfig();

NOTE: Config prior injection

use Zend\ConfigAggregator\ConfigAggregator;
use Zend\ConfigAggregator\PhpFileProvider;

$aggregator = new ConfigAggregator(
    [
        new PhpFileProvider('config/autoload/{{,*.}global,{,*.}local}.php'),
        new PhpFileProvider('config/development.config.php') // only override if development mode is ENABLED
    ],
    __DIR__ . '/../data/cache/application.config.php'
);

return $aggregator->getMergedConfig();

@weierophinney
Copy link
Member Author

Not sure what to do about the indentation; the assumption is that the declaration will be in the following format:

$aggregator = new ConfigAggregator([
    /* ... */
], __DIR__ . '/../data/cache/application.config.php');

in which case the generated indentation will be correct. I'll poke around a bit now to see if I can make it discover the indentation and re-use it, though.

Added tests to see if the ConfigAggregator injector correctly indents
injected entries. To do this, I added new start/expected files that use
an alternate coding style for declaring the `ConfigAggregator` instance.
Currently, I observe the following:

- Removal of a package works, and keeps expected indentation
- Injection of a package does not keep expected indentation
@weierophinney
Copy link
Member Author

@gabbydgab The latest commits now enforce using the same indentation as is found in the file itself.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 91.205% when pulling a2d6b4a on weierophinney:hotfix/30 into e6ae441 on zendframework:master.

@weierophinney weierophinney merged commit a2d6b4a into zendframework:master Jan 9, 2017
weierophinney added a commit that referenced this pull request Jan 9, 2017
weierophinney added a commit that referenced this pull request Jan 9, 2017
@weierophinney weierophinney deleted the hotfix/30 branch January 9, 2017 14:48
@weierophinney
Copy link
Member Author

You can update your project to use this code by changing your zend-component-installer constraint to read:

^1.0 || ^0.6 || ^1.0.0-dev

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.

None yet

3 participants