Skip to content
New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix broken event system #677

Merged
merged 4 commits into from
Aug 25, 2018
Merged

Fix broken event system #677

merged 4 commits into from
Aug 25, 2018

Conversation

roelvanduijnhoven
Copy link
Contributor

@roelvanduijnhoven roelvanduijnhoven commented Nov 23, 2017 •
edited
Loading

See comment below.

Roel van Duijnhoven added 4 commits November 23, 2017 13:39
Make SharedEventManager usable
b7ca55c 
Works now similar to how https://github.com/zendframework/zend-eventmanager/blob/master/src/EventManagerAwareTrait.php works.
Inject EventManager ourselves.
ffe3fc8 
Such that attached listeners set in constructor will not be overwritten
later by Zend/Mvc's initializer.
Use proper way to pass custom event.
aaff500 
This way the listeners in ZF2 style do not have to be adapted.
Remove triggers that are unsuable.
1c4c15e 
The only way to use them was via the shared service manager. But in ZF3
this manager is only instantiated after construction. Thus this has no
function anymore.
@roelvanduijnhoven roelvanduijnhoven changed the title Make SharedEventManager usable Fix broken event system Nov 27, 2017
roelvanduijnhoven pushed a commit to Webador/JwPersistentUser that referenced this pull request Nov 27, 2017
Do not rely on hack introduced in ZF3. This works on top of the patch-1
59aada5 
as introced in ZF-Commons/ZfcUser#677.
@roelvanduijnhoven roelvanduijnhoven mentioned this pull request Nov 27, 2017
Open
@roelvanduijnhoven
Copy link
Contributor Author

roelvanduijnhoven commented Nov 27, 2017 •
edited
Loading

This PR tries to make the (shared) event system functional again under ZF3. See #678. It addresses all these points by:

  • Make sure to tag all classes using the EventManager with EventManagerAwareInterface.
  • Make sure that we do not set a shared manager ourselves; otherwise ZF3 will not properly inject its own version.
  • Set the EventManager directly ourselves on the AdapterChain method such that events set in the factory are not lost.
  • Send a custom AdapterChainEvent to listeners of AdapterChain. This is in line with how it was under ZF2 and does not require hacks.

With these fixes our app functions the way it did before. The only change was to replace the init events with a delegator for those forms.

The tests do need to be updated. But wanted to get some feedback first about if this will be merged.
@Danielss89 What do you think about this?

@roelvanduijnhoven
Copy link
Contributor Author

Would love to hear some feedback @Danielss89, maybe @Netiul? Now working of a patched version myself; but not ideal!

APaikens
APaikens approved these changes Mar 27, 2018
View reviewed changes
Copy link

@APaikens APaikens left a comment

Choose a reason for hiding this comment

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

Tested locally, these changes work - and it helps a lot.

@Danielss89
Copy link
Member

Looks good to me. Haven't tested this yet though.

@olexp
Copy link

olexp commented Jul 12, 2018

This can be removed from adapters and AdapterChain:
use Zend\EventManager\EventInterface;

These changes work for me. @Danielss89 could you merge the request?

@asgrim
Copy link

asgrim commented Aug 25, 2018

@Danielss89 poke on this? :)

@Danielss89 Danielss89 merged commit 03c8d81 into ZF-Commons:3.x Aug 25, 2018
@asgrim
Copy link

asgrim commented Aug 26, 2018

Thanks! 👍

@roelvanduijnhoven
Copy link
Contributor Author

Thanks @Danielss89 a bunch: much appreciated. Now I can stop with working based of strange forks and branches. Could you perhaps release a new version?

@zluiten
Copy link
Contributor

zluiten commented Nov 15, 2018

What about releasing a new version @Danielss89 ?

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

@asgrim asgrim asgrim approved these changes

@bartmcleod bartmcleod bartmcleod approved these changes

@APaikens APaikens APaikens approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@roelvanduijnhoven @Danielss89 @olexp @asgrim @zluiten @bartmcleod @APaikens