Skip to content

Move lib/ events to IEventDispatcher where possible #39571

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
Merged
merged 8 commits into from
Jul 27, 2023
Merged

Move lib/ events to IEventDispatcher where possible #39571

merged 8 commits into from
Jul 27, 2023

Conversation

@nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Jul 25, 2023 •
edited
Loading

Checklist

@nickvergessen nickvergessen added this to the Nextcloud 28 milestone Jul 25, 2023
@nickvergessen nickvergessen requested review from come-nc and kesselb July 25, 2023 17:40
@nickvergessen nickvergessen self-assigned this Jul 25, 2023
@nickvergessen nickvergessen requested a review from st3iny as a code owner July 25, 2023 17:40
@nickvergessen nickvergessen added the pending documentation This pull request needs an associated documentation update label Jul 25, 2023
@nickvergessen nickvergessen force-pushed the bugfix/noid/move-lib-to-IEventDispatcher branch from 457f1fd to 8fba3b4 Compare July 25, 2023 17:41
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 25, 2023
View reviewed changes
apps/files_sharing/lib/AppInfo/Application.php
// notifications api to accept incoming user shares
$oldDispatcher->addListener('OCP\Share::postShare', function (OldGenericEvent $event) {
$dispatcher->addListener('OCP\Share::postShare', function ($event) {
if (!$event instanceof OldGenericEvent) {

Check notice

Code scanning / Psalm

RedundantConditionGivenDocblockType

OCP\EventDispatcher\Event does not contain Symfony\Component\EventDispatcher\GenericEvent
apps/files_sharing/lib/AppInfo/Application.php
});
$oldDispatcher->addListener(IGroup::class . '::postAddUser', function (OldGenericEvent $event) {
$dispatcher->addListener(IGroup::class . '::postAddUser', function ($event) {
if (!$event instanceof OldGenericEvent) {

Check notice

Code scanning / Psalm

RedundantConditionGivenDocblockType

OCP\EventDispatcher\Event does not contain Symfony\Component\EventDispatcher\GenericEvent
apps/user_ldap/lib/AppInfo/Application.php

$userBackendRegisteredEvent = new UserBackendRegistered($userBackend, $userPluginManager);
$legacyDispatcher->dispatch('OCA\\User_LDAP\\User\\User::postLDAPBackendAdded', $userBackendRegisteredEvent);
$dispatcher->dispatch('OCA\\User_LDAP\\User\\User::postLDAPBackendAdded', $userBackendRegisteredEvent);

Check notice

Code scanning / Psalm

DeprecatedMethod

The method OCP\EventDispatcher\IEventDispatcher::dispatch has been marked as deprecated
apps/user_ldap/lib/AppInfo/Application.php
}

private function registerBackendDependents(IAppContainer $appContainer, EventDispatcherInterface $dispatcher) {
private function registerBackendDependents(IAppContainer $appContainer, IEventDispatcher $dispatcher) {

Check notice

Code scanning / Psalm

MissingReturnType

Method OCA\User_LDAP\AppInfo\Application::registerBackendDependents does not have a return type, expecting void
apps/user_ldap/lib/AppInfo/Application.php
}

private function registerBackendDependents(IAppContainer $appContainer, EventDispatcherInterface $dispatcher) {
private function registerBackendDependents(IAppContainer $appContainer, IEventDispatcher $dispatcher) {

Check notice

Code scanning / Psalm

DeprecatedInterface

Interface OCP\AppFramework\IAppContainer is marked as deprecated
@nickvergessen nickvergessen force-pushed the bugfix/noid/move-lib-to-IEventDispatcher branch from 8fba3b4 to cd75dc0 Compare July 26, 2023 06:38
@st3iny
Copy link
Member

st3iny commented Jul 26, 2023

Code is too deep for me. PHP CS check is unhappy though.

@st3iny st3iny removed their request for review July 26, 2023 09:11
@nickvergessen nickvergessen force-pushed the bugfix/noid/move-lib-to-IEventDispatcher branch 3 times, most recently from 1193b43 to 978cef9 Compare July 26, 2023 20:40
come-nc
come-nc reviewed Jul 27, 2023
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Did you keep track of all removed legacy events to fill the upgrade documentation accordingly?

@nickvergessen
fix!: Remove legacy event dispatching Symfony's GenericEvent
4f7de8e 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
fix: Move AppManager events to IEventDispatcher
5bb6a78 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
fix!: Remove legacy event dispatching Symfony's GenericEvent from Add…
1b387bb 
…itionalScripts

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
fix!: Remove legacy event dispatching Symfony's GenericEvent from 2FA…
dac31ad 
… Manager

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
fix: Move SystemTags events to IEventDispatcher
6953e14 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
fix: Move Node HookConnecter events to IEventDispatcher
44a0a62 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
fix: Move Sharing events to IEventDispatcher
c67c067 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
fix!: Remove User events which have a typed event replacement
35c313e 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the bugfix/noid/move-lib-to-IEventDispatcher branch from 978cef9 to 35c313e Compare July 27, 2023 07:58
@nickvergessen
Copy link
Member Author

nickvergessen commented Jul 27, 2023 •
edited
Loading

Did you keep track of all removed legacy events to fill the upgrade documentation accordingly?

Yes, will send a PR once the last remaining events are migrated. Todo list is:
https://github.com/nextcloud/server/issues?q=label%3A%22pending+documentation%22+author%3Anickvergessen+is%3Aclosed 😎

And can also be rechecked with the links on #38546 (comment) afterwards

@nickvergessen nickvergessen requested a review from come-nc July 27, 2023 08:03
@nickvergessen nickvergessen enabled auto-merge July 27, 2023 16:58
@nickvergessen nickvergessen merged commit 6b72217 into master Jul 27, 2023
@nickvergessen nickvergessen deleted the bugfix/noid/move-lib-to-IEventDispatcher branch July 27, 2023 16:59
@nickvergessen nickvergessen mentioned this pull request Jul 28, 2023
Merged
15 tasks
@nickvergessen nickvergessen removed the pending documentation This pull request needs an associated documentation update label Jul 28, 2023
@nickvergessen
Copy link
Member Author

Docs in nextcloud/documentation#10858

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

Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@come-nc come-nc come-nc approved these changes

@kesselb kesselb Awaiting requested review from kesselb

@miaulalala miaulalala Awaiting requested review from miaulalala miaulalala is a code owner

Assignees

@nickvergessen nickvergessen

Labels

3. to review Waiting for reviews technical debt

Projects

None yet

Milestone

Nextcloud 28

Development

Successfully merging this pull request may close these issues.

5 participants

@nickvergessen @st3iny @ChristophWurst @come-nc