Page MenuHomePhabricator
Create Task
Maniphest T286209

Create a tests to prevent unexpected usage of ILoadBalancer and ILBFactory instances in Wikibase
Closed, ResolvedPublic
Actions

Description

This means we want something similar to the forbidden Client/Repo usage tests.

MediaWikiServices::getDBLoadBalancerFactory and MediaWikiServices::getDBLoadBalancer shouldn't be called in Wikibase production code outside of the three places where it is currently being called.

Details

SubjectRepoBranchLines +/-
Add ClientNoBadUsageTestmediawiki/extensions/Wikibasemaster+59 -0
Add RepoNoBadUsageTestmediawiki/extensions/Wikibasemaster+53 -0
Also strip line comments in NoBadDependencyUsageTestmediawiki/extensions/Wikibasemaster+2 -1
Rewrite NoBadDependencyUsageTest as NoBadUsageTestmediawiki/extensions/Wikibasemaster+259 -102
Test number of MW RDBMS references in lib/mediawiki/extensions/Wikibasemaster+6 -0
Customize query in gerrit

Related Objects
Search...

Event Timeline

Michael created this task.Jul 5 2021, 2:46 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 5 2021, 2:46 PM
Michael mentioned this in T283246: Use DomainDB abstraction in all places in Wikibase where DB connections are created or ILBFactory / ILoadBalancer are otherwise used.Jul 5 2021, 2:46 PM
Maintenance_bot moved this task from incoming to in progress on the Wikidata board.Jul 5 2021, 3:15 PM
Lucas_Werkmeister_WMDE subscribed.Jul 6 2021, 12:19 PM

So what exactly do we want to prevent…

  • The Wikimedia\Rdmbs\ namespace: this would be closest to the existing repo/client/lib usage test, but I’m pretty sure this isn’t feasible – there are several uses of other types in that namespace, notably IResultWrapper, that should be allowed.
  • The I?(LoadBalancer|LBFactory) types (with an exception to allow ILoadBalancer::CONN_TRX_AUTOCOMMIT, unless we copy that flag somewhere else).
  • The getDBLoadBalancer(Factory)? service container functions, and/or the old-style wfGet[DL]B global functions.

Maybe both of the last ones? We want to disallow classes with ILoadBalancer parameters, and also inline usage like MediaWikiServices::getInstance()->getDBLoadBalancer()->getConnection().

Ladsgroup subscribed.Jul 6 2021, 12:41 PM

Where the test should reside? In Lib? I want to avoid that as lib should be dismantled. In Client and repo separately? then how are going to test lib? Maybe we shouldn't test lib in this regard at all.

Lucas_Werkmeister_WMDE added a subscriber: Addshore.Jul 6 2021, 12:41 PM

@Addshore had previously found phparch and phpat, two projects that are supposed to help you keep your architecture clean. Though both seem to be fairly focused on namespaces.

I think ideally I would want this to be a part of PHPCS, so that we can allow the permitted places (RepoDomainDbFactory etc.) using // phpcs:disable comments in those files, rather than hard-coding paths to those files in some other test file. But that might not be the easiest solution.

Lucas_Werkmeister_WMDE claimed this task.Jul 6 2021, 12:48 PM
Lucas_Werkmeister_WMDE moved this task from To Do (prioritised from top to bottom) to Doing on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.
Lucas_Werkmeister_WMDE added a comment.EditedJul 6 2021, 3:10 PM

Doesn’t look like there’s any existing PHPCS ruleset we could easily use. (mediawiki-codesniffer has several sniffs for forbidden or deprecated things, but they’re all hard-coded, not configurable. Slevomat coding standard has a forbidden classes sniff, but it’s for detecting when the class is extended (interface is implemented, trait is used), not when it’s referenced in any way. PHPCS itself has a forbidden functions sniff, but it’s for global functions (print → echo), not methods.)

I guess it’ll have to be a PHPUnit test with a bunch of regexes.

gerritbot added a comment.Jul 6 2021, 3:49 PM

Change 703460 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Test number of MW RDBMS references in lib/

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

gerritbot added a project: Patch-For-Review.Jul 6 2021, 3:49 PM
gerritbot added a comment.Jul 6 2021, 4:04 PM

Change 703462 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Also strip line comments in NoBadDependencyUsageTest

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

gerritbot added a comment.Jul 7 2021, 11:31 AM

Change 703587 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Rewrite NoBadDependencyUsageTest as NoBadUsageTest

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

Lucas_Werkmeister_WMDE moved this task from Doing to Peer Review on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Jul 7 2021, 11:31 AM
gerritbot added a comment.Jul 7 2021, 11:37 AM

Change 703588 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] WIP: Add RepoNoBadUsageTest

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

gerritbot added a comment.Jul 8 2021, 8:54 AM

Change 703460 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Test number of MW RDBMS references in lib/

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

gerritbot added a comment.Jul 8 2021, 8:54 AM

Change 703462 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Also strip line comments in NoBadDependencyUsageTest

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

gerritbot added a comment.Jul 8 2021, 8:56 AM

Change 703587 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Rewrite NoBadDependencyUsageTest as NoBadUsageTest

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

gerritbot added a comment.Jul 8 2021, 8:56 AM

Change 703588 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Add RepoNoBadUsageTest

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

ReleaseTaggerBot added a project: MW-1.37-notes (1.37.0-wmf.14; 2021-07-12).Jul 8 2021, 9:00 AM
Maintenance_bot removed a project: Patch-For-Review.Jul 8 2021, 9:10 AM
Lucas_Werkmeister_WMDE moved this task from Peer Review to Doing on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Jul 8 2021, 9:31 AM
gerritbot added a comment.Jul 8 2021, 10:47 AM

Change 703720 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Add ClientNoBadUsageTest

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

gerritbot added a project: Patch-For-Review.Jul 8 2021, 10:47 AM
Lucas_Werkmeister_WMDE moved this task from Doing to Peer Review on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Jul 8 2021, 10:47 AM
gerritbot added a comment.Jul 9 2021, 10:56 AM

Change 703720 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Add ClientNoBadUsageTest

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

• toan moved this task from Peer Review to Test (Verification) on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Jul 9 2021, 12:19 PM
Addshore closed this task as Resolved.Jul 15 2021, 9:47 AM

Awesome refactoring of the test happened here too ! :)

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