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

Use a factory to create the SendResponseListener #198

Conversation

weierophinney
Copy link
Member

Reported in:

The SendResponseListener was lazy-instantiating an event manager on first request to getEventManager(). However, because initializers run after delegators, this meant that the EM instance composed did not have a shared EM instance, which triggered the initializer to re-inject, losing any listeners injected by delegators.

This patch introduces a factory for the SendResponseListener. The factory creates the instance, and then injects it with an EM instance pulled from the container; since these are guaranteed to have a shared EM instance, the initializer will skip injection, keeping any listeners injected by delegators.

Reported in:

- zendframework/zend-mvc-console#10
- zendframework/zend-mvc-console#11
- zendframework/zend-mvc-console#12

The `SendResponseListener` was lazy-instantiating an event manager on
first request to `getEventManager()`. However, because initializers run
after delegators, this meant that the EM instance composed did not have
a shared EM instance, which triggered the initializer to re-inject,
losing any listeners injected by delegators.

This patch introduces a factory for the `SendResponseListener`. The
factory creates the instance, and then injects it with an EM instance
pulled from the container; since these are guaranteed to have a shared
EM instance, the initializer will skip injection, keeping any listeners
injected by delegators.
@GeeH
Copy link
Contributor

GeeH commented Aug 29, 2016

Looks fine to me. 👍

@GeeH GeeH closed this Aug 29, 2016
@GeeH GeeH reopened this Aug 29, 2016
@weierophinney
Copy link
Member Author

.03% is statistically insignificant. i really wish we could tell coveralls to not report failure when the threshold is that low. 😠

@weierophinney
Copy link
Member Author

Thanks, @GeeH - I'll merge and tag with 3.0.3, now that I've merged the other PRs.

weierophinney added a commit that referenced this pull request Aug 29, 2016
@svycka
Copy link
Contributor

svycka commented Aug 30, 2016

@weierophinney but you can change this in coveralls settings if you want. There is two options:

  • COVERAGE THRESHOLD FOR FAILURE
  • COVERAGE DECREASE THRESHOLD FOR FAILURE
    you can set these as you wish.

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.

3 participants