Replace direct access to MediaWikiServices::getInstance()->getLanguageConverterFactory() with injected factory
Description
Details
Subject | Repo | Branch | Lines +/- | |
---|---|---|---|---|
parser: Inject a LanguageConverterFactory through DI containers: | mediawiki/core | master | +300 -169 |
Status | Subtype | Assigned | Task | ||
---|---|---|---|---|---|
Resolved | roman-stolar | T226832 Deprecate Language::convertTitle(), findVariantLink(), updateConversionTable(), and friends | |||
Resolved | Peter.ovchyn | T226833 Introduce LanguageConverterFactory service | |||
Resolved | Peter.ovchyn | T243320 Inject a LanguageConverterFactory through ParserFactory into Parser |
Event Timeline
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:
- Add default $languageConverterFactory = null
- We have to add a fallback not to break backward compatibility:
$this->languageConverterFactory = $languageConverterFactory ?? MediaWikiServices::getInstance()->getLanguageConverterFactory();
- I can't use hard deprecation like in T243317#5823612.
- 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.
- 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();
- 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
Change 569028 merged by jenkins-bot:
[mediawiki/core@master] parser: Inject a LanguageConverterFactory through DI containers: