Page MenuHomePhabricator

Make WikibaseQualityConstraints use MediawikiServices for its services
Closed, ResolvedPublic

Details

Related Gerrit Patches:
mediawiki/extensions/WikibaseQualityConstraints : masterMove ResultsSource to services
mediawiki/extensions/WikibaseQualityConstraints : masterExtract Wikibase services from default service wiring
mediawiki/extensions/WikibaseQualityConstraints : masterMove constraint checkers and DelegatingConstraintChecker to services
mediawiki/extensions/WikibaseQualityConstraints : masterRegister two Wikibase services with MediaWikiServices
mediawiki/extensions/WikibaseQualityConstraints : masterMove four helper classes to services
mediawiki/extensions/WikibaseQualityConstraints : masterReplace null SparqlHelper with DummySparqlHelper
mediawiki/extensions/WikibaseQualityConstraints : masterMove ConstraintParameterParser to services
mediawiki/extensions/WikibaseQualityConstraints : masterMove ViolationMessageSerializer+Deserializer to services
mediawiki/extensions/WikibaseQualityConstraints : masterExtract constants for service names
mediawiki/extensions/WikibaseQualityConstraints : masterMove CheckResultSerializer+Deserializer to services
mediawiki/extensions/WikibaseQualityConstraints : masterMove ConstraintRepository+ConstraintLookup to services
mediawiki/extensions/WikibaseQualityConstraints : masterSet up ServiceWiring, add LoggingHelper service

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

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.

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

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

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

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.

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

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

Jonas moved this task from Peer Review to Done on the Wikidata-Campsite board.Jun 12 2018, 2:26 PM

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

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

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

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

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

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

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

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

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

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

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.

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

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

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

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
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 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 raised the priority of this task from High to Needs Triage.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added subscribers: gerritbot, Aklapper.

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

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.

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

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

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

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

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

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

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

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

Jonas updated the task description. (Show Details)Sep 17 2018, 10:27 AM

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

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

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

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

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 added a subscriber: Jonas.

Is this done now?

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

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

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

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

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

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

Lucas_Werkmeister_WMDE closed this task as Resolved.Sep 24 2018, 9:36 AM

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