Page MenuHomePhabricator

Remove the need to generate ParserOutput before PageContentSave hook is called
Open, Needs TriagePublic

Description

Generating ParserOutput is expensive, so it should be deferred as much as possible, and should only be done on demand. When savin an edit, parsing could perhaps even be delayed until after the HTTP response has been sent (though that would mean using PoolCounter for parsing).

At present hower, the ParserOutput needs to be generated before the PageContentSave hook is called, otherwise (at least) ApiFlowEditHeaderTest::testCache breaks. This became apparent while refactoring WikiPage::doEditContent for T174038, see https://gerrit.wikimedia.org/r/c/405015/78/includes/Storage/PageUpdater.php#595

Analysis, per @Anomie:

I suspect that what's breaking is this:
The old version of WikiPage::doEditContent() called prepareContentForEdit() which generated the ParserOutput right then, so when doEditUpdates() gets called from the DeferredUpdate scheduled by WikiPage::doCreate() there's no need to parse. I note there's a comment there that says "Get the pre-save transform content and final parser output".
The new version of WikiPage::doEditContent() makes a PageUpdater and calls its createRevision(), which calls DerivedPageDataUpdater::prepareContent() and PageUpdater::doCreate() without ever having to actually generate a ParserOutput. Thus, when DerivedPageDataUpdater::doUpdates() is called from the DeferredUpdate scheduled by PageUpdater::doCreate(), it does find that it needs to parse at that point.
And the order of operations in that Flow test is presumably:

  • Create a page with a call to WikiPage::doEditContent(), in a way that somehow avoids processing the DeferredUpdate.
  • Set up the "no set!" mock cache in Flow\Tests\Api\ApiTestCase::expectCacheInvalidate()
  • Then, during the course of doing that test, a $db->commit() results in the DeferredUpdates being run.

Event Timeline

Tgr created this task.May 10 2018, 9:02 PM
Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptMay 10 2018, 9:02 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
daniel renamed this task from Make MCR pass ApiFlowEditHeaderTest::testCache to Remove the need to generate ParserOutput before PageContentSave hook is called.May 14 2018, 7:59 AM
daniel updated the task description. (Show Details)
daniel updated the task description. (Show Details)
daniel updated the task description. (Show Details)

Hi @Catrope,

Is test failing? Do we have to fix?

Vvjjkkii renamed this task from Remove the need to generate ParserOutput before PageContentSave hook is called to i6caaaaaaa.Jul 1 2018, 1:10 AM
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed a subscriber: Aklapper.
CommunityTechBot renamed this task from i6caaaaaaa to Remove the need to generate ParserOutput before PageContentSave hook is called.Jul 2 2018, 4:19 PM
CommunityTechBot raised the priority of this task from High to Needs Triage.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added a subscriber: Aklapper.
Restricted Application added a project: Growth-Team. · View Herald TranscriptOct 5 2018, 1:45 PM