Repository navigation
Move hooks and emitters all to the eventdispatcher #14552
Description
We have these old hooks and eventemitters still around.
I vote to move them all to the eventdispatcher. Then we can still have the events emitted as a legacy solution for a few releases. And then kill them off. That way we'll at some point finally have 1 unified system.
CC: @ChristophWurst @nickvergessen
Migrated events
The following collection lists legacy hooks/events/emitters that continue to trigger their predecessor to keep backward compatibility. They need to be removed once the old methods are dropped:
Activity
nickvergessen commentedon Mar 5, 2019
Yes
nickvergessen commentedon Mar 6, 2019
Especially the emitter stuff is really annoying because that means you have to create the objects. But yeah, we should be able to move them all into a legacy listener which is then deprecated for ~19?
ChristophWurst commentedon Mar 6, 2019
👍 absolutely!
I would also like to see more events using custom objects https://symfony.com/doc/current/components/event_dispatcher.html#creating-an-event-class. We can add those to OCP and properly version them. This makes it also less error prone to add new parameters in the future. Moreover they are probably easier to discover that way.
rullzer commentedon Mar 6, 2019
Yeah once we have 1 proper way we should also spend some time to document them for sure 👍
MorrisJobke commentedon Jul 7, 2020
As I'm currently going through this: We have really quite some ways of handling events:
OC_Hook::connectandOC_Hook::emitEmitterTraitwithlistenandemitto be added to objects like the user manager and listeners being registered on that objectEventDispatcherInterfacevia$server->getEventDispatcher()OCP\EventDispatcher\IEventDispatcher- since 17 the way to go - best withdispatchTypedas this only holds the event object and then dispatch based on that fully typed one (no more juggling with strings that need to match)@rullzer @nickvergessen @ChristophWurst Did I miss something here?
How to start this? Move one by one events over from one of the first three to the fourth one? Or move them step by step through the stages 1-4? Or should we remove first either all the emits or all the listeners and move them to the newest approach plus a compatibility layer (aka emitting or listening on the old way).
nickvergessen commentedon Jul 7, 2020
No stages, if we update something from 1-3 always directly to 4.
MorrisJobke commentedon Jul 7, 2020
Was also just some weird idea I want to throw in and directly discard again.
Some little history lessons:
Our IEventDispatcher:
Symfony EventDispatcher:
EmitterTrait:
OC_Hook:
We added one way every two years. So unfortunately we need to wait for next year for this overhaul. 😆
ChristophWurst commentedon Jul 7, 2020
The deprecation of the old mechanism is easy: #18349. The trickier aspect is the migration of old "events" to the new events system.
ChristophWurst commentedon Jul 7, 2020
We should possibly pro-actively add new events for the old hooks and events so apps can migrate easier. Because if you find out an event is not there with the new dispatcher in 1.5y but you still want to support an old release you're possibly out of luck.
MorrisJobke commentedon Jul 7, 2020
Okay - so the first step would be to come up with a long list of events and hooks and try to emit them at all currently available places together with the old approach (whichever of the 3 it is). Then once this is done we can move over the listeners in a second step. Once all is moved over the old system can be removed 3 years after the 18 release - in January 2023.
Does that sound like a plan?
MorrisJobke commentedon Jul 7, 2020
I just noticed that some events are way older than when we implemented our new event system. For example the
SabrePluginEvent. https://github.com/nextcloud/server/blob/41b5e5923a44965e435d35d01b5009abb2dd5c97/lib/public/SabrePluginEvent.php#L36-L35 I guess this is nothing we will handle in our event system, or does it?Same for the
ManagerEventwhich itself has parameters to get event names passed in.server/lib/public/App/ManagerEvent.php
Line 60 in cb05782
Most likely this was because those classes extended the symfony event and thus got automatically migrated: #17568
Should we generate new events for them? And should those events we put into their apps or into the server. For putting them in the public namespace of the server would speak that any app then can use them and rely on their availability. Because some are in apps: https://github.com/nextcloud/suspicious_login/blob/bd2d60c3520f5e7d6af384a98c4c4ecf43bc1ec9/lib/Event/PostLoginEvent.php#L30
ChristophWurst commentedon Jul 7, 2020
That was before I added the one to server. That one can die now, actually :)
MorrisJobke commentedon Jul 7, 2020
@ChristophWurst But I assume it right that the events we want to maintain should all be inside the
OCPnamespace, right? Any special events namespace (\OCP\Events\Files\FileChangedEvent) or each in their related namespace (\OCP\Files\FileChangedEvent)? Because some existing ones are already in our OCP namespace.nickvergessen commentedon Jul 7, 2020
I much more prefer feature namespaces over technical limitations. The lib/Controller/ folder always bothers me.
MorrisJobke commentedon Jul 7, 2020
This was more like a "where should we put it from a semantically standpoint". I'm fine with both, but we should put it in the server. So you prefer the latter one, right?
ChristophWurst commentedon Jul 7, 2020
OCP/SubSystem/Events/SomeEvent😉Migrate OC_Group post_removeFromGroup hook to actual event object
Migrate OC_Group post_removeFromGroup hook to actual event object
Migrate OC_Group post_removeFromGroup hook to actual event object