From: daniel Date: Sun, 8 Jul 2018 19:01:32 +0000 (+0200) Subject: Avoid losing cached ParserOutput in doEditContent. X-Git-Tag: 1.34.0-rc.0~4844^2 X-Git-Url: http://git.cyclocoop.org/%22.%20generer_url_ecrire%28%22sites%22%2C%22%22%29.%20%22?a=commitdiff_plain;h=59ea200662b4d1afe166b9c46da50ea851595416;p=lhc%2Fweb%2Fwiklou.git 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 5facc620c3..0b28074205 100644 --- 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 fa15a12c4a..aaaa73b73c 100644 --- 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 );