Avoid losing cached ParserOutput in doEditContent.

Without this fix, the Content would be parsed twice during an edit,
if extensions that hook into EditFilter and similar
pre-parse hooks call WikiPage::prepareContentForEdit.

With this fix, ParserOutput created by prepareContentForEdit is
re-used by doEditContent.

This is a stop-gap solution. The Real Fix (tm) is to stop using
doEditContent, and use a PageUpdater instead.

Bug: T198483
Change-Id: I42123e48de2b087ef98d8a4855ee3aebd7f1de57

diff --git a/includes/page/WikiPage.php b/includes/page/WikiPage.php
index 5facc62..0b28074 100644 (file)
--- a/includes/page/WikiPage.php
+++ b/includes/page/WikiPage.php
@@ -1664,13 +1664,17 @@ class WikiPage implements Page, IDBAccessObject {
         *        to perform updates, if the edit was already saved.
         * @param RevisionSlotsUpdate|null $forUpdate The new content to be saved by the edit (pre PST),
         *        if the edit was not yet saved.
+        * @param bool $forEdit Only re-use if the cached DerivedPageDataUpdater has the current
+        *       revision as the edit's parent revision. This ensures that the same
+        *       DerivedPageDataUpdater cannot be re-used for two consecutive edits.
         *
         * @return DerivedPageDataUpdater
         */
        private function getDerivedDataUpdater(
                User $forUser = null,
                RevisionRecord $forRevision = null,
-               RevisionSlotsUpdate $forUpdate = null
+               RevisionSlotsUpdate $forUpdate = null,
+               $forEdit = false
        ) {
                if ( !$forRevision && !$forUpdate ) {
                        // NOTE: can't re-use an existing derivedDataUpdater if we don't know what the caller is
@@ -1693,7 +1697,8 @@ class WikiPage implements Page, IDBAccessObject {
                        && !$this->derivedDataUpdater->isReusableFor(
                                $forUser,
                                $forRevision,
-                               $forUpdate
+                               $forUpdate,
+                               $forEdit ? $this->getLatest() : null
                        )
                ) {
                        $this->derivedDataUpdater = null;
@@ -1716,16 +1721,18 @@ class WikiPage implements Page, IDBAccessObject {
         * @since 1.32
         *
         * @param User $user
+        * @param RevisionSlotsUpdate|null $forUpdate If given, allows any cached ParserOutput
+        *        that may already have been returned via getDerivedDataUpdater to be re-used.
         *
         * @return PageUpdater
         */
-       public function newPageUpdater( User $user ) {
+       public function newPageUpdater( User $user, RevisionSlotsUpdate $forUpdate = null ) {
                global $wgAjaxEditStash, $wgUseAutomaticEditSummaries, $wgPageCreationLog;
 
                $pageUpdater = new PageUpdater(
                        $user,
                        $this, // NOTE: eventually, PageUpdater should not know about WikiPage
-                       $this->getDerivedDataUpdater( $user ),
+                       $this->getDerivedDataUpdater( $user, null, $forUpdate, true ),
                        $this->getDBLoadBalancer(),
                        $this->getRevisionStore()
                );
@@ -1820,10 +1827,13 @@ class WikiPage implements Page, IDBAccessObject {
                        $flags = ( $flags & ~EDIT_MINOR );
                }
 
+               $slotsUpdate = new RevisionSlotsUpdate();
+               $slotsUpdate->modifyContent( 'main', $content );
+
                // NOTE: while doEditContent() executes, callbacks to getDerivedDataUpdater and
                // prepareContentForEdit will generally use the DerivedPageDataUpdater that is also
                // used by this PageUpdater. However, there is no guarantee for this.
-               $updater = $this->newPageUpdater( $user );
+               $updater = $this->newPageUpdater( $user, $slotsUpdate );
                $updater->setContent( 'main', $content );
                $updater->setOriginalRevisionId( $originalRevId );
                $updater->setUndidRevisionId( $undidRevId );

diff --git a/tests/phpunit/includes/page/WikiPageDbTestBase.php b/tests/phpunit/includes/page/WikiPageDbTestBase.php
index fa15a12..aaaa73b 100644 (file)
--- a/tests/phpunit/includes/page/WikiPageDbTestBase.php
+++ b/tests/phpunit/includes/page/WikiPageDbTestBase.php
@@ -249,6 +249,8 @@ abstract class WikiPageDbTestBase extends MediaWikiLangTestCase {
                        CONTENT_MODEL_WIKITEXT
                );
 
+               $preparedEditBefore = $page->prepareContentForEdit( $content, null, $user1 );
+
                $status = $page->doEditContent( $content, "[[testing]] 1", EDIT_NEW, false, $user1 );
 
                $this->assertTrue( $status->isOK(), 'OK' );
@@ -259,9 +261,14 @@ abstract class WikiPageDbTestBase extends MediaWikiLangTestCase {
                $this->assertTrue( $status->value['revision']->getContent()->equals( $content ), 'equals' );
 
                $rev = $page->getRevision();
+               $preparedEditAfter = $page->prepareContentForEdit( $content, $rev, $user1 );
+
                $this->assertNotNull( $rev->getRecentChange() );
                $this->assertSame( $rev->getId(), (int)$rev->getRecentChange()->getAttribute( 'rc_this_oldid' ) );
 
+               // make sure that cached ParserOutput gets re-used throughout
+               $this->assertSame( $preparedEditBefore->output, $preparedEditAfter->output );
+
                $id = $page->getId();
 
                // Test page creation logging
@@ -342,6 +349,26 @@ abstract class WikiPageDbTestBase extends MediaWikiLangTestCase {
                $this->assertEquals( 2, $n, 'pagelinks should contain two links from the page' );
        }
 
+       /**
+        * @covers WikiPage::doEditContent
+        */
+       public function testDoEditContent_twice() {
+               $title = Title::newFromText( __METHOD__ );
+               $page = WikiPage::factory( $title );
+               $content = ContentHandler::makeContent( '$1 van $2', $title );
+
+               // Make sure we can do the exact same save twice.
+               // This tests checks that internal caches are reset as appropriate.
+               $status1 = $page->doEditContent( $content, __METHOD__ );
+               $status2 = $page->doEditContent( $content, __METHOD__ );
+
+               $this->assertTrue( $status1->isOK(), 'OK' );
+               $this->assertTrue( $status2->isOK(), 'OK' );
+
+               $this->assertTrue( isset( $status1->value['revision'] ), 'OK' );
+               $this->assertFalse( isset( $status2->value['revision'] ), 'OK' );
+       }
+
        /**
         * Undeletion is covered in PageArchiveTest::testUndeleteRevisions()
         * TODO: Revision deletion
@@ -2278,14 +2305,25 @@ more stuff
                        ->method( 'getParserOutput' )
                        ->willReturn( new ParserOutput( 'HTML' ) );
 
-               $updater = $page->newPageUpdater( $user );
+               $preparedEditBefore = $page->prepareContentForEdit( $content, null, $user );
+
+               // provide context, so the cache can be kept in place
+               $slotsUpdate = new revisionSlotsUpdate();
+               $slotsUpdate->modifyContent( 'main', $content );
+
+               $updater = $page->newPageUpdater( $user, $slotsUpdate );
                $updater->setContent( 'main', $content );
                $revision = $updater->saveRevision(
                        CommentStoreComment::newUnsavedComment( 'test' ),
                        EDIT_NEW
                );
 
+               $preparedEditAfter = $page->prepareContentForEdit( $content, $revision, $user );
+
                $this->assertSame( $revision->getId(), $page->getLatest() );
+
+               // Parsed output must remain cached throughout.
+               $this->assertSame( $preparedEditBefore->output, $preparedEditAfter->output );
        }
 
        /**
@@ -2311,7 +2349,7 @@ more stuff
 
                $updater1->prepareUpdate( $revision );
 
-               // Re-use updater with same revision or content
+               // Re-use updater with same revision or content, even if base changed
                $this->assertSame( $updater1, $page->getDerivedDataUpdater( $user, $revision ) );
 
                $slotsUpdate = RevisionSlotsUpdate::newFromContent(
@@ -2319,6 +2357,12 @@ more stuff
                );
                $this->assertSame( $updater1, $page->getDerivedDataUpdater( $user, null, $slotsUpdate ) );
 
+               // Don't re-use for edit if base revision ID changed
+               $this->assertNotSame(
+                       $updater1,
+                       $page->getDerivedDataUpdater( $user, null, $slotsUpdate, true )
+               );
+
                // Don't re-use with different user
                $updater2a = $page->getDerivedDataUpdater( $admin, null, $slotsUpdate );
                $updater2a->prepareContent( $admin, $slotsUpdate, false );