Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

make 'listeners' key can be configured outside application.config.php #5612

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

samsonasik commented Dec 14, 2013

because in ZF2 Skeleton App use : Zend\Mvc\Application::init(require 'config/application.config.php')

Member

Ocramius commented Dec 14, 2013

can you give more insight on what you are doing here?

Contributor

samsonasik commented Dec 14, 2013

@Ocramius for example : I make module and has custom 'listeners' config on my module for specific purpose, and the 'listeners' key will not be called on serviceManager->get('Application')->bootstrap($listeners) on Zend\Mvc\Application::init because old $listeners is comes from application.config.php ( in ZF2 skeleton case ).

Contributor

blanchonvincent commented Dec 15, 2013

@samsonasik Hmmm to avoid BC maybe we can have a merge with listeners from app config and listeners from modules config ?

Contributor

samsonasik commented Dec 15, 2013

@blanchonvincent done ;). please let me know if I missed something ;)

@blanchonvincent blanchonvincent and 1 other commented on an outdated diff Dec 15, 2013

library/Zend/Mvc/Application.php
$serviceManager = new ServiceManager(new Service\ServiceManagerConfig($smConfig));
$serviceManager->setService('ApplicationConfig', $configuration);
$serviceManager->get('ModuleManager')->loadModules();
+
+ $config = array_unique(array_merge($configuration, $serviceManager->get('Config')));
@blanchonvincent

blanchonvincent Dec 15, 2013

Contributor

@samsonasik maybe can we merge only $configuration['listeners'] and $serviceManager->get('Config')['listeners'] it will be faster ?

@samsonasik

samsonasik Dec 16, 2013

Contributor

@blanchonvincent done, now only merge with key 'listeners'

Contributor

blanchonvincent commented Dec 16, 2013

👍

@weierophinney weierophinney commented on an outdated diff Mar 3, 2014

library/Zend/Mvc/Application.php
$serviceManager = new ServiceManager(new Service\ServiceManagerConfig($smConfig));
$serviceManager->setService('ApplicationConfig', $configuration);
$serviceManager->get('ModuleManager')->loadModules();
+
+ $configurationListeners = isset($configuration['listeners']) ? $configuration['listeners'] : array();
+ $config = $serviceManager->get('Config');
+ $configListeners = isset($config['listeners']) ? $config['listeners'] : array();
@weierophinney

weierophinney Mar 3, 2014

Owner

These variable names are very confusing. May I suggest $listenersFromAppConfig and $listenersFromConfigService instead?

@weierophinney weierophinney commented on an outdated diff Mar 3, 2014

library/Zend/Mvc/Application.php
$serviceManager = new ServiceManager(new Service\ServiceManagerConfig($smConfig));
$serviceManager->setService('ApplicationConfig', $configuration);
$serviceManager->get('ModuleManager')->loadModules();
+
+ $configurationListeners = isset($configuration['listeners']) ? $configuration['listeners'] : array();
+ $config = $serviceManager->get('Config');
+ $configListeners = isset($config['listeners']) ? $config['listeners'] : array();
+
+ $listeners = array_unique(array_merge($configurationListeners, $configListeners));
@weierophinney

weierophinney Mar 3, 2014

Owner

I'd argue that application-level configuration listeners should take precedence over those defined in modules, instead of the other way around (which is what you have here).

@weierophinney weierophinney added this to the 2.3.0 milestone Mar 3, 2014

Contributor

samsonasik commented Mar 3, 2014

@weierophinney done. fixed. please let me know if I missed something ;)

@weierophinney weierophinney added a commit that referenced this pull request Mar 4, 2014

@weierophinney weierophinney Merge pull request #5612 from samsonasik/listener/outside.application…
….config

make 'listeners' key can be configured outside application.config.php
fb28f08

@weierophinney weierophinney added a commit that referenced this pull request Mar 4, 2014

@weierophinney weierophinney Merge branch 'feature/5612' into develop
Close #5612
828015f
Owner

weierophinney commented Mar 4, 2014

Merged to develop for release with 2.3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment