Page MenuHomePhabricator

Move Autopromotion logic to UserGroupManager service
Closed, ResolvedPublic

Description

Currently, the Autopromote class uses multiple globals:

$wgAutopromote
$wgAutopromoteOnce
$wgEmailAuthentication

as well as the PermissionManager service, and soon the HookContainer/HookRunner and UserGroupManager services

I propose that this be moved into a service with the globals and dependencies injected.

Within deployed code, autopromote is used in two places: the SpecialUserrights and User classes, both in core, so the old class can be immediately deprecated

Event Timeline

Restricted Application added a project: User-DannyS712. · View Herald TranscriptMay 13 2020, 7:52 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
DannyS712 moved this task from Unsorted to Next on the User-DannyS712 board.EditedMay 13 2020, 7:54 AM
DannyS712 added a subscriber: daniel.

@daniel @Pchelolo would Platform Engineering be willing to review this?

Hm... Given that the two public methods are getAutopromoteGroups and getAutopromoteOnceGroups do you think it would be ok to move the methods into UserGroupManager created in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/545690 and deprecate/remove the class entirely?

They seem like an OK fit there. With introduction of the UseGroupManager, $user->getGroups() and $user->getFormerGroups() will belong in the manager, so the new service will have to depend on the UserGroupManager anyway. And I do not really see a lot of value in having these functions in a separate place.

One more pro for moving these into UserGroupManager is that it depends on Autopromote. Thus if we split the two services, we'd inevitably have a cyclic dependency.

eprodromou changed the task status from Open to Stalled.Jun 2 2020, 8:29 PM
eprodromou triaged this task as Low priority.
DannyS712 added a comment.EditedJun 2 2020, 10:31 PM

One more pro for moving these into UserGroupManager is that it depends on Autopromote. Thus if we split the two services, we'd inevitably have a cyclic dependency.

Sure - will do once the UserGroupManager is merged and deployed (make sure it won't be reverted)

DannyS712 renamed this task from Move Autopromotion logic to a service to Move Autopromotion logic to UserGroupManager service.Jun 4 2020, 3:41 PM

Change 602426 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] Move autopromote groups logic into UserGroupManager

https://gerrit.wikimedia.org/r/602426

Change 602737 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] Move User::addAutopromoteOnceGroups to UserGroupManager

https://gerrit.wikimedia.org/r/602737

DannyS712 changed the task status from Stalled to Open.Jun 11 2020, 3:41 AM
DannyS712 moved this task from Next to Others on the User-DannyS712 board.

Change 602426 merged by jenkins-bot:
[mediawiki/core@master] Move autopromote groups logic into UserGroupManager

https://gerrit.wikimedia.org/r/602426

Pchelolo closed this task as Resolved.Jun 12 2020, 7:39 PM

Change 602737 merged by jenkins-bot:
[mediawiki/core@master] Move User::addAutopromoteOnceGroups to UserGroupManager

https://gerrit.wikimedia.org/r/602737