Page MenuHomePhabricator

Don’t use WikibaseRepo when initializing Client pageterms API
Closed, ResolvedPublic

Description

Currently, when Wikibase Client initializes the pageterms API, it uses WikibaseRepo code if the Repo is enabled:

public static function newFromGlobalState( ApiQuery $apiQuery, string $moduleName ): self {
		// FIXME: HACK: make pageterms work directly on entity pages on the repo.
		// We should instead use an EntityIdLookup that combines the repo and the client
		// implementation, see T115117.
		// NOTE: when changing repo and/or client integration, remember to update the
		// self-documentation of the API module in the "apihelp-query+pageterms-description"
		// message and the PageTerms::getExamplesMessages() method.
		if ( ExtensionRegistry::getInstance()->isLoaded( 'WikibaseRepository' ) ) {
			$repo = WikibaseRepo::getDefaultInstance();
			$termBuffer = $repo->getTermBuffer();
			$entityIdLookup = $repo->getEntityContentFactory();
		} else {
			$client = WikibaseClient::getDefaultInstance();
			$termBuffer = $client->getTermBuffer();
			$entityIdLookup = $client->getEntityIdLookup();
		}

This is a hack dating back to 2015 (T115117#1719166). We should find some better solution for this, depending on what the expected behavior of that API on repo wikis is.

Event Timeline

If the API behavior should stay the same, then I imagine we could resolve the direct Client→Repo dependency by registering some kind of “term buffer + entity ID lookup” stash in Lib, and Repo fills it with its services upon initialization, and Client fills it with its services only if it wasn’t already initialized by Repo. So the two extensions would coordinate via Lib instead of talk directly to one another, so to speak. Something similar could probably also be done if Product says the API should first return the terms of the current entity and otherwise the terms of the entity linked to the current page, or something like that.

If the expected behavior is that the API always returns the terms of the entity connected to the current page, then I think we can simply remove the hack :)

One solution would be to move the “page terms of this item” functionality to a separate API module.

If the API behavior should stay the same, then I imagine we could resolve the direct Client→Repo dependency by registering some kind of “term buffer + entity ID lookup” stash in Lib, and Repo fills it with its services upon initialization, and Client fills it with its services only if it wasn’t already initialized by Repo. So the two extensions would coordinate via Lib instead of talk directly to one another, so to speak. Something similar could probably also be done if Product says the API should first return the terms of the current entity and otherwise the terms of the entity linked to the current page, or something like that.

I would be very much in favor of having simpler logic in our codebase, this seems complicated (meaning harder to maintain and more likely to have bugs and harder onboarding). Sometimes complicated logic is inevitable, like high concurrency, large scale caching, etc. But this case can be resolved by having other API modules (all boils down to what product decide of course) but I would say complicated logic is a negative point for having everything in one place.

Tarrow changed the task status from Open to Stalled.Jul 8 2020, 12:52 PM

Stalled until we know if we retain the "repo" hack. @Lydia_Pintscher / @Samantha_Alipio_WMDE

Tarrow updated the task description. (Show Details)Jul 9 2020, 7:14 AM
Lucas_Werkmeister_WMDE changed the task status from Stalled to Open.Jul 10 2020, 9:46 AM

Decision was made in today’s campsite daily – we’ll move the special behavior to a new entityterms API (T257658).