search

nextcloud / user_saml

:lock: App for authenticating Nextcloud users using SAML https://apps.nextcloud.com/apps/user_saml
https://portal.nextcloud.com/article/configuring-single-sign-on-10.html
GNU Affero General Public License v3.0
95 stars 75 forks source link

Use IEventDispatcher in GroupManager.php #853

Closed bdovaz closed 4 months ago

bdovaz commented 5 months ago

GroupManager currently relies on a private and deprecated API:

https://github.com/nextcloud/user_saml/blob/af8911cc72f4843dbcd2c906db04edb9dd3c98d0/lib/GroupManager.php#L145

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 commented 5 months ago

GroupManager currently relies on a private and deprecated API:

https://github.com/nextcloud/user_saml/blob/af8911cc72f4843dbcd2c906db04edb9dd3c98d0/lib/GroupManager.php#L145

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 commented 5 months ago

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