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.

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

Use IEventDispatcher in GroupManager.php #853

Closed
bdovaz opened this issue May 21, 2024 · 2 comments · Fixed by #856
Closed

Use IEventDispatcher in GroupManager.php #853

bdovaz opened this issue May 21, 2024 · 2 comments · Fixed by #856

Comments

@bdovaz
Copy link
Contributor

bdovaz commented May 21, 2024

GroupManager currently relies on a private and deprecated API:

$this->groupManager->emit('\OC\Group', 'preDelete', [$group]);

We should use the IEventDispatcher pattern as Nextcloud does in its IGroupManager implementation:

https://github.com/nextcloud/server/blob/5a6e48e85034aa81862b1fcbd93cb4ce58165e72/lib/private/Group/Manager.php#L287

Events:

https://github.com/nextcloud/server/tree/5a6e48e85034aa81862b1fcbd93cb4ce58165e72/lib/public/Group/Events

This interface exists since version 17.

If we don't want to be a breaking change, as seen in the example above, it sends it both ways (with PublicEmitter and IEventDispatcher).

@blizzz would you accept a PR?

@blizzz
Copy link
Member

blizzz commented May 31, 2024

GroupManager currently relies on a private and deprecated API:

$this->groupManager->emit('\OC\Group', 'preDelete', [$group]);

We should use the IEventDispatcher pattern as Nextcloud does in its IGroupManager implementation:

https://github.com/nextcloud/server/blob/5a6e48e85034aa81862b1fcbd93cb4ce58165e72/lib/private/Group/Manager.php#L287

Events:

https://github.com/nextcloud/server/tree/5a6e48e85034aa81862b1fcbd93cb4ce58165e72/lib/public/Group/Events

This interface exists since version 17.

If we don't want to be a breaking change, as seen in the example above, it sends it both ways (with PublicEmitter and IEventDispatcher).

@blizzz would you accept a PR?

Hey @bdovaz and thanks for the investigation.

It might be that those old events are listened to in server and thrown again properly – but it does not seem to be the case here. So yes, I would gladly accept a PR.

We can have both methods in 6.2.x and drop the legacy once bumping to 6.3 or 7. I do not see usage of the old event atm, and as you say the interface exists for ages, so it'd consider it okay for a minor bump. We can keep an issue open for that case.

@blizzz
Copy link
Member

blizzz commented May 31, 2024

On a second though, #690 qualifies for a minor bump, but so replacing would be just fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants