Page MenuHomePhabricator

Inject a LanguageConverterFactory through ParserFactory into Parser
Closed, ResolvedPublic

Description

Replace direct access to MediaWikiServices::getInstance()->getLanguageConverterFactory() with injected factory

Event Timeline

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

At the moment too many places (both core and extensions) where a parser is created with legacy arguments: new Parser(); or new Parser($config)

https://codesearch.wmflabs.org/search/?q=new%20Parser%5C(%5C)&i=nope&files=&repos=
So hard deprecation can't be applied here before removing the direct creation of the parser.

Moreover, I believe the way how we work with deprecation now isn't efficient enough:

For example, in order to introduce a languageConverterFactory argument in Parser::_contrstruct( ..., I have to:

  1. Add default $languageConverterFactory = null
  2. We have to add a fallback not to break backward compatibility:

$this->languageConverterFactory = $languageConverterFactory ?? MediaWikiServices::getInstance()->getLanguageConverterFactory();

  1. I can't use hard deprecation like in T243317#5823612.
  2. We have to repeat the same for ParserFactory.

It's not clear enough why we have to support legacy arguments in Factories, as Factory is something that supposed to be created in MediaWikiServices only. So their constructors for internal usage only.

  1. Why we need all those fallbacks then: $this->factory = $factory ?? MediaWikiServices::getInstance()->getParserFactory();
		$this->specialPageFactory = $spFactory ??
			MediaWikiServices::getInstance()->getSpecialPageFactory();
		$this->linkRendererFactory = $linkRendererFactory ??
			MediaWikiServices::getInstance()->getLinkRendererFactory();
		$this->nsInfo = $nsInfo ?? MediaWikiServices::getInstance()->getNamespaceInfo();
		$this->logger = $logger ?: new NullLogger();
		$this->badFileLookup = $badFileLookup ??
			MediaWikiServices::getInstance()->getBadFileLookup();
  1. Moreover, we've got a test that verifies supporting ParserFactoryIntegrationTest::testBackwardsCompatibleConstructorArguments

Maybe I'm wrong, but I believe that introducing the DI for the class should start from removing the legacy creation of this class across all the code.
I mean once we introduce ParserFactory we have to immediately rewrite all places where Parser is created using factories before changing class Parser itself.

Change 569028 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/core@master] parser: Inject a LanguageConverterFactory through ParserFactory into class Parser

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

Change 569028 merged by jenkins-bot:
[mediawiki/core@master] parser: Inject a LanguageConverterFactory through DI containers:

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