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(dispatcher): Move remaining simple cases in apps/ folder to IEven… #39485

Merged
merged 1 commit into from
Jul 26, 2023
Merged

fix(dispatcher): Move remaining simple cases in apps/ folder to IEven… #39485

merged 1 commit into from
Jul 26, 2023

Conversation

nickvergessen
Copy link
Member

…tDispatcher

Checklist

@nickvergessen nickvergessen added 3. to review Waiting for reviews technical debt pending documentation This pull request needs an associated documentation update labels Jul 19, 2023
@nickvergessen nickvergessen added this to the Nextcloud 28 milestone Jul 19, 2023
@nickvergessen nickvergessen self-assigned this Jul 19, 2023
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 19, 2023
View reviewed changes
apps/files_sharing/lib/AppInfo/Application.php
@@ -148,12 +143,18 @@
});

// 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 Note

OCP\EventDispatcher\Event does not contain Symfony\Component\EventDispatcher\GenericEvent
apps/files_sharing/lib/AppInfo/Application.php
/** @var Listener $listener */
$listener = $this->getContainer()->query(Listener::class);
$listener->shareNotification($event);
});
$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 Note

OCP\EventDispatcher\Event does not contain Symfony\Component\EventDispatcher\GenericEvent
apps/user_ldap/lib/AppInfo/Application.php
@@ -136,7 +134,7 @@
$groupManager->addBackend($groupBackend);

$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 Note

The method OCP\EventDispatcher\IEventDispatcher::dispatch has been marked as deprecated
apps/user_ldap/lib/AppInfo/Application.php
@@ -153,7 +151,7 @@
);
}

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

Check notice

Code scanning / Psalm

MissingReturnType Note

Method OCA\User_LDAP\AppInfo\Application::registerBackendDependents does not have a return type, expecting void
apps/user_ldap/lib/AppInfo/Application.php
@@ -153,7 +151,7 @@
);
}

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

Check notice

Code scanning / Psalm

DeprecatedInterface Note

Interface OCP\AppFramework\IAppContainer is marked as deprecated
@nickvergessen nickvergessen force-pushed the bugfix/noid/move-remaining-easy-usages-to-IEventDispatcher branch from 08ce53a to f733301 Compare July 19, 2023 20:07
@nickvergessen nickvergessen force-pushed the bugfix/noid/move-remaining-easy-usages-to-IEventDispatcher branch from f733301 to 77bc6c3 Compare July 25, 2023 09:19
@nickvergessen nickvergessen mentioned this pull request Jul 25, 2023
Merged
4 tasks
@nickvergessen nickvergessen requested review from ChristophWurst, a team, ArtificialOwl and Fenn-CS and removed request for a team July 25, 2023 20:20
marcelklehr
marcelklehr reviewed Jul 26, 2023
View reviewed changes
apps/files_sharing/lib/AppInfo/Application.php
$dispatcher->addListener('OCP\Share::postShare', function ($event) {
if (!$event instanceof OldGenericEvent) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Why is this guard needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

It helps to trick psalm, so it doesn't yield on $listener->shareNotification($event) Later on, which only eats GenericEvent.
But it only is an intermediate step anyway to move forward to 5.4.x of the dispatcher. When we have to update to 6.4.x at the end of the year, GenericEvent will no longer exist and we need to do more work I guess, so I took the soft/low effort approach and added this mini guard.

susnux
susnux approved these changes Jul 26, 2023
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

🧹

@nickvergessen nickvergessen merged commit b76b0bb into master Jul 26, 2023
45 of 46 checks passed
@nickvergessen nickvergessen deleted the bugfix/noid/move-remaining-easy-usages-to-IEventDispatcher branch July 26, 2023 15:29
This was referenced Jul 27, 2023
Merged
Merged
@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

@marcelklehr marcelklehr marcelklehr approved these changes

@susnux susnux susnux approved these changes

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

@kesselb kesselb Awaiting requested review from kesselb

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl was automatically assigned from nextcloud/server-backend

@Fenn-CS Fenn-CS Awaiting requested review from Fenn-CS Fenn-CS was automatically assigned from nextcloud/server-backend

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.
4 participants
@nickvergessen @marcelklehr @susnux @come-nc