Refactored to use upcoming EventManager v3 #31
Conversation
Updates all code to work with the upcoming v3 of EventManager. In particular: - Updated the EventManager factory to inject the SharedEventManager at instantiation, if that service is present. - Updated the EventManager initializer to inject not only if no EM instance is present, but also if the current EM instance has no shared manager present. The latter is a good indication that the EM instance was lazy-loaded by the getter. - Updated the Application constructor to pass arguments. This was necessary due to a strange circular dependency with the changes to the initializer. It's also better DI. - Fixed all `attachAggregate()` calls to use `$aggregate->attach()`, and ensured there were no calls to `$events->attach()` that were attaching aggregates. - Reviewed all `trigger()` calls for appropriate signatures, updating those that needed it. In many cases, an additional call to `$event->setName()` was also required. This can likely be merged to develop sooner rather than later, as it represents a work-in-progress towards the updated MVC.
|
Pinging @zendframework/community-review-team and @ezimuel — would love some review. 😄 |
| public function __construct( | ||
| $configuration, | ||
| ServiceManager $serviceManager, | ||
| EventManagerInterface $events, |
There was a problem hiding this comment.
Should we provide defaults to avoid BC break?
There was a problem hiding this comment.
This is a BC break but we are working to mvc 3.0 if it is a GOOD BC break this is a good time to merge it
There was a problem hiding this comment.
@gianarb it is kind of a big one, tbh, and it can be avoided
There was a problem hiding this comment.
Putting a note here as threads got broken up by me answering via email.
This change was for a reason, which I stated in the PR summary: I ended up with a circular reference issue with the ApplicationFactory + EventManagerAware initializer that resulted in invalid EventManager state. The result was that the EM instance injected no longer had the default listeners injected, which broke the entire thing.
As I noted in another comment elsewhere: hardly anybody directly instantiates the Application instance. The skeleton application pulls it from the service manager. As such, we can call this change out in the migration documentation for the very small percentage of users who are manually instantiating.
- Updated EventManagerIntrospectionTrait:
- Made methods private.
- Added `getArrayOfListenersForEvent()`; casts to array before returning; used
for counting listeners and/or traversal where priority isn't interesting.
- Added `assertListenerAtPriority()` for simplifying assertions looking for
specific listeners registered at specific priorities.
- Added docblocks to all methods.
- Updated tests per the above.
- Various other small changes as suggested by @Ocramius.
|
Feedback incorporated, @Ocramius 😄 |
|
Bah, CS checks! Fixed and pushed. |
|
Thanks @weierophinney! |
| @@ -14,11 +14,12 @@ | |||
| }, | |||
| "require": { | |||
| "php": ">=5.5", | |||
| "zendframework/zend-eventmanager": "~2.5", | |||
| "zendframework/zend-eventmanager": "dev-develop as 2.7.0", | |||
There was a problem hiding this comment.
Note: should probably use zend-eventmanager dev-develop as 3.0.0
There was a problem hiding this comment.
Can't yet; not all components are updated to zend-eventmanager v3 yet, which means that there will be an unresolveable conflict. (I know; I tried. 😄)
These cover the BC breaks created with the current patch.
Also updates dev-develop branch to 3.0-dev.
|
Added a migration guide covering the BC breaks introduced, and updated the branch-alias for the develop branch to 3.0-dev. |
|
Okay, merging. If anybody finds anything else wrong, please open an issue or PR and ping me. 😄 |
Updates all code to work with the upcoming v3 of EventManager. In particular:
attachAggregate()calls to use$aggregate->attach(), and ensured there were no calls to$events->attach()that were attaching aggregates.trigger()calls for appropriate signatures, updating those that needed it. In many cases, an additional call to$event->setName()was also required.This can likely be merged to develop sooner rather than later, as it represents a work-in-progress towards the updated MVC.