ParserOutput::getCacheTime should stay the same after the first call.

Previously, getCacheTime would default to the current time, potentially
causing the return value to change over subsequent calls. With this change,
the value is determined on the first call, and then remembered for subsequent
calls.

Bug: T205464
Change-Id: If240161c71d523ad5b0d33b9378950e0bebceb6e

diff --git a/includes/parser/CacheTime.php b/includes/parser/CacheTime.php
index 26d5bdd..e9840a4 100644 (file)
--- a/includes/parser/CacheTime.php
+++ b/includes/parser/CacheTime.php
@@ -38,13 +38,13 @@ class CacheTime {
        public $mVersion = Parser::VERSION;
 
        /**
-        * @var string|int TS_MW timestamp when this object was generated, or -1 for uncacheable. Used
+        * @var string|int TS_MW timestamp when this object was generated, or -1 for not cacheable. Used
         * in ParserCache.
         */
        public $mCacheTime = '';
 
        /**
-        * @var int|null Seconds after which the object should expire, use 0 for uncacheable. Used in
+        * @var int|null Seconds after which the object should expire, use 0 for not cacheable. Used in
         * ParserCache.
         */
        public $mCacheExpiry = null;
@@ -58,7 +58,11 @@ class CacheTime {
         * @return string TS_MW timestamp
         */
        public function getCacheTime() {
-               return wfTimestamp( TS_MW, $this->mCacheTime );
+               // NOTE: keep support for undocumented used of -1 to mean "not cacheable".
+               if ( $this->mCacheTime === '' ) {
+                       $this->mCacheTime = MWTimestamp::now();
+               }
+               return $this->mCacheTime;
        }
 
        /**
@@ -68,6 +72,11 @@ class CacheTime {
         * @return string
         */
        public function setCacheTime( $t ) {
+               // NOTE: keep support for undocumented used of -1 to mean "not cacheable".
+               if ( is_string( $t ) && $t !== '-1' ) {
+                       $t = MWTimestamp::convert( TS_MW, $t );
+               }
+
                return wfSetVar( $this->mCacheTime, $t );
        }
 
@@ -120,9 +129,10 @@ class CacheTime {
        public function getCacheExpiry() {
                global $wgParserCacheExpireTime;
 
+               // NOTE: keep support for undocumented used of -1 to mean "not cacheable".
                if ( $this->mCacheTime < 0 ) {
                        return 0;
-               } // old-style marker for "not cacheable"
+               }
 
                $expire = $this->mCacheExpiry;
 
@@ -157,11 +167,12 @@ class CacheTime {
        public function expired( $touched ) {
                global $wgCacheEpoch;
 
-               return !$this->isCacheable() // parser says it's uncacheable
+               $expiry = MWTimestamp::convert( TS_MW, MWTimestamp::time() - $this->getCacheExpiry() );
+
+               return !$this->isCacheable() // parser says it's not cacheable
                        || $this->getCacheTime() < $touched
                        || $this->getCacheTime() <= $wgCacheEpoch
-                       || $this->getCacheTime() <
-                               wfTimestamp( TS_MW, time() - $this->getCacheExpiry() ) // expiry period has passed
+                       || $this->getCacheTime() < $expiry // expiry period has passed
                        || !isset( $this->mVersion )
                        || version_compare( $this->mVersion, Parser::VERSION, "lt" );
        }

diff --git a/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php b/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php
index 7931236..18f039d 100644 (file)
--- a/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php
+++ b/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php
@@ -15,6 +15,7 @@ use MediaWiki\Storage\RevisionSlotsUpdate;
 use MediaWiki\Storage\SlotRecord;
 use MediaWikiTestCase;
 use MWCallableUpdate;
+use MWTimestamp;
 use PHPUnit\Framework\MockObject\MockObject;
 use TextContent;
 use TextContentHandler;
@@ -31,6 +32,12 @@ use WikitextContent;
  */
 class DerivedPageDataUpdaterTest extends MediaWikiTestCase {
 
+       public function tearDown() {
+               MWTimestamp::setFakeTime( false );
+
+               parent::tearDown();
+       }
+
        /**
         * @param string $title
         *
@@ -470,6 +477,11 @@ class DerivedPageDataUpdaterTest extends MediaWikiTestCase {
         * @covers \MediaWiki\Storage\DerivedPageDataUpdater::getPreparedEdit()
         */
        public function testGetPreparedEditAfterPrepareUpdate() {
+               $clock = MWTimestamp::convert( TS_UNIX, '20100101000000' );
+               MWTimestamp::setFakeTime( function () use ( &$clock ) {
+                       return $clock++;
+               } );
+
                $page = $this->getPage( __METHOD__ );
 
                $mainContent = new WikitextContent( 'first [[main]] ~~~' );

diff --git a/tests/phpunit/includes/parser/ParserOutputTest.php b/tests/phpunit/includes/parser/ParserOutputTest.php
index 7091d9c..94cbf5c 100644 (file)
--- a/tests/phpunit/includes/parser/ParserOutputTest.php
+++ b/tests/phpunit/includes/parser/ParserOutputTest.php
@@ -32,6 +32,12 @@ class ParserOutputTest extends MediaWikiLangTestCase {
                ];
        }
 
+       public function tearDown() {
+               MWTimestamp::setFakeTime( false );
+
+               parent::tearDown();
+       }
+
        /**
         * Test to make sure ParserOutput::isLinkInternal behaves properly
         * @dataProvider provideIsLinkInternal
@@ -935,4 +941,33 @@ EOF
                }
        }
 
+       /**
+        * @covers ParserOutput::getCacheTime
+        * @covers ParserOutput::setCacheTime
+        */
+       public function testGetCacheTime() {
+               $clock = MWTimestamp::convert( TS_UNIX, '20100101000000' );
+               MWTimestamp::setFakeTime( function () use ( &$clock ) {
+                       return $clock++;
+               } );
+
+               $po = new ParserOutput();
+               $time = $po->getCacheTime();
+
+               // Use current (fake) time per default. Ignore the last digit.
+               // Subsequent calls must yield the exact same timestamp as the first.
+               $this->assertStringStartsWith( '2010010100000', $time );
+               $this->assertSame( $time, $po->getCacheTime() );
+
+               // After setting, the getter must return the time that was set.
+               $time = '20110606112233';
+               $po->setCacheTime( $time );
+               $this->assertSame( $time, $po->getCacheTime() );
+
+               // support -1 as a marker for "not cacheable"
+               $time = -1;
+               $po->setCacheTime( $time );
+               $this->assertSame( $time, $po->getCacheTime() );
+       }
+
 }