Page MenuHomePhabricator
Create Task
Maniphest T183096

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

Assigned To
Addshore
Authored By
thiemowmde
Dec 17 2017, 5:32 PM
Tags
Referenced Files
None
Subscribers
Addshore
Aklapper
aude
Ebe123
gerritbot
hoo
Ladsgroup
View All 11 Subscribers

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

Details

Show related patches Customize query in gerrit

Revisions and Commits

rESCR extension-Score
rESCR4e031e3d2f61 Replace class_exists with ExtensionRegistry checks

Related Objects

Event Timeline

thiemowmde created this task.Dec 17 2017, 5:32 PM
Restricted Application added a project: TCB-Team (now WMDE-TechWish). · View Herald TranscriptDec 17 2017, 5:32 PM
Restricted Application added subscribers: TerraCodes, Aklapper. · View Herald Transcript
thiemowmde triaged this task as Medium priority.Dec 17 2017, 5:33 PM
thiemowmde moved this task from incoming to ready to go on the Wikidata board.
Ebe123 added a project: MediaWiki-extensions-Score.Dec 17 2017, 9:23 PM
Ebe123 subscribed.

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.

Ebe123 moved this task from Backlog to Audio on the MediaWiki-extensions-Score board.Dec 17 2017, 9:23 PM
Ebe123 removed a project: MediaWiki-extensions-Score.
thiemowmde added a comment.Dec 18 2017, 7:44 AM

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)
gerritbot subscribed.Dec 18 2017, 8:40 AM

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

gerritbot added a project: Patch-For-Review.Dec 18 2017, 8:40 AM

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

thiemowmde mentioned this in rEASR554e3943cdd6: Replace class_exists with proper extension registration checks.Dec 18 2017, 8:41 AM
gerritbot added a comment.Dec 18 2017, 8:42 AM

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

WMDE-Fisch mentioned this in rEASR83dbac6d7b3c: Replace class_exists with proper extension registration checks.Dec 18 2017, 11:58 AM
WMDE-Fisch mentioned this in rEASR6a0c3ee2eb67: Replace class_exists with proper extension registration checks.Dec 18 2017, 12:03 PM
gerritbot added a comment.Dec 18 2017, 12:04 PM

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

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

gerritbot added a comment.Dec 18 2017, 12:10 PM

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

WMDE-Fisch mentioned this in rESCC633b44ac92f2: Replace class_exists with proper extension registration checks.Dec 18 2017, 12:11 PM
ReleaseTaggerBot added a project: MW-1.31-release-notes (WMF-deploy-2018-01-02 (1.31.0-wmf.15)).Dec 18 2017, 1:00 PM
gerritbot added a comment.Dec 18 2017, 1:05 PM

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

gerritbot added a comment.Dec 18 2017, 4:26 PM

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

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

gerritbot added a comment.Dec 18 2017, 7:04 PM

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

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

gerritbot added a comment.Dec 18 2017, 11:38 PM

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

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

gerritbot added a comment.Dec 19 2017, 8:55 PM

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
gerritbot added a comment.Dec 27 2017, 12:14 PM

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

Ladsgroup added a comment.Dec 27 2017, 12:22 PM

http://codesearch.wmflabs.org/extensions/?q=class_exists&i=nope&files=&repos= says 381 cases which most of them are just to check extensions.

thiemowmde updated the task description. (Show Details)Dec 27 2017, 12:30 PM
gerritbot added a comment.Dec 27 2017, 12:32 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

thiemowmde added a comment.Dec 27 2017, 12:34 PM

@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.

gerritbot added a comment.Dec 28 2017, 4:11 AM

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

Ebe123 added a commit: rESCR4e031e3d2f61: Replace class_exists with ExtensionRegistry checks.Dec 28 2017, 4:12 AM
gerritbot added a comment.Dec 28 2017, 8:55 AM

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

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

gerritbot added a comment.Dec 28 2017, 11:58 AM

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

Umherirrender updated the task description. (Show Details)Dec 28 2017, 9:51 PM
gerritbot added a comment.Dec 28 2017, 10:49 PM

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

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

gerritbot added a comment.Dec 28 2017, 11:48 PM

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

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

gerritbot added a comment.Dec 28 2017, 11:50 PM

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

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

Lea_WMDE moved this task from Backlog to Doing (others) on the Advanced-Search board.Jan 10 2018, 8:44 AM
Kizule moved this task from Backlog to Doing on the good first task board.Jan 31 2018, 3:56 PM
gerritbot added a comment.Feb 3 2018, 2:02 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

gerritbot added a comment.Feb 3 2018, 2:17 PM

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

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

ReleaseTaggerBot edited projects, added MW-1.31-release-notes (WMF-deploy-2018-02-06 (1.31.0-wmf.20)); removed MW-1.31-release-notes (WMF-deploy-2018-01-02 (1.31.0-wmf.15)).Feb 3 2018, 3:00 PM
matej_suchanek removed a project: Patch-For-Review.Feb 5 2018, 12:43 PM
Umherirrender mentioned this in T188144: Add sniff to detect class_exists( 'string' ).Feb 24 2018, 11:30 AM
thiemowmde moved this task from Incoming to Advanced Search on the TCB-Team (now WMDE-TechWish) board.Jun 12 2018, 4:06 PM
gerritbot added a comment.Jul 6 2018, 4:10 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

gerritbot added a project: Patch-For-Review.Jul 6 2018, 4:10 PM
Addshore moved this task from Unsorted to Migrate / Replace on the Technical-Debt board.Jul 12 2018, 3:59 PM
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.

Addshore added a project: [DEPRECATED] wdwb-tech.Aug 18 2018, 8:55 PM
Addshore moved this task from Inbox to Investigate & Discuss on the [DEPRECATED] wdwb-tech board.
thiemowmde added a comment.Aug 19 2018, 1:58 PM

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.

gerritbot added a comment.Aug 19 2018, 3:17 PM

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

gerritbot added a comment.Aug 19 2018, 3:20 PM

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 subscribed.Aug 19 2018, 3:22 PM

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

gerritbot added a comment.Aug 19 2018, 4:53 PM

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

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

gerritbot added a comment.Aug 19 2018, 4:53 PM

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

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

ReleaseTaggerBot added a project: MW-1.32-notes (WMF-deploy-2018-08-21 (1.32.0-wmf.18)).Aug 19 2018, 5:00 PM
Addshore added a comment.EditedAug 20 2018, 7:44 AM
In T183096#4513119, @thiemowmde wrote:

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.

gerritbot added a comment.Aug 20 2018, 7:47 AM

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

gerritbot added a comment.Aug 20 2018, 7:53 AM

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

thiemowmde added a comment.Aug 20 2018, 7:57 AM

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.
Addshore mentioned this in T202254: Use ExtensionRegistry instead of class_exists to check for CirrusSearch in Wikibase.Aug 20 2018, 8:01 AM

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.

Addshore claimed this task.Aug 20 2018, 8:02 AM
Restricted Application added a project: User-Addshore. · View Herald TranscriptAug 20 2018, 8:02 AM
Addshore moved this task from ready to go to in progress on the Wikidata board.Aug 20 2018, 8:02 AM
Addshore moved this task from Unsorted 💣 to Active 🚁 on the User-Addshore board.
gerritbot added a comment.Aug 20 2018, 10:07 AM

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

gerritbot added a comment.Aug 20 2018, 2:48 PM

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.

Addshore moved this task from Active 🚁 to Closing ✔️ on the User-Addshore board.Aug 20 2018, 3:17 PM
gerritbot added a comment.Jan 2 2019, 5:56 PM

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

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

ReleaseTaggerBot added a project: MW-1.33-notes (1.33.0-wmf.12; 2019-01-08).Jan 2 2019, 6:01 PM
thiemowmde removed a project: TCB-Team (now WMDE-TechWish).Jan 7 2022, 2:48 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL