Page MenuHomePhabricator

Revision timestamp not preserved on cached ParserOutput after cache clear
Open, Needs TriagePublicBUG REPORT

Description

Steps to replicate the issue:

Load a wiki page and note the correct "last edited" date shown at the bottom (e.g. March 10, at 07:49).
Trigger a cache invalidation — either by restarting/flushing memcache, or by loading the page with ?action=purge. Note: he page renders correctly on this first load.
Reload the page normally. The "last edited" date now incorrectly shows the time the cache was rebuilt (e.g. March 13, at 07:49) rather than the actual revision date.

What happens?
After the parser cache is rebuilt (whether from a memcache restart or an action=purge), the "last edited" date shown to users reflects the time the page was re-parsed, not the time of the actual revision. the first load is correct, but every subsequent request served from the new cache entry shows the wrong date.
What should have happened instead?
The "last edited" date should always reflect the actual revision timestamp from the database, regardless of when the parser cache was last built or rebuilt.

Software version:
MediaWiki 1.43

Other information:
Root cause is in the parser cache layer. When rebuilding the cache, setRevisionTimestamp() is only called if getRevisionTimestamp() === null on the cached ParserOutput. Because the object already carries a timestamp from the ContentParse, the condition is doesn't seem to be true at all and the $revTimestamp from the database is never applied causing the page to display the cacheTime instead of the revTime. The fix is to drop the === null guard so the DB value always wins:

Current:

if ( $parserOutput->getRevisionTimestamp() === null && $revTimestamp !== null ) {
    $parserOutput->setRevisionTimestamp( $revTimestamp );
}

Fix:

if ( $revTimestamp !== null ) {
    $parserOutput->setRevisionTimestamp( $revTimestamp );
}

Event Timeline

i'm not sure which tags should've been added so please tag the correct one if there is a better one

Change #1251219 had a related patch set uploaded (by Aklapper; author: Yvarravy):

[mediawiki/core@master] fixing where parserOuput would show cacheTime instead of revtimestamp

https://gerrit.wikimedia.org/r/1251219

is somebody able to review this?

I don't think this is the correct fix -- the "bad" revisiontimestamp shouldn't be written into the ParserOutput in the first place. It seems like you are saying that someone is writing the *parse time* into the *revision timestamp* field in the parser output, which is then yes preventing the proper revision timestamp from being written later but the real fix is to stop the initial bad write, IMO.

@cscott it seems like a piece of logic can be added inside WikitextContentHandler.php to ensure the problem doesn't happen aswell but im not sure if this is the best place maybe you have an opion about it? right now i added the following lines inside the function fillParserOutput inside the if statement at line 363
Current:

363                 if ( $parserOptions->getUseParsoid() ) {
364                         $parser = $this->parsoidParserFactory->create();
365                         // Parsoid renders the #REDIRECT magic word as an invisible
366                         // <link> tag and doesn't require it to be stripped.
367                         // T349087: ...and in fact, RESTBase relies on getting
368                         // redirect information from this <link> tag, so it needs
369                         // to be present.
370                         // Further, Parsoid can accept a Content in place of a string.
371                         $text = $content;
372                         $extraArgs = [ $cpoParams->getPreviousOutput() ];
373                 } else {

new change(line 373 to 374):

363                 if ( $parserOptions->getUseParsoid() ) {
364                         $parser = $this->parsoidParserFactory->create();
365                         // Parsoid renders the #REDIRECT magic word as an invisible
366                         // <link> tag and doesn't require it to be stripped.
367                         // T349087: ...and in fact, RESTBase relies on getting
368                         // redirect information from this <link> tag, so it needs
369                         // to be present.
370                         // Further, Parsoid can accept a Content in place of a string.
371                         $text = $content;
372                         $extraArgs = [ $cpoParams->getPreviousOutput() ];
373                         $parser->setOptions( $parserOptions);
374                         $parser->getRevisionTimestamp();
375                 } else {