Page MenuHomePhabricator

The `UserrightsPage` class is not safe to extend
Open, MediumPublic

Description

UserrightsPage (SpecialUserrights.php) is not safe to extend, but it is extended by:

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

So, what do you recommend? Mark it as @stable to extend?

So, what do you recommend? Mark it as @stable to extend?

No, I recommend splitting out whatever functionality needs to be available to extensions to a separate class/trait/service

Change 642859 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Revert "Inject services into SpecialUserrights"

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

@daniel @Naike would you be willing to reassess your evaluation of this as a future initiative / something for the icebox. The above revert isn't the first time that a change to SpecialUserrights has broken things for CentralAuth

From the latest revert

the problem is that SpecialGlobalGroupMembership doesn't actually call parent::__construct() and so none of the fallbacks provided for in the constructor were used, resulting in

[X7xOV6wQBHcAAE5b7KoAAAAC] /wiki/Special:GlobalUserRights/DannyS712 Error from line 143 of /srv/mediawiki/php-master/includes/specials/SpecialUserrights.php: Call to a member function getCanonical() on null

Backtrace:

#0 /srv/mediawiki/php-master/includes/specialpage/SpecialPage.php(639): UserrightsPage->execute(string)
#1 /srv/mediawiki/php-master/includes/specialpage/SpecialPageFactory.php(1272): SpecialPage->run(string)
#2 /srv/mediawiki/php-master/includes/MediaWiki.php(310): MediaWiki\SpecialPage\SpecialPageFactory->executePath(Title, RequestContext)
#3 /srv/mediawiki/php-master/includes/MediaWiki.php(945): MediaWiki->performRequest()
#4 /srv/mediawiki/php-master/includes/MediaWiki.php(548): MediaWiki->main()
#5 /srv/mediawiki/php-master/index.php(53): MediaWiki->run()
#6 /srv/mediawiki/php-master/index.php(46): wfIndexMain()
#7 /srv/mediawiki/w/index.php(3): require(string)
#8 {main}

Change 642859 merged by jenkins-bot:
[mediawiki/core@master] Revert "Inject services into SpecialUserrights"

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

Change 643299 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/CentralAuth@master] Prepare special page constructor for service injection in core

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

Change 643302 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/GlobalUserrights@master] Prepare special page constructor for service injection in core

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

Change 643299 merged by jenkins-bot:
[mediawiki/extensions/CentralAuth@master] Prepare special page constructor for service injection in core

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

Change 643155 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/core@master] Reapply "Inject services into SpecialUserrights"

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

Change 643302 merged by jenkins-bot:
[mediawiki/extensions/GlobalUserrights@master] Prepare special page constructor for service injection in core

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

Change 643155 merged by jenkins-bot:
[mediawiki/core@master] Reapply "Inject services into SpecialUserrights"

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

Pchelolo subscribed.

UserrightPage special in core uses cross-wiki User access via a UserrightsProxy, which is ill-documented and ill-supported concept. With introduction of ActorStore, we are going to refactor UserrightPage.

In order to even approach doing that we need to make make SpecialGlobalGroupMembership not extend the UserrightsPage. As a first step in this, we need to just make SpecialGlobalGroupMembership extend SpecialPage and copy all of the methods inherited from UserrightsPage directly into it. This will be an intermediate step in this refactoring, as we standardize on cross-wiki user access, we will refactor CentralAuth to use it as well.

daniel lowered the priority of this task from High to Medium.Feb 10 2021, 12:15 PM

Change 738574 had a related patch set uploaded (by Zabe; author: Zabe):

[mediawiki/extensions/CentralAuth@master] Stop extending the UserrightsPage class

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

Do we need to fix GlobalUserrights too, not being deployed or (apparently) actively maintained?

Change 738574 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] Stop extending the UserrightsPage class

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

Do we need to fix GlobalUserrights too, not being deployed or (apparently) actively maintained?

To answer my own question: I was going to do that anyway, then noticed that the extension has a compatibility policy of "master" and supports MW >= 1.35. I honestly don't want to copy 900 lines of code from master and make sure that each of them would still work on 1.35, adding BC hacks if not. I think it's not unfair to let the maintainers do whatever they think is best, be it bumping the minimum supported version or adding the BC hacks.

Do we need to fix GlobalUserrights too, not being deployed or (apparently) actively maintained?

It's very much appreciated! (I'm not sure what kind of active maintaining it needs or what, if anything, is broken; but it's very much used at ShoutWiki and hence should be compatible with the latest LTS release...or if not, then that certainly is a problem.)

To answer my own question: I was going to do that anyway, then noticed that the extension has a compatibility policy of "master" and supports MW >= 1.35. I honestly don't want to copy 900 lines of code from master and make sure that each of them would still work on 1.35, adding BC hacks if not. I think it's not unfair to let the maintainers do whatever they think is best, be it bumping the minimum supported version or adding the BC hacks.

(Didn't see this comment of yours before posting mine, so apologies for the noise)
I think bumping minimum required version of MW is fine as long as there's something 1.35-compatible; for the things I maintain (incl. the social tools family of extensions) I aim for "master version of the extension/skin is compatible with LTS (previously latest stable) MW" because keeping up with the branched versions and whatnot is hard and generally speaking not worth it (for me; YMMV, obviously!).

But at this point I'm fine with whatever approach, as long as 1) we can have a version of GlobalUserrights that works w/ MW 1.35 and 2) ideally also have a version that won't explode when we upgrade from 1.35 to the next LTS release. ;)
It's a rather old and feature-complete extension (well, save for one silly hook, implemented on ShoutWiki as a one-liner core hack to the Special:Statistics code with the main code living in GlobalUserrights -- it's just responsible for displaying accurate stats on that special page for global groups like staff etc.) so I don't forsee any active feature development for it, just minimal maintenance work to ensure it keeps functioning as intended despite core MW changes.

@ashley Thanks, this helps. My suggestion would be to bump the requirement to MW 1.38+ and just copy the missing pieces from UserrightsPage (similar to r738574). The REL1_37 version of the extension would still be compatible with MW 1.35. Otherwise you can check that the copied code works across all supported versions, but that's harder.