Page MenuHomePhabricator
Create Task
Maniphest T196053

Make WikibaseQualityConstraints use MediawikiServices for its services
Closed, ResolvedPublic
Actions

Description

From https://gerrit.wikimedia.org/r/#/c/434704/

This way is an anti-pattern and we should not go this way. The standard that people are trying to apply to mediawiki and its extensions (for now) is to use MediawikiServices instead. The best extension I can find is https://github.com/wikimedia/mediawiki-extensions-Cognate Please use it this way

Patch-For-Review:

Details

Show related patches Customize query in gerrit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 31 2018, 9:37 AM
Lucas_Werkmeister_WMDE renamed this task from Make WikibaseQaulityConstraints use MediawikiServices for its services to Make WikibaseQualityConstraints use MediawikiServices for its services.May 31 2018, 11:44 AM
• RazShuty added a project: Wikidata-Campsite.Jun 6 2018, 9:42 AM
Lucas_Werkmeister_WMDE claimed this task.Jun 11 2018, 1:06 PM
Lucas_Werkmeister_WMDE moved this task from Incoming to In Progress on the Wikidata-Campsite board.
Lucas_Werkmeister_WMDE added a subscriber: Addshore.Jun 11 2018, 1:26 PM

I’m a bit confused about the use of MediaWikiServices there. Consider the following snippet of Cognate’s ServiceWiring.php:

	'CognateRepo' => function ( MediaWikiServices $services ) {
		$repo = new CognateRepo(
			CognateServices::getStore(),
			CognateServices::getCacheInvalidator(),
			$services->getTitleFormatter(),
			CognateServices::getLogger()
		);
		$repo->setStatsdDataFactory( $services->getStatsdDataFactory() );
		return $repo;
	},

This function uses the CognateServices utility methods to get some other services which, ultimately, are also declared in ServiceWiring.php. But those utility methods always use MediaWikiServices::getInstance(), not the $services passed into the CognateRepo callback. So as far as I can tell, it’s possible that CognateRepo contains a mixture of service from two different service locators – the default one, and whatever was passed into the callback. Isn’t that a problem?

@Addshore perhaps you can comment on this? It looks like you were the main person to work on MediaWikiServices in Cognate.

gerritbot subscribed.Jun 11 2018, 3:57 PM

Change 439618 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Set up ServiceWiring, add LoggingHelper service

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

gerritbot added a project: Patch-For-Review.Jun 11 2018, 3:57 PM

Change 439619 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Move ConstraintRepository+ConstraintLookup to services

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

gerritbot added a comment.Jun 11 2018, 3:57 PM

Change 439620 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Move CheckResultSerializer+Deserializer to services

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

Lucas_Werkmeister_WMDE updated the task description. (Show Details)Jun 11 2018, 3:58 PM

I’ve started moving some services into service wiring. To solve the problem described above, the ConstraintsServices methods take an optional $services parameter to use that MediaWikiServices instance instead of the default one.

Lucas_Werkmeister_WMDE moved this task from In Progress to Peer Review on the Wikidata-Campsite board.Jun 11 2018, 3:59 PM
gerritbot added a comment.Jun 12 2018, 12:55 PM

Change 439618 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Set up ServiceWiring, add LoggingHelper service

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

Lucas_Werkmeister_WMDE updated the task description. (Show Details)Jun 12 2018, 1:02 PM
ReleaseTaggerBot added a project: MW-1.32-notes (WMF-deploy-2018-06-12 (1.32.0-wmf.8)).Jun 12 2018, 2:00 PM
• Jonas moved this task from Peer Review to Done on the Wikidata-Campsite board.Jun 12 2018, 2:26 PM
gerritbot added a comment.Jun 12 2018, 2:33 PM

Change 439619 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Move ConstraintRepository+ConstraintLookup to services

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

Lucas_Werkmeister_WMDE updated the task description. (Show Details)Jun 12 2018, 3:02 PM
gerritbot added a comment.Jun 12 2018, 3:09 PM

Change 439934 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Extract constants for service names

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

Lucas_Werkmeister_WMDE updated the task description. (Show Details)Jun 12 2018, 3:11 PM
Lucas_Werkmeister_WMDE moved this task from Done to Peer Review on the Wikidata-Campsite board.Jun 12 2018, 3:31 PM
gerritbot added a comment.Jun 12 2018, 3:53 PM

Change 439953 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Move ViolationMessageSerializer+Deserializer to services

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

Lucas_Werkmeister_WMDE updated the task description. (Show Details)Jun 12 2018, 3:53 PM
Lucas_Werkmeister_WMDE added a comment.Jun 12 2018, 3:56 PM

@Ladsgroup are you aware of any existing task to use MediaWikiServices in Wikibase itself? Because a lot of our services depend on Wikibase services, which we currently get from WikibaseRepo::getDefaultInstance().

gerritbot added a comment.Jun 12 2018, 4:16 PM

Change 439962 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Move ConstraintParameterParser to services

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

Lucas_Werkmeister_WMDE updated the task description. (Show Details)Jun 12 2018, 4:16 PM
gerritbot added a comment.Jun 12 2018, 4:28 PM

Change 439620 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Move CheckResultSerializer+Deserializer to services

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

gerritbot added a comment.Jun 12 2018, 4:30 PM

Change 439934 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Extract constants for service names

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

Lucas_Werkmeister_WMDE updated the task description. (Show Details)Jun 12 2018, 4:55 PM
ReleaseTaggerBot edited projects, added MW-1.32-notes (WMF-deploy-2018-06-26 (1.32.0-wmf.10)); removed MW-1.32-notes (WMF-deploy-2018-06-12 (1.32.0-wmf.8)).Jun 12 2018, 5:00 PM
Lucas_Werkmeister_WMDE updated the task description. (Show Details)Jun 14 2018, 12:10 PM
gerritbot added a comment.Jun 14 2018, 2:31 PM

Change 439953 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Move ViolationMessageSerializer+Deserializer to services

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

• Jonas updated the task description. (Show Details)Jun 18 2018, 10:38 AM
• Jonas moved this task from Peer Review to In Progress on the Wikidata-Campsite board.
gerritbot added a comment.Jun 18 2018, 10:51 AM

Change 439962 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Move ConstraintParameterParser to services

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

gerritbot added a comment.Jun 25 2018, 4:17 PM

Change 441905 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Move four helper classes to services

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

Lucas_Werkmeister_WMDE updated the task description. (Show Details)Jun 25 2018, 4:18 PM
Addshore added a comment.Jun 26 2018, 1:30 PM
In T196053#4272028, @Lucas_Werkmeister_WMDE wrote:

I’m a bit confused about the use of MediaWikiServices there. Consider the following snippet of Cognate’s ServiceWiring.php:

	'CognateRepo' => function ( MediaWikiServices $services ) {
		$repo = new CognateRepo(
			CognateServices::getStore(),
			CognateServices::getCacheInvalidator(),
			$services->getTitleFormatter(),
			CognateServices::getLogger()
		);
		$repo->setStatsdDataFactory( $services->getStatsdDataFactory() );
		return $repo;
	},

This function uses the CognateServices utility methods to get some other services which, ultimately, are also declared in ServiceWiring.php. But those utility methods always use MediaWikiServices::getInstance(), not the $services passed into the CognateRepo callback. So as far as I can tell, it’s possible that CognateRepo contains a mixture of service from two different service locators – the default one, and whatever was passed into the callback. Isn’t that a problem?

@Addshore perhaps you can comment on this? It looks like you were the main person to work on MediaWikiServices in Cognate.

Yes, this should probably get services from the main service locator passed into the function

Ladsgroup moved this task from incoming to in progress on the Wikidata board.Jun 28 2018, 3:14 PM
ReleaseTaggerBot edited projects, added MW-1.32-notes (WMF-deploy-2018-07-10 (1.32.0-wmf.12)); removed MW-1.32-notes (WMF-deploy-2018-06-26 (1.32.0-wmf.10)).Jun 30 2018, 1:01 PM
• Vvjjkkii renamed this task from Make WikibaseQualityConstraints use MediawikiServices for its services to exbaaaaaaa.Jul 1 2018, 1:07 AM
• Vvjjkkii removed Lucas_Werkmeister_WMDE as the assignee of this task.
• Vvjjkkii triaged this task as High priority.
• Vvjjkkii edited projects, added CheckUser, Connected-Open-Heritage-Batch-uploads (RAÄ-KMB_1_2017-02), Tamil-Sites, Gamepress, Hashtags, Jade, KartoEditor, Language-2018-Apr-June, New-Editor-Experiences, Mail, TCB-Team (now WMDE-TechWish); removed MW-1.32-notes (WMF-deploy-2018-07-10 (1.32.0-wmf.12)).
• Vvjjkkii updated the task description. (Show Details)
• Vvjjkkii removed subscribers: gerritbot, Aklapper.
CommunityTechBot renamed this task from exbaaaaaaa to Make WikibaseQualityConstraints use MediawikiServices for its services.Jul 2 2018, 3:16 AM
CommunityTechBot assigned this task to Lucas_Werkmeister_WMDE.
CommunityTechBot raised the priority of this task from High to Needs Triage.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot removed projects: TCB-Team (now WMDE-TechWish), Mail, New-Editor-Experiences, Language-2018-Apr-June, KartoEditor, Jade, Hashtags, Gamepress, Tamil-Sites, Connected-Open-Heritage-Batch-uploads (RAÄ-KMB_1_2017-02), CheckUser.
CommunityTechBot added subscribers: gerritbot, Aklapper.
Addshore moved this task from Unsorted to Needs refactor on the Technical-Debt board.Jul 12 2018, 3:57 PM
Addshore added a comment.Aug 18 2018, 1:21 PM

@Lucas_Werkmeister_WMDE what's the status of this one?
It has been in doing on the campsite for quite some time, perhaps we should come up with a final list of the things that need to be moved to use MediaWikiServices and make some sub tasks so it is more manageable and can be shared around?

Addshore added a project: [DEPRECATED] wdwb-tech.Aug 18 2018, 5:25 PM
Addshore moved this task from Inbox to Investigate & Discuss on the [DEPRECATED] wdwb-tech board.Aug 18 2018, 8:58 PM
Lucas_Werkmeister_WMDE added a comment.Aug 20 2018, 11:12 AM

Well, it was blocked for a while on SparqlHelper, which can’t trivially be made into a service because it’s sometimes null.

The result of the discussion on Discourse was that there are two ways forward:

  • Hack: Add some kind of DummySparqlHelper which we use instead of null. All the checkers and other helpers which get a SparqlHelper injected and check if it’s null instead check if it’s a DummySparqlHelper. This is ugly, but fairly straightforward.
  • Proper solution: Refactor the code until we don’t need any such hacks. This will probably mean that there won’t be any such thing as a SparqlHelper; instead, there’ll be one TypeCheckerHelper with SPARQL support and one without it, one SPARQL-backed UniqueValueChecker and a dummy “can’t check this constraint” checker for the same constraint type, same thing for “format” constraints, etc.

The “proper solution” looks like it will require a lot of work, for relatively little benefit (cleaner code, I guess), and I’m still not totally sure what it should look like (for example, the code to actually do an HTTP request to the SPARQL endpoint and parse the response into JSON should probably still be unified, but where? that’s another service that should really be null when a SPARQL endpoint is not configured).

The “hack” shouldn’t be too complicated, I just haven’t done it yet. But that’s probably the easiest way forward for now, to unblock the rest of the services conversion.

Addshore moved this task from Investigate & Discuss to Research on the [DEPRECATED] wdwb-tech board.Aug 20 2018, 11:44 AM
Addshore added a project: Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)).Aug 23 2018, 9:27 AM
Addshore moved this task from To Do (prioritised from top to bottom) to Doing on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Aug 23 2018, 9:27 AM
gerritbot added a comment.Sep 3 2018, 3:30 PM

Change 457493 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Replace null SparqlHelper with DummySparqlHelper

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

Lucas_Werkmeister_WMDE updated the task description. (Show Details)Sep 3 2018, 3:34 PM
Lucas_Werkmeister_WMDE moved this task from Doing to Peer Review on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.

I’ve gone ahead with the hack and rebased the old change. Should be ready for review again.

Lucas_Werkmeister_WMDE updated the task description. (Show Details)Sep 3 2018, 3:37 PM
Addshore moved this task from Research to Blocked on the [DEPRECATED] wdwb-tech board.Sep 10 2018, 9:00 AM
gerritbot added a comment.Sep 10 2018, 9:46 AM

Change 457493 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Replace null SparqlHelper with DummySparqlHelper

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

ReleaseTaggerBot added a project: MW-1.32-notes (WMF-deploy-2018-09-18 (1.32.0-wmf.22)).Sep 10 2018, 10:00 AM
Lucas_Werkmeister_WMDE updated the task description. (Show Details)Sep 10 2018, 10:29 AM
gerritbot added a comment.Sep 10 2018, 11:17 AM

Change 441905 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Move four helper classes to services

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

Lucas_Werkmeister_WMDE updated the task description. (Show Details)Sep 10 2018, 11:57 AM
Lucas_Werkmeister_WMDE moved this task from Peer Review to Doing on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Sep 11 2018, 12:50 PM
gerritbot added a comment.Sep 11 2018, 3:01 PM

Change 459801 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Register two Wikibase services with MediaWikiServices

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

gerritbot added a comment.Sep 11 2018, 3:01 PM

Change 459802 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Move constraint checkers and DelegatingConstraintChecker to services

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

Lucas_Werkmeister_WMDE updated the task description. (Show Details)Sep 11 2018, 3:02 PM
Lucas_Werkmeister_WMDE moved this task from Doing to Peer Review on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.
• Jonas updated the task description. (Show Details)Sep 17 2018, 10:27 AM
gerritbot added a comment.Sep 17 2018, 10:54 AM

Change 459801 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Register two Wikibase services with MediaWikiServices

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

gerritbot added a comment.Sep 17 2018, 2:35 PM

Change 460913 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Extract Wikibase services from default service wiring

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

Lucas_Werkmeister_WMDE updated the task description. (Show Details)Sep 17 2018, 2:37 PM
gerritbot added a comment.Sep 17 2018, 3:18 PM

Change 460918 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Move ResultsSource to services

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

Lucas_Werkmeister_WMDE updated the task description. (Show Details)Sep 17 2018, 3:18 PM
Addshore triaged this task as Medium priority.Sep 19 2018, 7:02 AM
• Jonas updated the task description. (Show Details)Sep 21 2018, 9:42 AM
• Jonas subscribed.

Is this done now?

gerritbot added a comment.Sep 21 2018, 10:08 AM

Change 460913 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Extract Wikibase services from default service wiring

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

gerritbot added a comment.Sep 21 2018, 10:08 AM

Change 459802 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Move constraint checkers and DelegatingConstraintChecker to services

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

gerritbot added a comment.Sep 21 2018, 10:08 AM

Change 460918 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Move ResultsSource to services

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

ReleaseTaggerBot edited projects, added MW-1.32-notes (WMF-deploy-2018-09-25 (1.32.0-wmf.23)); removed MW-1.32-notes (WMF-deploy-2018-09-18 (1.32.0-wmf.22)).Sep 21 2018, 11:00 AM
Lucas_Werkmeister_WMDE closed this task as Resolved.Sep 24 2018, 9:36 AM
Lucas_Werkmeister_WMDE moved this task from Peer Review to Done on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.

I uploaded I7e91dd84ed to fix the Cognate issue I mentioned in T196053#4272028, and apart from that I think this is done, yes.

Addshore removed a project: Wikidata-Campsite.Nov 1 2018, 11:22 AM
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 · Credits