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

Added ability to configure MvcEvent listeners. #3931

Merged
merged 7 commits into from Mar 11, 2013

Conversation

Projects
None yet
3 participants
Contributor

stephanoff commented Mar 1, 2013

Added ability to configure initial list of MvcEvent listeners with config file:

array(
    'listeners' => array('RouteListener', 'DispatchListener', 'ViewManager', 'SendReponseListener'),
)

weierophinney and others added some commits Mar 1, 2013

Contributor

blanchonvincent commented Mar 2, 2013

@stephanoff I think you should have an array "$defaultListeners" with 'RouteListener', 'DispatchListener', 'ViewManager', 'SendResponseListener' and do a merge with the "$listeners" in bootstrap() before attach. In this case, you can override only one listener, you should not re-write all the listeners.

Contributor

blanchonvincent commented Mar 2, 2013

@stephanoff can you add unit tests with override listeners ?

Contributor

stephanoff commented Mar 2, 2013

@blanchonvincent I thought about list of default listeners, but I guess that setting hardcoded list of listeners will prevent developers to build application with custom set of listeners.

Contributor

stephanoff commented Mar 2, 2013

@blanchonvincent but from other side, developers can use custom implementation of Zend\Mvc\Application with overriden Application::bootstrap() method for more flexible configuration. Ok, I will add default set of listeners.

@stephanoff stephanoff Merged listeners with default set of listeners.
Change-Id: I382bc52191f3202b47ecd7910a2ffb4108c7e78e
ddde86b
Contributor

blanchonvincent commented Mar 2, 2013

@stephanoff i would say :

$defaultListeners = array('RouteListener', 'DispatchListener', 'ViewManager', 'SendReponseListener');
$listeners = array_merge($defaultListeners, $listeners);

"array_merge" keep second value for two keys, with that each developer can override all or only one listener.

Contributor

blanchonvincent commented Mar 2, 2013

@stephanoff you must add unit tests with the PR

Contributor

blanchonvincent commented Mar 2, 2013

@stephanoff you PR is really cool, i like and need that :)
If you are not familiar with unit tests, i will help you.

Contributor

stephanoff commented Mar 2, 2013

Thank you for assistance. I'm familiar with phpunit, but restricted with time. Give me one hour.

stephanoff added some commits Mar 2, 2013

@stephanoff stephanoff Added test for custom application listeners.
Change-Id: Ib47b1604675e5beef8945b57df9dfb28ddef3ffd
fb0dfc0
@stephanoff stephanoff Fixed typo in application's default listeners.
Change-Id: Ifb6241ff0d3799dbefdd9bd160e7f14c3ab68bf8
d96aa9b
Contributor

stephanoff commented Mar 2, 2013

@blanchonvincent Added test for custom listener. I'm not sure that this is best case, may be you can suggest more better approcah.

Contributor

stephanoff commented Mar 2, 2013

But using array_merge we cannot be ensure uniquess of listeners.
Regarding unit tests - I will replace my test with your.

Contributor

blanchonvincent commented Mar 2, 2013

@stephanoff oops => array_unique(array_merge($defaultListeners, $listeners))

stephanoff added some commits Mar 2, 2013

@stephanoff stephanoff Minor improvement in bootstrap method listeners merge.
Change-Id: Ib79a1e12bf93f53f44765ac2add526c3ccecfa6f
366e395
@stephanoff stephanoff Updated test for custom application listener.
Change-Id: I32e556441e7ba46367b1a1a7485ab00e0cdcd6df
ae18b80
Contributor

stephanoff commented Mar 3, 2013

@blanchonvincent Updated Application and TestCase classes.

@ghost ghost assigned weierophinney Mar 8, 2013

@weierophinney weierophinney commented on the diff Mar 11, 2013

library/Zend/Mvc/Application.php
{
$serviceManager = $this->serviceManager;
$events = $this->getEventManager();
- $events->attach($serviceManager->get('RouteListener'));
- $events->attach($serviceManager->get('DispatchListener'));
- $events->attach($serviceManager->get('ViewManager'));
- $events->attach($serviceManager->get('SendResponseListener'));
+ $defaultListeners = array(
@weierophinney

weierophinney Mar 11, 2013

Owner

I'd actually make this a class property. That would make overriding them simpler, as the developer wouldn't need to override the bootstrap() method, only the property. (I can do this during merge.)

@stephanoff

stephanoff Mar 12, 2013

Contributor

Sounds good.

@weierophinney weierophinney added a commit that referenced this pull request Mar 11, 2013

@weierophinney weierophinney Merge pull request #3931 from stephanoff/develop-application
Added ability to configure MvcEvent listeners.
8334ed7

@weierophinney weierophinney added a commit that referenced this pull request Mar 11, 2013

@weierophinney weierophinney [#3931] CS fixes
- trailing whitespace
4f0f855

@weierophinney weierophinney merged commit ae18b80 into zendframework:develop Mar 11, 2013

1 check failed

default The Travis build failed
Details

@stephanoff stephanoff deleted the unknown repository branch Mar 12, 2013

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