From f3919c7c79b958769ad36ea6eba03ee9c8baf568 Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Tue, 23 Jul 2019 07:33:40 -0700
Subject: [PATCH] objectcache: let BagOStuff::getWithSetCallback() callbacks
 modify the TTL

Also simplify the code by removing the is_callable() check and relying on
regular PHP errors instead of an exception for bad callbacks.

Change-Id: I084b0132c5fb05f1941a6d6839cfa74e2cf677f0
---
 includes/libs/objectcache/BagOStuff.php       |  8 +++---
 .../libs/objectcache/BagOStuffTest.php        | 25 ++++++++++++++++---
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php
index 906e955464..18b952425c 100644
--- a/includes/libs/objectcache/BagOStuff.php
+++ b/includes/libs/objectcache/BagOStuff.php
@@ -115,7 +115,8 @@ abstract class BagOStuff implements IExpiringStore, IStoreKeyEncoder, LoggerAwar
 	/**
 	 * Get an item with the given key, regenerating and setting it if not found
 	 *
-	 * Nothing is stored nor deleted if the callback returns false
+	 * The callback can take $ttl as argument by reference and modify it.
+	 * Nothing is stored nor deleted if the callback returns false.
 	 *
 	 * @param string $key
 	 * @param int $ttl Time-to-live (seconds)
@@ -128,10 +129,7 @@ abstract class BagOStuff implements IExpiringStore, IStoreKeyEncoder, LoggerAwar
 		$value = $this->get( $key, $flags );
 
 		if ( $value === false ) {
-			if ( !is_callable( $callback ) ) {
-				throw new InvalidArgumentException( "Invalid cache miss callback provided." );
-			}
-			$value = call_user_func( $callback );
+			$value = $callback( $ttl );
 			if ( $value !== false ) {
 				$this->set( $key, $value, $ttl, $flags );
 			}
diff --git a/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php b/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php
index 522af43662..4a56fc580f 100644
--- a/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php
+++ b/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php
@@ -113,6 +113,9 @@ class BagOStuffTest extends MediaWikiTestCase {
 	 * @covers MediumSpecificBagOStuff::changeTTL
 	 */
 	public function testChangeTTL() {
+		$now = 1563892142;
+		$this->cache->setMockTime( $now );
+
 		$key = $this->cache->makeKey( self::TEST_KEY );
 		$value = 'meow';
 
@@ -126,7 +129,7 @@ class BagOStuffTest extends MediaWikiTestCase {
 		$this->assertFalse( $this->cache->changeTTL( $key, 15 ) );
 
 		$this->cache->add( $key, $value, 5 );
-		$this->assertTrue( $this->cache->changeTTL( $key, time() - 3600 ) );
+		$this->assertTrue( $this->cache->changeTTL( $key, $now - 3600 ) );
 		$this->assertFalse( $this->cache->get( $key ) );
 	}
 
@@ -134,6 +137,9 @@ class BagOStuffTest extends MediaWikiTestCase {
 	 * @covers MediumSpecificBagOStuff::changeTTLMulti
 	 */
 	public function testChangeTTLMulti() {
+		$now = 1563892142;
+		$this->cache->setMockTime( $now );
+
 		$key1 = $this->cache->makeKey( 'test-key1' );
 		$key2 = $this->cache->makeKey( 'test-key2' );
 		$key3 = $this->cache->makeKey( 'test-key3' );
@@ -166,7 +172,7 @@ class BagOStuffTest extends MediaWikiTestCase {
 		$this->assertEquals( 2, $this->cache->get( $key2 ) );
 		$this->assertEquals( 3, $this->cache->get( $key3 ) );
 
-		$ok = $this->cache->changeTTLMulti( [ $key1, $key2, $key3 ], time() + 86400 );
+		$ok = $this->cache->changeTTLMulti( [ $key1, $key2, $key3 ], $now + 86400 );
 		$this->assertTrue( $ok, "Expiry set for all keys" );
 
 		$ok = $this->cache->changeTTLMulti( [ $key1, $key2, $key3, $key4 ], 300 );
@@ -210,17 +216,28 @@ class BagOStuffTest extends MediaWikiTestCase {
 	 * @covers MediumSpecificBagOStuff::getWithSetCallback
 	 */
 	public function testGetWithSetCallback() {
+		$now = 1563892142;
+		$this->cache->setMockTime( $now );
 		$key = $this->cache->makeKey( self::TEST_KEY );
+
+		$this->assertFalse( $this->cache->get( $key ), "No value" );
+
 		$value = $this->cache->getWithSetCallback(
 			$key,
 			30,
-			function () {
+			function ( &$ttl ) {
+				$ttl = 10;
+
 				return 'hello kitty';
 			}
 		);
 
 		$this->assertEquals( 'hello kitty', $value );
-		$this->assertEquals( $value, $this->cache->get( $key ) );
+		$this->assertEquals( $value, $this->cache->get( $key ), "Value set" );
+
+		$now += 11;
+
+		$this->assertFalse( $this->cache->get( $key ), "Value expired" );
 	}
 
 	/**
-- 
2.20.1