Page MenuHomePhabricator

Deprecate/Remove ContentHandler::makeParserOptions()
Closed, ResolvedPublic

Description

ContentHandler::makeParserOptions() should be removed since it is ill-conceived, and incompatible with the idea of MCR:

Ill-conceived because the only reason to have it would be content-specific options, which would be set depending on some outside factor like user options. But that kind of thing should not exist: MediaWiki core should be able to have definite knowledge of what parameters influence parsing, to enable caching, etc.

Incompatible with MCR because the need for constructing a combined ParserOutput object. Which ContentHandler should control the options for the combined output?

The creation of canonical ParserOptions could be left to a RevisionRenderer (T194048) or some kind of PageTypeHandler. For now, it probably OK to move the logic to WikiPage::makeParserOptions(). Or have it in DerivedPageDataUpdater::getCanonicalParserOptions() directly. But we'll still need WikiPage::makeParserOptions() to work, for backwards compatibility. It's used quite a bit. On the other hand, WikiPage is going to be deprecated as the MCR refactoring progresses.

Event Timeline

Vvjjkkii renamed this task from Deprecate/Remove ContentHandler::makeParserOptions() to 4adaaaaaaa.Jul 1 2018, 1:10 AM
Vvjjkkii raised the priority of this task from Normal to High.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed subscribers: gerritbot, Aklapper.
CommunityTechBot renamed this task from 4adaaaaaaa to Deprecate/Remove ContentHandler::makeParserOptions().Jul 2 2018, 6:03 AM
CommunityTechBot lowered the priority of this task from High to Normal.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added subscribers: gerritbot, Aklapper.
daniel updated the task description. (Show Details)Jul 9 2018, 4:33 PM

For now, it probably OK to move the logic to WikiPage::makeParserOptions().

I'd be fine with that, except we don't normally have a WikiPage where we need one. We usually do have a Title, but WikiPage::factory() chokes on Special-page titles and the like making it hard to use. Putting it on any other instanced object (except maybe Title, which is already too huge) would have similar issues.

Callers of the method I see in my current git checkout:

  • WikitextContentTest::testRedirectParserOption() - Meh, it's a test.
  • CategoryMembershipChangeJob::getCategoriesAtRev() - Could pass down a WikiPage.
  • WikiPage::makeParserOptions() - This would be the new "getter".
  • AbstractContent::getParserOutput() - We only really have a Title.
    • ProofreadPage\Page\PageContent::getParserOutput()
    • TemplateStylesContent::getParserOutput()
    • Wikibase\EntityContent::getParserOutput()
  • ProofreadPage\Index\IndexContentHandler::buildParser() - We don't even know what title we're trying to handle here.
  • ApiParsoidBatch::parse() - Only a Title.
    • ApiParsoidBatch::preprocess()
  • ScribuntoContent::fillParserOutput() - Unused code path
  • SpamBlacklistHooks::onUploadVerifyUpload() - Has a Title.

The most straightforward migration would be to replace these all with direct calls to ParserOptions::newCanonical(). Most would pass new User, $wgContLang, while a few would pull the User and Language from the IContextSource they're currently using.

The other option would be to create a static ParserOptions::newCanonicalFromContext( $context ) that works just like the ContentHandler method. Or we could put it on some other class, but I can't think of any other good candidates.

Since I suspect you might suggest it, a "ParserOptionsFactory" would be orthogonal as a refactor of the existing static factory methods on ParserOptions. I'd rather not do two things at once here.

You are right, static factory methods on ParserOptions are probably the best choice for now. Ideally, ParserOutput is a plain value object, then we don't even need a factory - but even if we do, we can add that later.

We definitely want something like ParserOptions::newCanonical(), but I'm a bit torn on what parameters it should take, and what decisions it should make.

  • a Title. Even if we don't do this right now, conceptually, the default target language may depend on the namespace or a suffix.
  • the User. The factory may decide at this point that the out put depends on the user language.
  • ...or just require the language to be passed, avoiding access to the global $wgContLang.

Or maybe, pass nothing at all, leaving it to the callers to use setters?

Anomie added a comment.EditedJul 11 2018, 3:39 PM

ParserOptions::newCanonical() already exists, taking a User and a Language for the 'userlang' option. If they're not passed, they currently default to $wgUser and $wgLang. Now that I think about it fresh, I think I'll just extend that method to accept an IContextSource or the string 'canonical' too.

The ParserOptions itself doesn't hold a title, although perhaps it should instead of the title being a separate parameter to Parser::parse(). The current situation, assuming I haven't missed anything important, is:

  • The Title being parsed is state on the Parser object rather than ParserOptions, and is passed into methods like Content::getParserOutput().
  • The Parser normally determines the "target" language by looking at either the user language or the Title's page language, with the 'targetLanguage' ParserOption being an override that's not often set.
  • The Language object itself doesn't actually know its own variant (for languages with variants), instead the variant to use is determined by LanguageConverter inspecting various bits of global state (see also T44240).
daniel added a comment.EditedJul 11 2018, 6:23 PM

The way languages work is quite terrible, see e.g. T114640: make Parser::getTargetLanguage aware of multilingual wikis and T109705: [Task] Consistently and correctly get target or 'cached' language in ParserOptions when userlang option is used. We'll probably not resolve this here. Passing the Title just makes the interface more future proof, it would be ignored for now.

As to passing IContextSource... please don't. Just ask it for user and language, and pass that. IContextSource is a terrible kitchen sink and we should try to get rid of it, not spread its use.

Change 445220 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] Deprecate ContentHandler::makeParserOptions()

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

Change 445221 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] Hard-deprecate ContentHandler::makeParserOptions()

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

Change 445223 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/extensions/ParsoidBatchAPI@master] Avoid deprecated ContentHandler::makeParserOptions()

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

Change 445224 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/extensions/ProofreadPage@master] Replace deprecated ContentHandler::makeParserOptions()

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

Change 445225 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/extensions/Scribunto@master] Remove unreacahable code path

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

Change 445226 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/extensions/SpamBlacklist@master] Replace deprecated ContentHandler::makeParserOptions()

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

Change 445227 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/extensions/TemplateStyles@master] Replace deprecated ContentHandler::makeParserOptions()

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

Change 445228 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/extensions/Wikibase@master] Replace deprecated ContentHandler::makeParserOptions()

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

Change 445225 merged by jenkins-bot:
[mediawiki/extensions/Scribunto@master] Remove unreachable code path

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

The longer I think about this, the more I want ParserOptions creation to have a Title for context.
The reason is that default parser behavior may vary with the type of page. The special case for conversion tables in WikiPage::makeParserOptions() is the point in case.

We can leave further changes for later, but I think those changes will be easier if we let things call WikiPage::makeParserOptions() for now, instead of changing ParserOptions::newCanonical() to make it accept IContext or Title.

The longer I think about this, the more I want ParserOptions creation to have a Title for context.
The reason is that default parser behavior may vary with the type of page. The special case for conversion tables in WikiPage::makeParserOptions() is the point in case.

That sounds like a clear YAGNI to me. The special case for conversion tables has a comment saying that shouldn't be using Title, it should use a special content model, which seems to me like the more reasonable solution.

We can leave further changes for later, but I think those changes will be easier if we let things call WikiPage::makeParserOptions() for now, instead of changing ParserOptions::newCanonical() to make it accept IContext or Title.

Only one of the current callers of ContentHandler::makeParserOptions() has any reasonable way of getting a WikiPage. So no, let's not make that the solution.

Change 445220 merged by jenkins-bot:
[mediawiki/core@master] Deprecate ContentHandler::makeParserOptions()

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

Change 445223 merged by jenkins-bot:
[mediawiki/extensions/ParsoidBatchAPI@master] Avoid deprecated ContentHandler::makeParserOptions()

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

Change 445224 merged by jenkins-bot:
[mediawiki/extensions/ProofreadPage@master] Replace deprecated ContentHandler::makeParserOptions()

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

Anomie closed this task as Resolved.Jul 13 2018, 7:48 PM

Change 445226 merged by jenkins-bot:
[mediawiki/extensions/SpamBlacklist@master] Replace deprecated ContentHandler::makeParserOptions()

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

Change 445227 merged by jenkins-bot:
[mediawiki/extensions/TemplateStyles@master] Replace deprecated ContentHandler::makeParserOptions()

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

Change 445228 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Replace deprecated ContentHandler::makeParserOptions()

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

Change 445221 merged by jenkins-bot:
[mediawiki/core@master] Hard-deprecate ContentHandler::makeParserOptions()

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