Page MenuHomePhabricator

Decouple $wgSearchType/$wgSearchTypeAlternatives from PHP class
Closed, ResolvedPublic

Description

Forked from T250812: Deal with dropping of Global CirrusSearch class

It's awkward that we basically end up doing new $wgSearchType() in PHP code to instantiate the class for the search, expecting a 1:1 mapping between the value set in $wgSearchType/$wgSearchTypeAlternatives to be a PHP class

We need some way of decoupling/mapping this

Use case is for switching from the global CirrusSearch class in T250812: Deal with dropping of Global CirrusSearch class to the namespaced CirruSearch\CirrusSearch without necessarily having to change $wgSearchType in config... If there was some way to tell MW that for $wgSearchType 'CirrusSearch' to use a mapped class name, rather than instantiating the text in the string

Either potentially a mapping variable that CirrusSearch can set in extension.json ('cirrussearch' => 'CirruSearch\CirrusSearch'), or updating $wgSearchType/$wgSearchTypeAlternatives to take a similar format if a key and value is provided, instantiate value for the key, and falling back (and deprecate?) the non key => value format

Event Timeline

Reedy created this task.Apr 24 2020, 2:29 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 24 2020, 2:29 AM
Reedy renamed this task from Decouple $wgSearchType/$wgSearchTypeAlternatives to Decouple $wgSearchType/$wgSearchTypeAlternatives from PHP class.Apr 24 2020, 2:29 AM
daniel added a subscriber: daniel.

Moving this to "atching" on the CPT board. The proposal sounds good, but it seems to me this should be owned by Discovery-Search.

Reedy added a comment.EditedApr 27 2020, 11:27 AM

Cheers Daniel. Even if CPT isn't going to do it, mind chiming in about the below? I don't know if Discovery-Search are necessarily going to have the time to work on this any time soon, so I might JFDI so we can get it in 1.35 as the LTS

Any suggestion from anyone with how to proceed? It seems to me changing the format of $wgSearchType/$wgSearchTypeAlternatives is probably complications waiting to happen. Generally, the docs have the user set $wgSearchType in LocalSettings.php, and in the cases of like CirrusSearch, after the wfLoadExtension(), so expecting them to set $wgSearchType['CirrusSearch'] = 'CirrusSearch\CirrusSeach...

So that leaves some sort of mapping elsewhere. Another $wg/config mapping that can be set in extension.json (though, attributes feels more sensible, but it's not an extension depending on each other)? A hook? Some sort of Service addition/override?

That all being said, just putting a top level item in extension.json... SearchMapping (not fussed on actual resultant name) and it taking something like...

"SearchMapping": { 'displayname': 'Namespaced\\Class\\Name' }

and then in the real world...

"SearchMapping": { 'CirrusSeach': 'CirrusSearch\\CirrusSearch' }

Feels the right way forward

I guess then in SearchEngineFactory::create() we can use the mapping... Renaming the variables to reflect some sort of reality (class becomes $name or similar, and then lookup $name in $wgSearchMappingVariableThing` and use that if there's a result

	/**
	 * Create SearchEngine of the given type.
	 * @param string|null $type
	 * @return SearchEngine
	 */
	public function create( $type = null ) {
		$configuredClass = $this->config->getSearchType();
		$alternativesClasses = $this->config->getSearchTypes();

		$lb = MediaWikiServices::getInstance()->getDBLoadBalancer();
		if ( $type !== null && in_array( $type, $alternativesClasses ) ) {
			$class = $type;
		} elseif ( $configuredClass !== null ) {
			$class = $configuredClass;
		} else {
			$class = self::getSearchEngineClass( $lb );
		}

		if ( is_subclass_of( $class, SearchDatabase::class ) ) {
			return new $class( $lb );
		} else {
			return new $class();
		}
	}

Using phab as a rubber duck.. :D

Change 592650 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@master] Add $wgSearchMappings

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

Change 592656 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/CirrusSearch@master] Add "SearchMappings" to extension.json

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

This looks like the handler/registry pattern we are already using elsewhere. In the past, we have generally used a global variable for such a mapping (e.g. $wgContentHandlers). Using an ExtensionRegistry attribute is the better solution, since it avoids global state. In any case, the implementation should not be specified as a class name, but an "object spec" as supported by ObjectFactory. See how MediaWiki\Rest\EntryPoint instantiates the Router for an example of how this could work.

Reedy added a comment.Apr 27 2020, 2:33 PM

This looks like the handler/registry pattern we are already using elsewhere. In the past, we have generally used a global variable for such a mapping (e.g. $wgContentHandlers). Using an ExtensionRegistry attribute is the better solution, since it avoids global state. In any case, the implementation should not be specified as a class name, but an "object spec" as supported by ObjectFactory. See how MediaWiki\Rest\EntryPoint instantiates the Router for an example of how this could work.

ExtensionRegistry attributes in the extension schema are documented as

Registration information for other extensions

So it doesn't seem right to abuse this for stuff in MW core

I'll have a look at the ObjectFactor/object spec stuff and see if I make some adjustments

Obviously we need to keep the old style new $wgSearchType style to keep that back compatability, but certainly for this "mapping" we should be able to do something "better"

Reedy added a comment.Apr 27 2020, 4:03 PM

I'll have a look at the ObjectFactor/object spec stuff and see if I make some adjustments

Done

Change 592650 merged by jenkins-bot:
[mediawiki/core@master] search: Add 'SearchMappings' attribute to map canonical name to PHP class

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

Reedy closed this task as Resolved.Apr 30 2020, 2:28 PM
Reedy claimed this task.
Reedy removed a project: Patch-For-Review.

Change 592656 merged by jenkins-bot:
[mediawiki/extensions/CirrusSearch@master] Add "SearchMappings" to extension.json

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

Change 593582 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@master] registration: Add SearchMappings to CORE_ATTRIBS

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

Reedy reopened this task as Open.Apr 30 2020, 7:26 PM

Not quite working as expected. Patches in progress

Change 593582 merged by jenkins-bot:
[mediawiki/core@master] registration: Move SearchMappings to CORE_ATTRIBS from NOT_ATTRIBS

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

Reedy closed this task as Resolved.May 2 2020, 6:53 PM

Change 596417 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@master] Add RELEASE-NOTES for extension.json SearchMappings

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

Change 596417 merged by jenkins-bot:
[mediawiki/core@master] Add RELEASE-NOTES for extension.json SearchMappings

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