From e85fe191c91a282fa1d466997a346782644a8870 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Thu, 18 Apr 2019 18:36:40 -0700 Subject: [PATCH] parser: inject the time for {{REVISIONTIMESTAMP}} on pre-save parse DerivedPageDataUpdater::prepareContent already locks in the revision timestamp before insertion, so inject that into the parser options used for any pre-save parse (e.g for edit filters). This means that a reparse is no longer needed within in the same save request to get the post-save canonical output. A parse will still be required if the edit filter output used an edit stash output, since the revision timestamp is not set at stash time. Instead of using vary-revision, add a vary-revision-timestamp flag for the revision timestamp words. The month/day/hour variants retain their prior optimizations for allowing edit stash output reuse for the post-save canonical output. Change-Id: Ic2c13db4d21197c79a89de0de56745ca32918eb6 --- includes/GlobalFunctions.php | 3 +- includes/Revision/RenderedRevision.php | 55 +++++++++++++----- includes/Revision/RevisionRenderer.php | 7 +++ includes/Storage/DerivedPageDataUpdater.php | 18 +++++- includes/Storage/PageEditStash.php | 6 ++ includes/page/WikiPage.php | 1 + includes/parser/Parser.php | 62 +++++++++++---------- includes/parser/ParserOptions.php | 2 +- includes/parser/ParserOutput.php | 19 +++++++ 9 files changed, 126 insertions(+), 47 deletions(-) diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php index 7256eab2fb..16c63dade7 100644 --- a/includes/GlobalFunctions.php +++ b/includes/GlobalFunctions.php @@ -1882,10 +1882,9 @@ function wfTimestampOrNull( $outputtype = TS_UNIX, $ts = null ) { /** * Convenience function; returns MediaWiki timestamp for the present time. * - * @return string + * @return string TS_MW timestamp */ function wfTimestampNow() { - # return NOW return MWTimestamp::now( TS_MW ); } diff --git a/includes/Revision/RenderedRevision.php b/includes/Revision/RenderedRevision.php index c4a00545c4..4acb9c0a64 100644 --- a/includes/Revision/RenderedRevision.php +++ b/includes/Revision/RenderedRevision.php @@ -291,19 +291,28 @@ class RenderedRevision implements SlotRenderingProvider { $this->setRevisionInternal( $rev ); - $this->pruneRevisionSensitiveOutput( $this->revision->getId() ); + $this->pruneRevisionSensitiveOutput( + $this->revision->getId(), + $this->revision->getTimestamp() + ); } /** * Prune any output that depends on the revision ID. * - * @param int|bool $actualRevId The actual rev id, to check the used speculative rev ID + * @param int|bool $actualRevId The actual rev id, to check the used speculative rev ID * against, or false to not purge on vary-revision-id, or true to purge on * vary-revision-id unconditionally. + * @param string|bool $actualRevTimestamp The actual rev timestamp, to check against the + * parser output revision timestamp, or false to not purge on vary-revision-timestamp */ - private function pruneRevisionSensitiveOutput( $actualRevId ) { + private function pruneRevisionSensitiveOutput( $actualRevId, $actualRevTimestamp ) { if ( $this->revisionOutput ) { - if ( $this->outputVariesOnRevisionMetaData( $this->revisionOutput, $actualRevId ) ) { + if ( $this->outputVariesOnRevisionMetaData( + $this->revisionOutput, + $actualRevId, + $actualRevTimestamp + ) ) { $this->revisionOutput = null; } } else { @@ -311,7 +320,11 @@ class RenderedRevision implements SlotRenderingProvider { } foreach ( $this->slotsOutput as $role => $output ) { - if ( $this->outputVariesOnRevisionMetaData( $output, $actualRevId ) ) { + if ( $this->outputVariesOnRevisionMetaData( + $output, + $actualRevId, + $actualRevTimestamp + ) ) { unset( $this->slotsOutput[$role] ); } } @@ -372,19 +385,24 @@ class RenderedRevision implements SlotRenderingProvider { /** * @param ParserOutput $out * @param int|bool $actualRevId The actual rev id, to check the used speculative rev ID - * against, or false to not purge on vary-revision-id, or true to purge on + * against, false to not purge on vary-revision-id, or true to purge on * vary-revision-id unconditionally. + * @param string|bool $actualRevTimestamp The actual rev timestamp, to check against the + * parser output revision timestamp, false to not purge on vary-revision-timestamp, + * or true to purge on vary-revision-timestamp unconditionally. * @return bool */ - private function outputVariesOnRevisionMetaData( ParserOutput $out, $actualRevId ) { + private function outputVariesOnRevisionMetaData( + ParserOutput $out, + $actualRevId, + $actualRevTimestamp + ) { $method = __METHOD__; if ( $out->getFlag( 'vary-revision' ) ) { - // If {{PAGEID}} resolved to 0 or {{REVISIONTIMESTAMP}} used the current - // timestamp rather than that of an actual revision, then those words need - // to resolve to the actual page ID or revision timestamp, respectively. + // If {{PAGEID}} resolved to 0, then that word need to resolve to the actual page ID $this->saveParseLogger->info( - "$method: Prepared output has vary-revision...\n" + "$method: Prepared output has vary-revision..." ); return true; } elseif ( $out->getFlag( 'vary-revision-id' ) @@ -392,7 +410,16 @@ class RenderedRevision implements SlotRenderingProvider { && ( $actualRevId === true || $out->getSpeculativeRevIdUsed() !== $actualRevId ) ) { $this->saveParseLogger->info( - "$method: Prepared output has vary-revision-id with wrong ID...\n" + "$method: Prepared output has vary-revision-id with wrong ID..." + ); + return true; + } elseif ( $out->getFlag( 'vary-revision-timestamp' ) + && $actualRevTimestamp !== false + && ( $actualRevTimestamp === true || + $out->getRevisionTimestampUsed() !== $actualRevTimestamp ) + ) { + $this->saveParseLogger->info( + "$method: Prepared output has vary-revision-timestamp with wrong timestamp..." ); return true; } elseif ( $out->getFlag( 'vary-revision-exists' ) ) { @@ -400,7 +427,7 @@ class RenderedRevision implements SlotRenderingProvider { // Note that edit stashing always uses '-', which can be used for both // edit filter checks and canonical parser cache. $this->saveParseLogger->info( - "$method: Prepared output has vary-revision-exists...\n" + "$method: Prepared output has vary-revision-exists..." ); return true; } else { @@ -412,7 +439,7 @@ class RenderedRevision implements SlotRenderingProvider { // constructs the ParserOptions: For a null-edit, setCurrentRevisionCallback is called // with the old, existing revision. - wfDebug( "$method: Keeping prepared output...\n" ); + $this->saveParseLogger->debug( "$method: Keeping prepared output..." ); return false; } } diff --git a/includes/Revision/RevisionRenderer.php b/includes/Revision/RevisionRenderer.php index f97390ad49..a63e4f1e9a 100644 --- a/includes/Revision/RevisionRenderer.php +++ b/includes/Revision/RevisionRenderer.php @@ -132,6 +132,13 @@ class RevisionRenderer { return $this->getSpeculativeRevId( $dbIndex ); } ); + if ( !$rev->getId() && $rev->getTimestamp() ) { + // This is an unsaved revision with an already determined timestamp. + // Make the "current" time used during parsing match that of the revision. + // Any REVISION* parser variables will match up if the revision is saved. + $options->setTimestamp( $rev->getTimestamp() ); + } + $title = Title::newFromLinkTarget( $rev->getPageAsLinkTarget() ); $renderedRevision = new RenderedRevision( diff --git a/includes/Storage/DerivedPageDataUpdater.php b/includes/Storage/DerivedPageDataUpdater.php index ff5541d800..5f7401ade8 100644 --- a/includes/Storage/DerivedPageDataUpdater.php +++ b/includes/Storage/DerivedPageDataUpdater.php @@ -51,6 +51,9 @@ use MessageCache; use ParserCache; use ParserOptions; use ParserOutput; +use Psr\Log\LoggerAwareInterface; +use Psr\Log\LoggerInterface; +use Psr\Log\NullLogger; use RecentChangesUpdateJob; use ResourceLoaderWikiModule; use Revision; @@ -93,7 +96,7 @@ use WikiPage; * @since 1.32 * @ingroup Page */ -class DerivedPageDataUpdater implements IDBAccessObject { +class DerivedPageDataUpdater implements IDBAccessObject, LoggerAwareInterface { /** * @var UserIdentity|null @@ -135,6 +138,11 @@ class DerivedPageDataUpdater implements IDBAccessObject { */ private $loadbalancerFactory; + /** + * @var LoggerInterface + */ + private $logger; + /** * @var string see $wgArticleCountMethod */ @@ -292,6 +300,11 @@ class DerivedPageDataUpdater implements IDBAccessObject { // XXX only needed for waiting for replicas to catch up; there should be a narrower // interface for that. $this->loadbalancerFactory = $loadbalancerFactory; + $this->logger = new NullLogger(); + } + + public function setLogger( LoggerInterface $logger ) { + $this->logger = $logger; } /** @@ -849,11 +862,12 @@ class DerivedPageDataUpdater implements IDBAccessObject { if ( $stashedEdit ) { /** @var ParserOutput $output */ $output = $stashedEdit->output; - // TODO: this should happen when stashing the ParserOutput, not now! $output->setCacheTime( $stashedEdit->timestamp ); $renderHints['known-revision-output'] = $output; + + $this->logger->debug( __METHOD__ . ': using stashed edit output...' ); } // NOTE: we want a canonical rendering, so don't pass $this->user or ParserOptions diff --git a/includes/Storage/PageEditStash.php b/includes/Storage/PageEditStash.php index 46f957fded..bda4286932 100644 --- a/includes/Storage/PageEditStash.php +++ b/includes/Storage/PageEditStash.php @@ -280,6 +280,12 @@ class PageEditStash { "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.", + $context + ); } return $editInfo; diff --git a/includes/page/WikiPage.php b/includes/page/WikiPage.php index 863a3f307e..ee6adf1a28 100644 --- a/includes/page/WikiPage.php +++ b/includes/page/WikiPage.php @@ -1697,6 +1697,7 @@ class WikiPage implements Page, IDBAccessObject { MediaWikiServices::getInstance()->getDBLoadBalancerFactory() ); + $derivedDataUpdater->setLogger( LoggerFactory::getInstance( 'SaveParse' ) ); $derivedDataUpdater->setRcWatchCategoryMembership( $wgRCWatchCategoryMembership ); $derivedDataUpdater->setArticleCountMethod( $wgArticleCountMethod ); diff --git a/includes/parser/Parser.php b/includes/parser/Parser.php index 27c34a6dd3..04c9c9a649 100644 --- a/includes/parser/Parser.php +++ b/includes/parser/Parser.php @@ -2785,7 +2785,7 @@ class Parser { # The vary-revision flag must be set, because the magic word # will have a different value once the page is saved. $this->mOutput->setFlag( 'vary-revision' ); - wfDebug( __METHOD__ . ": {{PAGEID}} used in a new page, setting vary-revision...\n" ); + wfDebug( __METHOD__ . ": {{PAGEID}} used in a new page, setting vary-revision" ); } $value = $pageid ?: null; break; @@ -2802,13 +2802,14 @@ class Parser { $value = '-'; } else { $this->mOutput->setFlag( 'vary-revision-exists' ); + wfDebug( __METHOD__ . ": {{REVISIONID}} used, setting vary-revision-exists" ); $value = ''; } } else { # Inform the edit saving system that getting the canonical output after # revision insertion requires another parse using the actual revision ID $this->mOutput->setFlag( 'vary-revision-id' ); - wfDebug( __METHOD__ . ": {{REVISIONID}} used, setting vary-revision-id...\n" ); + wfDebug( __METHOD__ . ": {{REVISIONID}} used, setting vary-revision-id" ); $value = $this->getRevisionId(); if ( $value === 0 ) { $rev = $this->getRevisionObject(); @@ -2838,17 +2839,13 @@ class Parser { $value = $this->getRevisionTimestampSubstring( 0, 4, self::MAX_TTS, $index ); break; case 'revisiontimestamp': - # Let the edit saving system know we should parse the page - # *after* a revision ID has been assigned. This is for null edits. - $this->mOutput->setFlag( 'vary-revision' ); - wfDebug( __METHOD__ . ": {{REVISIONTIMESTAMP}} used, setting vary-revision...\n" ); - $value = $this->getRevisionTimestamp(); + $value = $this->getRevisionTimestampSubstring( 0, 14, self::MAX_TTS, $index ); break; case 'revisionuser': - # Let the edit saving system know we should parse the page - # *after* a revision ID has been assigned for null edits. + # Inform the edit saving system that getting the canonical output after + # revision insertion requires a parse that used the actual user ID $this->mOutput->setFlag( 'vary-user' ); - wfDebug( __METHOD__ . ": {{REVISIONUSER}} used, setting vary-user...\n" ); + wfDebug( __METHOD__ . ": {{REVISIONUSER}} used, setting vary-user" ); $value = $this->getRevisionUser(); break; case 'revisionsize': @@ -2996,7 +2993,7 @@ class Parser { /** * @param int $start * @param int $len - * @param int $mtts Max time-till-save; sets vary-revision if result might change by then + * @param int $mtts Max time-till-save; sets vary-revision-timestamp if result changes by then * @param string $variable Parser variable name * @return string */ @@ -3005,7 +3002,10 @@ class Parser { $resNow = substr( $this->getRevisionTimestamp(), $start, $len ); # Possibly set vary-revision if there is not yet an associated revision if ( !$this->getRevisionObject() ) { - # Get the timezone-adjusted timestamp $mtts seconds in the future + # Get the timezone-adjusted timestamp $mtts seconds in the future. + # This future is relative to the current time and not that of the + # parser options. The rendered timestamp can be compared to that + # of the timestamp specified by the parser options. $resThen = substr( $this->contLang->userAdjust( wfTimestamp( TS_MW, time() + $mtts ), '' ), $start, @@ -3013,10 +3013,10 @@ class Parser { ); if ( $resNow !== $resThen ) { - # Let the edit saving system know we should parse the page - # *after* a revision ID has been assigned. This is for null edits. - $this->mOutput->setFlag( 'vary-revision' ); - wfDebug( __METHOD__ . ": $variable used, setting vary-revision...\n" ); + # Inform the edit saving system that getting the canonical output after + # revision insertion requires a parse that used an actual revision timestamp + $this->mOutput->setFlag( 'vary-revision-timestamp' ); + wfDebug( __METHOD__ . ": $variable used, setting vary-revision-timestamp" ); } } @@ -3738,6 +3738,7 @@ class Parser { // If we transclude ourselves, the final result // will change based on the new version of the page $this->mOutput->setFlag( 'vary-revision' ); + wfDebug( __METHOD__ . ": self transclusion, setting vary-revision" ); } } } @@ -5915,7 +5916,7 @@ class Parser { * * The return value will be either: * - a) Positive, indicating a specific revision ID (current or old) - * - b) Zero, meaning the revision ID specified by getCurrentRevisionCallback() + * - b) Zero, meaning the revision ID is specified by getCurrentRevisionCallback() * - c) Null, meaning the parse is for preview mode and there is no revision * * @return int|null @@ -5970,20 +5971,25 @@ class Parser { /** * Get the timestamp associated with the current revision, adjusted for * the default server-local timestamp - * @return string + * @return string TS_MW timestamp */ public function getRevisionTimestamp() { - if ( is_null( $this->mRevisionTimestamp ) ) { - $revObject = $this->getRevisionObject(); - $timestamp = $revObject ? $revObject->getTimestamp() : wfTimestampNow(); - - # The cryptic '' timezone parameter tells to use the site-default - # timezone offset instead of the user settings. - # Since this value will be saved into the parser cache, served - # to other users, and potentially even used inside links and such, - # it needs to be consistent for all visitors. - $this->mRevisionTimestamp = $this->contLang->userAdjust( $timestamp, '' ); + if ( $this->mRevisionTimestamp !== null ) { + return $this->mRevisionTimestamp; } + + # Use specified revision timestamp, falling back to the current timestamp + $revObject = $this->getRevisionObject(); + $timestamp = $revObject ? $revObject->getTimestamp() : $this->mOptions->getTimestamp(); + $this->mOutput->setRevisionTimestampUsed( $timestamp ); // unadjusted time zone + + # The cryptic '' timezone parameter tells to use the site-default + # timezone offset instead of the user settings. + # Since this value will be saved into the parser cache, served + # to other users, and potentially even used inside links and such, + # it needs to be consistent for all visitors. + $this->mRevisionTimestamp = $this->contLang->userAdjust( $timestamp, '' ); + return $this->mRevisionTimestamp; } diff --git a/includes/parser/ParserOptions.php b/includes/parser/ParserOptions.php index 66b1612245..2954b13b5c 100644 --- a/includes/parser/ParserOptions.php +++ b/includes/parser/ParserOptions.php @@ -895,7 +895,7 @@ class ParserOptions { /** * Timestamp used for {{CURRENTDAY}} etc. - * @return string + * @return string TS_MW timestamp */ public function getTimestamp() { if ( !isset( $this->mTimestamp ) ) { diff --git a/includes/parser/ParserOutput.php b/includes/parser/ParserOutput.php index ef22a1f235..ccc6b37e05 100644 --- a/includes/parser/ParserOutput.php +++ b/includes/parser/ParserOutput.php @@ -207,6 +207,9 @@ class ParserOutput extends CacheTime { /** @var int|null Assumed rev ID for {{REVISIONID}} if no revision is set */ private $mSpeculativeRevId; + /** @var int|null Assumed rev timestamp for {{REVISIONTIMESTAMP}} if no revision is set */ + private $revisionTimestampUsed; + /** string CSS classes to use for the wrapping div, stored in the array keys. * If no class is given, no wrapper is added. */ @@ -439,6 +442,22 @@ class ParserOutput extends CacheTime { return $this->mSpeculativeRevId; } + /** + * @param string $timestamp TS_MW timestamp + * @since 1.34 + */ + public function setRevisionTimestampUsed( $timestamp ) { + $this->revisionTimestampUsed = $timestamp; + } + + /** + * @return string|null TS_MW timestamp or null if not used + * @since 1.34 + */ + public function getRevisionTimestampUsed() { + return $this->revisionTimestampUsed; + } + public function &getLanguageLinks() { return $this->mLanguageLinks; } -- 2.20.1