Repository navigation
Move hooks and emitters all to the eventdispatcher #14552
Description
Activity
Yes
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?
👍 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.
Reacted by Joas SchillingYeah once we have 1 proper way we should also spend some time to document them for sure 👍
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 object- Symfonys
EventDispatcherInterfacevia$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).
No stages, if we update something from 1-3 always directly to 4.
Reacted by Morris Jobke and Christoph WurstNo stages, if we update something from 1-3 always directly to 4.
Was also just some weird idea I want to throw in and directly discard again.
Some little history lessons:
Our IEventDispatcher:
- added in May 2019 to 17.0.0
- Add event dispatcher to OCP #15777
Symfony EventDispatcher:
- added in August 2015 to 8.2.0
- Event dispatcher owncloud/core#18316
EmitterTrait:
- added in May 2013 to 6.0.0
- New hook system owncloud/core#3266
OC_Hook:
- added in July 2011 to 2.0.0
- owncloud/core@f5c0dbd
We added one way every two years. So unfortunately we need to wait for next year for this overhaul. 😆
The deprecation of the old mechanism is easy: #18349. The trickier aspect is the migration of old "events" to the new events system.
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.
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.
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?
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.Something that should be handled as separate events.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
Because some are in apps: https://github.com/nextcloud/suspicious_login/blob/bd2d60c3520f5e7d6af384a98c4c4ecf43bc1ec9/lib/Event/PostLoginEvent.php#L30
That was before I added the one to server. That one can die now, actually :)
@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.I much more prefer feature namespaces over technical limitations. The lib/Controller/ folder always bothers me.
I much more prefer feature namespaces over technical limitations. The lib/Controller/ folder always bothers me.
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?
Any special events namespace
OCP/SubSystem/Events/SomeEvent😉- added a commit that references this issue
on Jul 7, 2020
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: