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

Give modules the ability to modify application config after their own co... #4505

Merged
merged 1 commit into from
May 24, 2013
Merged

Conversation

superdweebie
Copy link
Contributor

...nfig is merged in (including the ability to remove keys)

@superdweebie
Copy link
Contributor Author

Ref #3823

@superdweebie
Copy link
Contributor Author

This is another attempt at the remove config keys issue. It feels like a fairly clean solution.

Comments please.

@@ -89,7 +84,8 @@ public function attach(EventManagerInterface $events)
}

$this->callbacks[] = $events->attach(ModuleEvent::EVENT_LOAD_MODULE, array($this, 'onLoadModule'));
$this->callbacks[] = $events->attach(ModuleEvent::EVENT_LOAD_MODULES, array($this, 'onLoadModulesPost'), -1000);
$this->callbacks[] = $events->attach(ModuleEvent::EVENT_MERGE_CONFIG, array($this, 'onMergeConfig'), 1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a BC break.

@prolic
Copy link
Contributor

prolic commented May 20, 2013

It looks pretty solid to me, but I fear you broke BC with your new mergeConfig event.

@superdweebie
Copy link
Contributor Author

@prolic I've thought about this more, and reworked it a little. It's simpler now, and I don't think there are any BC breaks. ModuleManager test suite passes only with changes to the number of event listeners (because there is one new one).

In ConfigListener I also changed the name of the onLoadModulesPost event to onLoadModules. It was confusing - despite it's name, it is triggered by EVENT_LOAD_MODULES not EVENT_LOAD_MOSULES_POST.

@prolic
Copy link
Contributor

prolic commented May 21, 2013

good for me

@weierophinney
Copy link
Member

On Monday, May 20, 2013, Tim Roediger wrote:

@prolic https://github.com/prolic I've thought about this more, and
reworked it a little. It's simpler now, and I don't think there are any BC
breaks. ModuleManager test suite passes only with changes to the number of
event listeners (because there is one new one).

In ConfigListener I also changed the name of the onLoadModulesPost event
to onLoadModules. It was confusing - despite it's name, it is triggered
by EVENT_LOAD_MODULES not EVENT_LOAD_MOSULES_POST.

This is a no-go, as any listener on the previous event name will no longer
be triggered. That change must be reverted to maintain backwards
compatibility.


Reply to this email directly or view it on GitHubhttps://github.com//pull/4505#issuecomment-18180230
.

Matthew Weier O'Phinney
[email protected]
http://mwop.net/

@superdweebie
Copy link
Contributor Author

@weierophinney sorry, I didn't express myself properly. I haven't changed any event names, as you say, that would certainly be a BC break. This is what I have done:

Before: ConfigListener::onLoadModulesPost called when EVENT_LOAD_MODULES triggered (I think that's confusing.)
After: ConfigListener::onLoadModules called when EVENT_LOAD_MODULES triggered

All I have changed is the method name to better reflect the event it is attached to.

@weierophinney
Copy link
Member

@superdweebie This looks good -- essentially, all you're doing is triggering an additional event, and modules can hook into that in order to change the config if desired -- is that correct?

Can you write up a sample showing how this would be done, and how it solves the "I want to remove a config key dynamically" problem? That way, those searching and landing here will know how it works, plus we have something we can copy into the docs.

Once I see that, I'll merge.

@ghost ghost assigned weierophinney May 23, 2013
@superdweebie
Copy link
Contributor Author

Some docs...

This PR introduces a new ModuleManager Event named CONFIG_MERGE. It is triggered by Zend\ModuleManager\ConfigListener::onLoadModules.
The order of events is:

ModuleManager triggers loadModules
ModuleManager triggers loadModule
ConfigListener triggers configMerge
ModuleManager triggers loadModulesPost

The CONFIG_MERGE event is triggered after all application configuration files from all sources have been loaded, including module configuration
and globs, but before those configs have been merged, cached or used.

ConfigListener::onMergeConfig is attached to CONFIG_MERGE at high prioirity (1000), and will merge all the configs.

To alter the merged application config before it is used or cached, attach to CONFIG_MERGE at a lower priority. eg:

<?php

use Zend\ModuleManager\ModuleEvent;
use Zend\ModuleManager\ModuleManager;

class Module
{

    public function init(ModuleManager $moduleManager) {
        $eventManager->attach(ModuleEvent::EVENT_MERGE_CONFIG, array($this, 'onConfigMerge'), 1);
    }

    public function onConfigMerge(ModuleEvent $event) {

                    //get the complete merged application config
        $config = $event->getConfigListener()->getMergedConfig(false);

        //Do something here to modify config.
        //In particular you can delete config keys. eg:
        //  unset($config['mykey']);

                    //return the modified config back to the ConfigListener ready for caching and use           
        $event->getConfigListener()->setMergedConfig($config);     
    }

}

Note that if a cached config is used by ModuleManager then CONFIG_MERGE will not be triggered.

@superdweebie
Copy link
Contributor Author

If you're reading this thread on email, I've edited the previous message to fix some typos.

@EvanDotPro
Copy link
Member

👍 This is pretty much exactly what I had in mind for a solution. Much more explicit than the other strategy, I like it.

@weierophinney weierophinney merged commit 6e48271 into zendframework:develop May 24, 2013
weierophinney added a commit that referenced this pull request May 24, 2013
Give modules the ability to modify application config after their own co...
weierophinney added a commit that referenced this pull request May 24, 2013
@weierophinney
Copy link
Member

This is now merged; I'm working on an "advanced configuration" tutorial that will document this new feature currently, and will reference this issue when I post the pull request for it.

@superdweebie
Copy link
Contributor Author

Thanks @weierophinney and @EvanDotPro. It's nice to have this one resolved, and with a good clean solution too.

@superdweebie superdweebie deleted the resolve-3823 branch May 25, 2013 01:36
weierophinney added a commit to zendframework/zend-modulemanager that referenced this pull request May 15, 2015
…/resolve-3823

Give modules the ability to modify application config after their own co...
weierophinney added a commit to zendframework/zend-modulemanager that referenced this pull request May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants