From: Aaron Schulz Date: Thu, 4 Jul 2019 10:01:31 +0000 (-0700) Subject: parser: add vary-revision-sha1 and related ParserOutput methods X-Git-Tag: 1.34.0-rc.0~969^2 X-Git-Url: https://git.heureux-cyclage.org/?a=commitdiff_plain;h=dd6ed7840f21cb31c0816a26cb2836a9de42d2b3;p=lhc%2Fweb%2Fwiklou.git parser: add vary-revision-sha1 and related ParserOutput methods This can be used to avoid double parsed on save if the prior output can be reused in-spite of involving a self content reference. Change-Id: Idcd30a3fa3f7012dac76ce8bbf46625453ae331f --- diff --git a/includes/Revision/RenderedRevision.php b/includes/Revision/RenderedRevision.php index 4acb9c0a64..cf1cc947a7 100644 --- a/includes/Revision/RenderedRevision.php +++ b/includes/Revision/RenderedRevision.php @@ -430,6 +430,16 @@ class RenderedRevision implements SlotRenderingProvider { "$method: Prepared output has vary-revision-exists..." ); return true; + } elseif ( + $out->getFlag( 'vary-revision-sha1' ) && + $out->getRevisionUsedSha1Base36() !== $this->revision->getSha1() + ) { + // If a self-transclusion used the proposed page text, it must match the final + // page content after PST transformations and automatically merged edit conflicts + $this->saveParseLogger->info( + "$method: Prepared output has vary-revision-sha1 with wrong SHA-1..." + ); + return true; } else { // NOTE: In the original fix for T135261, the output was discarded if 'vary-user' was // set for a null-edit. The reason was that the original rendering in that case was diff --git a/includes/Storage/PageEditStash.php b/includes/Storage/PageEditStash.php index 2285f4a953..fb19f59f19 100644 --- a/includes/Storage/PageEditStash.php +++ b/includes/Storage/PageEditStash.php @@ -269,23 +269,28 @@ class PageEditStash { if ( $editInfo->output->getFlag( 'vary-revision' ) ) { // This can be used for the initial parse, e.g. for filters or doEditContent(), - // but a second parse will be triggered in doEditUpdates(). This is not optimal. + // but a second parse will be triggered in doEditUpdates() no matter what $logger->info( - "Cache for key '{key}' has vary_revision; post-insertion parse inevitable.", - $context - ); - } elseif ( $editInfo->output->getFlag( 'vary-revision-id' ) ) { - // Similar to the above if we didn't guess the ID correctly. - $logger->debug( - "Cache for key '{key}' has vary_revision_id; post-insertion parse possible.", - $context - ); - } elseif ( $editInfo->output->getFlag( 'vary-revision-timestamp' ) ) { - // Similar to the above if we didn't guess the timestamp correctly. - $logger->debug( - "Cache for key '{key}' has vary_revision_timestamp; post-insertion parse possible.", + "Cache for key '{key}' has 'vary-revision'; post-insertion parse inevitable.", $context ); + } else { + static $flagsMaybeReparse = [ + // Similar to the above if we didn't guess the ID correctly + 'vary-revision-id', + // Similar to the above if we didn't guess the timestamp correctly + 'vary-revision-timestamp', + // Similar to the above if we didn't guess the content correctly + 'vary-revision-sha1' + ]; + foreach ( $flagsMaybeReparse as $flag ) { + if ( $editInfo->output->getFlag( $flag ) ) { + $logger->debug( + "Cache for key '{key}' has $flag; post-insertion parse possible.", + $context + ); + } + } } return $editInfo; diff --git a/includes/parser/CoreParserFunctions.php b/includes/parser/CoreParserFunctions.php index 7fece00398..5aa1a691b0 100644 --- a/includes/parser/CoreParserFunctions.php +++ b/includes/parser/CoreParserFunctions.php @@ -823,7 +823,7 @@ class CoreParserFunctions { } // fetch revision from cache/database and return the value - $rev = self::getCachedRevisionObject( $parser, $title ); + $rev = self::getCachedRevisionObject( $parser, $title, 'vary-revision-sha1' ); $length = $rev ? $rev->getSize() : 0; if ( $length === null ) { // We've had bugs where rev_len was not being recorded for empty pages, see T135414 @@ -1126,41 +1126,56 @@ class CoreParserFunctions { * * @param Parser $parser * @param Title $title + * @param string $vary ParserOuput vary-* flag * @return Revision * @since 1.23 */ - private static function getCachedRevisionObject( $parser, $title = null ) { - if ( is_null( $title ) ) { + private static function getCachedRevisionObject( $parser, $title, $vary ) { + if ( !$title ) { return null; } - // Use the revision from the parser itself, when param is the current page - // and the revision is the current one - if ( $title->equals( $parser->getTitle() ) ) { - $parserRev = $parser->getRevisionObject(); - if ( $parserRev && $parserRev->isCurrent() ) { - // force reparse after edit with vary-revision flag - $parser->getOutput()->setFlag( 'vary-revision' ); - wfDebug( __METHOD__ . ": use current revision from parser, setting vary-revision...\n" ); - return $parserRev; + $revision = null; + + $isSelfReferential = $title->equals( $parser->getTitle() ); + if ( $isSelfReferential ) { + // Revision is for the same title that is currently being parsed. Only use the last + // saved revision, regardless of Parser::getRevisionId() or fake revision injection + // callbacks against the current title. + $parserRevision = $parser->getRevisionObject(); + if ( $parserRevision && $parserRevision->isCurrent() ) { + $revision = $parserRevision; + wfDebug( __METHOD__ . ": used current revision, setting $vary" ); } } - // Normalize name for cache - $page = $title->getPrefixedDBkey(); - - if ( !( $parser->currentRevisionCache && $parser->currentRevisionCache->has( $page ) ) - && !$parser->incrementExpensiveFunctionCount() ) { - return null; + $parserOutput = $parser->getOutput(); + if ( !$revision ) { + if ( + !$parser->isCurrentRevisionOfTitleCached( $title ) && + !$parser->incrementExpensiveFunctionCount() + ) { + return null; // not allowed + } + // Get the current revision, ignoring Parser::getRevisionId() being null/old + $revision = $parser->fetchCurrentRevisionOfTitle( $title ); + // Register dependency in templatelinks + $parserOutput->addTemplate( + $title, + $revision ? $revision->getPage() : 0, + $revision ? $revision->getId() : 0 + ); } - $rev = $parser->fetchCurrentRevisionOfTitle( $title ); - $pageID = $rev ? $rev->getPage() : 0; - $revID = $rev ? $rev->getId() : 0; - // Register dependency in templatelinks - $parser->getOutput()->addTemplate( $title, $pageID, $revID ); + if ( $isSelfReferential ) { + // Upon page save, the result of the parser function using this might change + $parserOutput->setFlag( $vary ); + if ( $vary === 'vary-revision-sha1' && $revision ) { + $parserOutput->setRevisionUsedSha1Base36( $revision->getSha1() ); + } + } - return $rev; + return $revision; } /** @@ -1221,7 +1236,7 @@ class CoreParserFunctions { return ''; } // fetch revision from cache/database and return the value - $rev = self::getCachedRevisionObject( $parser, $t ); + $rev = self::getCachedRevisionObject( $parser, $t, 'vary-revision-id' ); return $rev ? $rev->getId() : ''; } @@ -1238,7 +1253,7 @@ class CoreParserFunctions { return ''; } // fetch revision from cache/database and return the value - $rev = self::getCachedRevisionObject( $parser, $t ); + $rev = self::getCachedRevisionObject( $parser, $t, 'vary-revision-timestamp' ); return $rev ? MWTimestamp::getLocalInstance( $rev->getTimestamp() )->format( 'j' ) : ''; } @@ -1255,7 +1270,7 @@ class CoreParserFunctions { return ''; } // fetch revision from cache/database and return the value - $rev = self::getCachedRevisionObject( $parser, $t ); + $rev = self::getCachedRevisionObject( $parser, $t, 'vary-revision-timestamp' ); return $rev ? MWTimestamp::getLocalInstance( $rev->getTimestamp() )->format( 'd' ) : ''; } @@ -1272,7 +1287,7 @@ class CoreParserFunctions { return ''; } // fetch revision from cache/database and return the value - $rev = self::getCachedRevisionObject( $parser, $t ); + $rev = self::getCachedRevisionObject( $parser, $t, 'vary-revision-timestamp' ); return $rev ? MWTimestamp::getLocalInstance( $rev->getTimestamp() )->format( 'm' ) : ''; } @@ -1289,7 +1304,7 @@ class CoreParserFunctions { return ''; } // fetch revision from cache/database and return the value - $rev = self::getCachedRevisionObject( $parser, $t ); + $rev = self::getCachedRevisionObject( $parser, $t, 'vary-revision-timestamp' ); return $rev ? MWTimestamp::getLocalInstance( $rev->getTimestamp() )->format( 'n' ) : ''; } @@ -1306,7 +1321,7 @@ class CoreParserFunctions { return ''; } // fetch revision from cache/database and return the value - $rev = self::getCachedRevisionObject( $parser, $t ); + $rev = self::getCachedRevisionObject( $parser, $t, 'vary-revision-timestamp' ); return $rev ? MWTimestamp::getLocalInstance( $rev->getTimestamp() )->format( 'Y' ) : ''; } @@ -1323,7 +1338,7 @@ class CoreParserFunctions { return ''; } // fetch revision from cache/database and return the value - $rev = self::getCachedRevisionObject( $parser, $t ); + $rev = self::getCachedRevisionObject( $parser, $t, 'vary-revision-timestamp' ); return $rev ? MWTimestamp::getLocalInstance( $rev->getTimestamp() )->format( 'YmdHis' ) : ''; } @@ -1340,7 +1355,7 @@ class CoreParserFunctions { return ''; } // fetch revision from cache/database and return the value - $rev = self::getCachedRevisionObject( $parser, $t ); + $rev = self::getCachedRevisionObject( $parser, $t, 'vary-user' ); return $rev ? $rev->getUserText() : ''; } diff --git a/includes/parser/Parser.php b/includes/parser/Parser.php index 4808cafe90..f0de1c9994 100644 --- a/includes/parser/Parser.php +++ b/includes/parser/Parser.php @@ -3691,6 +3691,18 @@ class Parser { return $this->currentRevisionCache->get( $cacheKey ); } + /** + * @param Title $title + * @return bool + * @since 1.34 + */ + public function isCurrentRevisionOfTitleCached( $title ) { + return ( + $this->currentRevisionCache && + $this->currentRevisionCache->has( $title->getPrefixedText() ) + ); + } + /** * Wrapper around Revision::newFromTitle to allow passing additional parameters * without passing them on to it. @@ -3725,8 +3737,7 @@ class Parser { foreach ( $stuff['deps'] as $dep ) { $this->mOutput->addTemplate( $dep['title'], $dep['page_id'], $dep['rev_id'] ); if ( $dep['title']->equals( $this->getTitle() ) ) { - // If we transclude ourselves, the final result - // will change based on the new version of the page + // Self-transclusion; final result may change based on the new page version $this->mOutput->setFlag( 'vary-revision' ); wfDebug( __METHOD__ . ": self transclusion, setting vary-revision" ); } diff --git a/includes/parser/ParserOutput.php b/includes/parser/ParserOutput.php index c8113f38be..23e5911574 100644 --- a/includes/parser/ParserOutput.php +++ b/includes/parser/ParserOutput.php @@ -216,6 +216,9 @@ class ParserOutput extends CacheTime { /** @var int|null Assumed rev timestamp for {{REVISIONTIMESTAMP}} if no revision is set */ private $revisionTimestampUsed; + /** @var string|null SHA-1 base 36 hash of any self-transclusion */ + private $revisionUsedSha1Base36; + /** string CSS classes to use for the wrapping div, stored in the array keys. * If no class is given, no wrapper is added. */ @@ -464,6 +467,33 @@ class ParserOutput extends CacheTime { return $this->revisionTimestampUsed; } + /** + * @param string $hash Lowercase SHA-1 base 36 hash + * @since 1.34 + */ + public function setRevisionUsedSha1Base36( $hash ) { + if ( $hash === null ) { + return; // e.g. RevisionRecord::getSha1() returned null + } + + if ( + $this->revisionUsedSha1Base36 !== null && + $this->revisionUsedSha1Base36 !== $hash + ) { + $this->revisionUsedSha1Base36 = ''; // mismatched + } else { + $this->revisionUsedSha1Base36 = $hash; + } + } + + /** + * @return string|null Lowercase SHA-1 base 36 hash, null if unused, or "" on inconsistency + * @since 1.34 + */ + public function getRevisionUsedSha1Base36() { + return $this->revisionUsedSha1Base36; + } + public function &getLanguageLinks() { return $this->mLanguageLinks; } diff --git a/tests/parser/ParserTestRunner.php b/tests/parser/ParserTestRunner.php index 7d46e834c1..b5c58839d8 100644 --- a/tests/parser/ParserTestRunner.php +++ b/tests/parser/ParserTestRunner.php @@ -25,6 +25,7 @@ * @file * @ingroup Testing */ + use Wikimedia\Rdbms\IDatabase; use MediaWiki\MediaWikiServices; use MediaWiki\Tidy\TidyDriverBase; @@ -129,6 +130,9 @@ class ParserTestRunner { */ private $keepUploads; + /** @var Title */ + private $defaultTitle; + /** * @param TestRecorder $recorder * @param array $options @@ -165,6 +169,8 @@ class ParserTestRunner { if ( isset( $options['upload-dir'] ) ) { $this->uploadDir = $options['upload-dir']; } + + $this->defaultTitle = Title::newFromText( 'Parser test' ); } /** @@ -840,10 +846,43 @@ class ParserTestRunner { $options->setTidy( true ); } - if ( isset( $opts['title'] ) ) { - $titleText = $opts['title']; - } else { - $titleText = 'Parser test'; + $revId = 1337; // see Parser::getRevisionId() + $title = isset( $opts['title'] ) + ? Title::newFromText( $opts['title'] ) + : $this->defaultTitle; + + if ( isset( $opts['lastsavedrevision'] ) ) { + $content = new WikitextContent( $test['input'] ); + $title = Title::newFromRow( (object)[ + 'page_id' => 187, + 'page_len' => $content->getSize(), + 'page_latest' => 1337, + 'page_namespace' => $title->getNamespace(), + 'page_title' => $title->getDBkey(), + 'page_is_redirect' => 0 + ] ); + $rev = new Revision( + [ + 'id' => $title->getLatestRevID(), + 'page' => $title->getArticleID(), + 'user' => $user, + 'content' => $content, + 'timestamp' => $this->getFakeTimestamp(), + 'title' => $title + ], + Revision::READ_LATEST, + $title + ); + $oldCallback = $options->getCurrentRevisionCallback(); + $options->setCurrentRevisionCallback( + function ( Title $t, $parser ) use ( $title, $rev, $oldCallback ) { + if ( $t->equals( $title ) ) { + return $rev; + } else { + return call_user_func( $oldCallback, $t, $parser ); + } + } + ); } if ( isset( $opts['maxincludesize'] ) ) { @@ -856,7 +895,6 @@ class ParserTestRunner { $local = isset( $opts['local'] ); $preprocessor = $opts['preprocessor'] ?? null; $parser = $this->getParser( $preprocessor ); - $title = Title::newFromText( $titleText ); if ( isset( $opts['styletag'] ) ) { // For testing the behavior of