Page MenuHomePhabricator
Create Task
Maniphest T194263

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

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.

Details

SubjectRepoBranchLines +/-
Hard-deprecate ContentHandler::makeParserOptions()mediawiki/coremaster+1 -0
Replace deprecated ContentHandler::makeParserOptions()mediawiki/extensions/Wikibasemaster+1 -1
Replace deprecated ContentHandler::makeParserOptions()mediawiki/extensions/TemplateStylesmaster+1 -1
Replace deprecated ContentHandler::makeParserOptions()mediawiki/extensions/SpamBlacklistmaster+1 -1
Deprecate ContentHandler::makeParserOptions()mediawiki/coremaster+113 -43
Replace deprecated ContentHandler::makeParserOptions()mediawiki/extensions/ProofreadPagemaster+3 -2
Avoid deprecated ContentHandler::makeParserOptions()mediawiki/extensions/ParsoidBatchAPImaster+12 -4
Remove unreachable code pathmediawiki/extensions/Scribuntomaster+3 -8
Customize query in gerrit

Related Objects
Search...

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.
StatusSubtypeAssignedTask
· · ·
ResolveddanielT194048 Introduce RevisionRenderer (baseline)
ResolvedAnomieT194263 Deprecate/Remove ContentHandler::makeParserOptions()
ResolvedAnomieT190712 Wikibase\Repo\Content\EntityHandler should not override makeParserOptions()
· · ·

Event Timeline

daniel triaged this task as Medium priority.May 9 2018, 1:09 PM
daniel created this task.
daniel added a subtask: T190712: Wikibase\Repo\Content\EntityHandler should not override makeParserOptions().
CCicalese_WMF edited projects, added Multi-Content-Revisions (MCR-SDC File Caption Support - phase 2); removed Multi-Content-Revisions.May 31 2018, 2:20 AM
• 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 Medium to High.
• Vvjjkkii added projects: CheckUser, Connected-Open-Heritage-Batch-uploads (RAÄ-KMB_1_2017-02), Tamil-Sites, Gamepress, Hashtags, Jade, KartoEditor, Language-2018-Apr-June, New-Editor-Experiences, Mail, TCB-Team (now WMDE-TechWish).
• 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 Medium.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot removed projects: TCB-Team (now WMDE-TechWish), Mail, New-Editor-Experiences, Language-2018-Apr-June, KartoEditor, Jade, Hashtags, Gamepress, Tamil-Sites, Connected-Open-Heritage-Batch-uploads (RAÄ-KMB_1_2017-02), CheckUser.
CommunityTechBot added subscribers: gerritbot, Aklapper.
daniel updated the task description. (Show Details)Jul 9 2018, 4:33 PM
CCicalese_WMF moved this task from Backlog to Ready on the Multi-Content-Revisions (MCR-SDC File Caption Support - phase 2) board.Jul 9 2018, 5:20 PM
CCicalese_WMF assigned this task to Anomie.Jul 9 2018, 5:28 PM
Anomie added a comment.Jul 10 2018, 8:38 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.

Anomie added a project: MediaWiki-Platform-Team-Archived (MWPT-Q1-Jul-Sep-2018).Jul 10 2018, 8:39 PM
Anomie moved this task from Ready to In Progress on the MediaWiki-Platform-Team-Archived (MWPT-Q1-Jul-Sep-2018) board.
daniel added a comment.Jul 10 2018, 10:01 PM

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).
Anomie closed subtask T190712: Wikibase\Repo\Content\EntityHandler should not override makeParserOptions() as Resolved.Jul 11 2018, 5:52 PM
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.

gerritbot added a comment.Jul 11 2018, 6:32 PM

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

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

gerritbot added a project: Patch-For-Review.Jul 11 2018, 6:32 PM

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

gerritbot added a comment.Jul 11 2018, 6:33 PM

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

gerritbot added a comment.Jul 11 2018, 6:33 PM

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

gerritbot added a comment.Jul 11 2018, 6:33 PM

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

gerritbot added a comment.Jul 11 2018, 6:33 PM

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

gerritbot added a comment.Jul 11 2018, 6:33 PM

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

gerritbot added a comment.Jul 11 2018, 6:33 PM

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

Anomie mentioned this in rETST435e06e2f5bc: Replace deprecated ContentHandler::makeParserOptions().Jul 11 2018, 6:38 PM
gerritbot added a comment.Jul 11 2018, 6:57 PM

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

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

ReleaseTaggerBot added a project: MW-1.32-notes (WMF-deploy-2018-07-17 (1.32.0-wmf.13)).Jul 11 2018, 7:00 PM
CCicalese_WMF edited projects, added Core-Platform-Team-Old (CPT-Q1-Jul-Sep-2018); removed MediaWiki-Platform-Team-Archived (MWPT-Q1-Jul-Sep-2018).Jul 11 2018, 9:15 PM
CCicalese_WMF moved this task from Backlog to In Progress on the Core-Platform-Team-Old (CPT-Q1-Jul-Sep-2018) board.Jul 11 2018, 9:24 PM
daniel added a comment.Jul 12 2018, 5:02 PM

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.

Anomie added a comment.Jul 12 2018, 5:40 PM
In T194263#4420512, @daniel wrote:

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.

CCicalese_WMF moved this task from Ready to In Progress on the Multi-Content-Revisions (MCR-SDC File Caption Support - phase 2) board.Jul 13 2018, 12:56 PM
gerritbot added a comment.Jul 13 2018, 6:59 PM

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

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

gerritbot added a comment.Jul 13 2018, 6:59 PM

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

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

gerritbot added a comment.Jul 13 2018, 7:01 PM

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
gerritbot added a comment.Jul 13 2018, 7:50 PM

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

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

gerritbot added a comment.Jul 13 2018, 7:50 PM

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

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

gerritbot added a comment.Jul 13 2018, 8:00 PM

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

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

gerritbot added a comment.Jul 13 2018, 8:01 PM

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

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

CCicalese_WMF moved this task from In Progress to Closed on the Core-Platform-Team-Old (CPT-Q1-Jul-Sep-2018) board.Jul 13 2018, 9:44 PM
CCicalese_WMF moved this task from In Progress to Done on the Multi-Content-Revisions (MCR-SDC File Caption Support - phase 2) board.Jul 13 2018, 10:47 PM
CCicalese_WMF removed a project: Patch-For-Review.Aug 25 2018, 12:59 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL