Page MenuHomePhabricator

Inject LanguageConverter through constructor in SearchNearMatcher
Open, LowPublic

Description

Motivation

At the moment converter for SearchNearMatcher is created in the constructor using MediaWikiServices directly.

...
    $this->languageConverter = MediaWikiServices::getInstance()->getLanguageConverterFactory()
        ->getLanguageConverter( $lang );
...

This leads to unreasonable encreaseing coupling.
The goal is to inject it through constructor arguments.

Expected result:

function __constructor(
...
    $languageConveter
    ) { 
...
    $this->languageConverter = languageConveter;
}

Event Timeline

Peter.ovchyn updated the task description. (Show Details)

This should indeed be done. See T243317#5823612 for the pattern.

Peter.ovchyn updated the task description. (Show Details)Feb 4 2020, 8:32 PM

It's turned out that this task is not that easy:

  1. SearchNearMatcher is created from SearchEngine::getNearMatcher.
public function getNearMatcher( Config $config ) {
	return new SearchNearMatcher( $config,
		MediaWikiServices::getInstance()->getContentLanguage() );
}
  1. SearchEngine is abstract class which might have different implementations. But it's created by SearchEngineFactory
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();
	}
}

So in order to inject Language and Container into SearchNearMatcher we have to inject them to SearchEngineFactory and SearchEngine first.

But I don't think that's the correct way as SearchNearMatcher isn't connected with SearchEngine and doesn't use any data from neither in core nor in extensions. So no need to use SearchEngine just as Factory for SearchNearMatcher.

I'd rather introduce SearchNearMatcherFactory and inject all required data from the container through it. This way we'll get rid of the dependency between SearchEngine and SearchNearMatcher.

daniel triaged this task as Low priority.Feb 5 2020, 12:59 PM

I'd rather introduce SearchNearMatcherFactory and inject all required data from the container through it. This way we'll get rid of the dependency between SearchEngine and SearchNearMatcher.

Yes, that sounds like a good approach. May be complicated though if different SearchEngine implementations want to create different SearchNearMatcher instances.

This is not high priority for now. We can push it back. Lowering prio.

Peter.ovchyn removed Peter.ovchyn as the assignee of this task.Feb 12 2020, 10:05 AM