Page MenuHomePhabricator

Use ExtensionRegistry instead of class_exists to check for enabled extensions
Closed, ResolvedPublic

Description

Several Wikibase codebases use class_exists checks in tests as well as productive code to check if a specific extension is enabled:

if ( !class_exists( 'CheckUser' ) ) {

This binds against a full qualified class name in a super-problematic way that does not fail when the class is renamed or moved to an other namespace. This already caused many regressions in other (related and unrelated) codebases.

All these conditionals must be replaced with a proper check :

if ( !ExtensionRegistry::getInstance()->isLoaded( 'CheckUser' ) ) {

See https://gerrit.wikimedia.org/r/398511 for an example.

ExtensionRegistry can only used, when the extension to check is using extension.json

Event Timeline

Restricted Application added a project: TCB-Team. · View Herald TranscriptDec 17 2017, 5:32 PM
Restricted Application added subscribers: TerraCodes, Aklapper. · View Herald Transcript
thiemowmde triaged this task as Normal priority.Dec 17 2017, 5:33 PM
thiemowmde moved this task from incoming to ready to go on the Wikidata board.
Ebe123 added a subscriber: Ebe123.

This is an issue that is found also with MediaWiki-extensions-Score in respect to TimedMediaHandler. In this area, the task attached is to formalize how TMH handles other extensions with providing players: T135501: Formalize how TMH provides a player for Score generated ogg/vorbis files.

Feel free to pick this up too, but please create a separate ticket. I'm going to close this when all code bases relevant for Wikidata have this resolved.

I did a quick search in my dev code base (which contains many more extensions) and found the following class names appear more than once in class_exists checks. Many of them are very obviously not checks for classes, but for enabled extensions.

  • EchoEvent (18)
  • BetaFeatures (15)
  • CirrusSearch (15)
  • CentralAuthUser (7)
  • CentralAuthSpoofUser (6)
  • EventLogging (6)
  • LqtDispatch (6)
  • GeoData (5)
  • ResourceLoaderSchemaModule (4)
  • SiteMatrix (4)
  • AbuseFilter (3)
  • AntiSpoofAuthenticationRequest (3)
  • CommentStore (3)
  • GuidedTourHooks (3)
  • RenameuserSQL (3)
  • ThanksHooks (3)
  • WikiEditorHooks (3)
  • ApiUsageException (2)
  • AuthManager (2)
  • Babel (2)
  • EchoHooks (2)
  • GlobalBlocking (2)
  • Imagick (2)
  • MobileContext (2)
  • PageImages (2)
  • PageProps (2)
  • PHPUnit_Framework_TestCase (2)
  • Scribunto_LuaEngineTestBase (2)
  • tidy (2)
  • TitleBlacklist (2)
  • VisualEditorHooks (2)
  • WikibaseClient (2)
  • XMLReader (2)

Change 398794 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/CentralAuth@master] Replace class_exists with proper extension registration checks

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

Change 398796 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/AdvancedSearch@master] Replace class_exists with proper extension registration checks

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

Change 398797 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Wikibase@master] Replace class_exists with proper extension registration checks

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

Change 398796 merged by jenkins-bot:
[mediawiki/extensions/AdvancedSearch@master] Replace class_exists with proper extension registration checks

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

Change 398820 had a related patch set uploaded (by WMDE-Fisch; owner: WMDE-Fisch):
[mediawiki/extensions/TwoColConflict@master] Replace class_exists with proper extension registration checks

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

Change 398831 had a related patch set uploaded (by Ladsgroup; owner: Amir Sarabadani):
[mediawiki/extensions/ORES@master] Use ExtensionRegistry to check if BetaFeatures is loaded

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

Change 398831 merged by jenkins-bot:
[mediawiki/extensions/ORES@master] Use ExtensionRegistry to check if BetaFeatures is loaded

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

Change 398820 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Replace class_exists with proper extension registration checks

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

Change 398797 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Replace class_exists with proper extension registration checks

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

Change 398794 merged by jenkins-bot:
[mediawiki/extensions/CentralAuth@master] Replace class_exists with proper extension registration checks

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

Ladsgroup updated the task description. (Show Details)Dec 27 2017, 11:43 AM

Change 400395 had a related patch set uploaded (by Ladsgroup; owner: Amir Sarabadani):
[mediawiki/extensions/Echo@master] Use ExtensionRegistry instead of class_exists

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

thiemowmde updated the task description. (Show Details)Dec 27 2017, 12:30 PM

Change 400398 had a related patch set uploaded (by Ladsgroup; owner: Amir Sarabadani):
[mediawiki/extensions/MobileFrontend@master] Use ExtensionRegistry instead of class_exists

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

@Ladsgroup, the numbers in my comment are from my local codebase and not something others can work on.

Note that the only problematic class_exists checks are the ones that don't utilize the …::class feature. class_exists( Babel::class ) is fine, but class_exists( '\Babel' ) is not. Only the later should be cleaned up systematically. In many cases it's just fine to do a class_exists check, especially if the code below actually uses the very same class. For example, class_exists( WikibaseRepo::class ) should (in my opinion) not be replaced with an extension registration check, because that would introduce the problematic string "Wikibase Repository" I don't want to hard-code in to many places.

Change 400463 had a related patch set uploaded (by Ebe123; owner: Ebe123):
[mediawiki/extensions/Score@master] Replace class_exists with ExtensionRegistry checks

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

Change 400463 merged by jenkins-bot:
[mediawiki/extensions/Score@master] Replace class_exists with ExtensionRegistry checks

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

Change 400576 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/CirrusSearch@master] Use ExtensionRegistry instead of class_exists

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

Change 400576 merged by jenkins-bot:
[mediawiki/extensions/CirrusSearch@master] Use ExtensionRegistry instead of class_exists

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

Change 400395 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Use ExtensionRegistry instead of class_exists

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

Change 400398 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Use ExtensionRegistry instead of class_exists

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

Zoranzoki21 moved this task from Backlog to Doing on the good first bug board.Jan 31 2018, 3:56 PM

Change 407873 had a related patch set uploaded (by Matěj Suchánek; owner: Huji):
[mediawiki/extensions/AbuseFilter@master] Use ExtensionRegistry to check if CheckUser is installed

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

Change 407873 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Use ExtensionRegistry to check if CheckUser is installed

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

thiemowmde moved this task from Incoming to Advanced Search on the TCB-Team board.Jun 12 2018, 4:06 PM

Change 444250 had a related patch set uploaded (by Matěj Suchánek; owner: Matěj Suchánek):
[mediawiki/extensions/Thanks@master] Use ExtensionRegistry instead of class_exists

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

Addshore added a comment.EditedAug 18 2018, 8:53 PM

Is there an up to date list of what needs to be done here?
This task is pretty hard to follow as it is! :)

According to @thiemowmde in T183096#3843756 this was going to be closed once the Wikibase code bases were dealt with but since then this task seems to have expanded and I see lots of other projects also tagged now.

The Wikibase codebase currently contains about 40 class_exists checks. The vast majority checks for CirrusSearch – I guess because that extension still doesn't have an extension.json.

Just making people aware of the issue is not very successful, as the most problematic class_exists( '…' ) checks with strings instead of the ::class feature get introduced repeatedly, some just a few days ago (https://gerrit.wikimedia.org/r/449209). I wish there would be a way to automatically forbid problematic class_exists, while still allowing non-problematic ones, but I can't think of a way.

I suggest to close this task as "done" as the bigger part of the issue (references to classes hidden in strings) got resolved for the most part.

Change 453760 had a related patch set uploaded (by TheDJ; owner: TheDJ):
[mediawiki/extensions/WikiEditor@master] Use ExtensionRegistry instead of class_exists

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

Change 453761 had a related patch set uploaded (by TheDJ; owner: TheDJ):
[mediawiki/extensions/TimedMediaHandler@master] Replace class_exists with proper extension registration checks

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

TheDJ added a subscriber: TheDJ.Aug 19 2018, 3:22 PM

whoops, copied a little bit too much there. Those weren't meant to attach to this ticket.

Change 453761 merged by jenkins-bot:
[mediawiki/extensions/TimedMediaHandler@master] Replace class_exists with proper extension registration checks

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

Change 453760 merged by jenkins-bot:
[mediawiki/extensions/WikiEditor@master] Use ExtensionRegistry instead of class_exists

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

Addshore added a comment.EditedAug 20 2018, 7:44 AM

I wish there would be a way to automatically forbid problematic class_exists, while still allowing non-problematic ones, but I can't think of a way.

That sounds like it should be possible in code sniffer.

I suggest to close this task as "done" as the bigger part of the issue (references to classes hidden in strings) got resolved for the most part.

Can close it as done, and file follow ups for the CirrusSearch cases.

Change 453939 had a related patch set uploaded (by Addshore; owner: Addshore):
[mediawiki/extensions/Wikibase@master] use ::class for class_exists checks instead of string

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

Change 453941 had a related patch set uploaded (by Addshore; owner: Addshore):
[mediawiki/extensions/Wikibase@master] Use ExtensionRegistry check for Echo in tests instead of class_exists

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

Sorry, I confused two ideas:

  1. Yes, we certainly can create a sniff that forbids class_exists( 'string' ).
  2. To make a sniff understand if a class_exists can be replaced with an ExtensionRegistry check, the sniff would need to know which class belongs to which extension, and if the extension even can be used via ExtensionRegistry.

Patches exist for any other occurrences that I spotted.
I'll leave this open until they are reviewed and land in Wikibase.

I have created T87892 for CirrusSearch.

Restricted Application added a project: User-Addshore. · View Herald TranscriptAug 20 2018, 8:02 AM
Addshore moved this task from Unsorted 💣 to Active 🚁 on the User-Addshore board.

Change 453939 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] use ::class for class_exists checks instead of string

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

Change 453941 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Use ExtensionRegistry check for Echo in tests instead of class_exists

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

Addshore closed this task as Resolved.Aug 20 2018, 3:17 PM

T87892 remains open for for CirrusSearch.

Change 444250 merged by jenkins-bot:
[mediawiki/extensions/Thanks@master] Use ExtensionRegistry instead of class_exists

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