Migrate more apps to IEventDispatcher #39222
Conversation
nickvergessen
added
bug
3. to review
nickvergessen
requested review from
ArtificialOwl and
icewind1991
and removed request for
a team
![github-advanced-security[bot]](https://github.com/avatars/in/57789?s=60&v=4){kind=small}
| /** @var IUser $user */ | ||
| $user = $event->getSubject(); | ||
| $dispatcher->addListener('OC\AccountManager::userUpdated', function ($event) use ($appContainer) { | ||
| if ($event instanceof GenericEvent) { |
Check notice
Code scanning / Psalm
DocblockTypeContradiction Note
nickvergessen
removed
the
pending documentation
| @@ -694,7 +687,6 @@ public function formatOperation(array $operation): array { | |||
| */ | |||
| public function getEntitiesList(): array { | |||
| $this->dispatcher->dispatchTyped(new RegisterEntitiesEvent($this)); | |||
| $this->legacyEventDispatcher->dispatch(IManager::EVENT_NAME_REG_ENTITY, new GenericEvent($this)); | |||
There was a problem hiding this comment.
Code in applications which are registering a listener for this, will they get an error or just silently won’t be called anymore?
There was a problem hiding this comment.
"Silently" won't be called anymore after 3+ years of deprecation. It is documented in the changelogs never the less:
https://docs.nextcloud.com/server/latest/developer_manual/app_publishing_maintenance/app_upgrade_guide/upgrade_to_28.html#removed-events
| $dispatcher->addListener('OC\AccountManager::userUpdated', function (GenericEvent $event) use ($appContainer) { | ||
| /** @var IUser $user */ | ||
| $user = $event->getSubject(); | ||
| $dispatcher->addListener('OC\AccountManager::userUpdated', function ($event) use ($appContainer) { |
There was a problem hiding this comment.
There is no typed event version for this one?
This change is not really updating to the new system, it’s still using the old one, no?
There was a problem hiding this comment.
There is no typed event version for this one?
No unluckily not.
This change is not really updating to the new system, it’s still using the old one, no?
For "listeners" there is no new/old system. It's the same. But yeah I'm not planing to migrate the things to typed events with the current work. I just want to free the way to be able to merge #38546 and that comes with a breaking API change, unless our wrapper is used when dispatching.
But for the ease of search and being sure, I'm move all code away from directly using the thirdparty.
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
nickvergessen
force-pushed
the
bugfix/noid/more-event-dispatchers
branch
from
6371b22
to
aa039c9
Compare
|
Rebased after conflicts |
One step closer for #38546
Checklist