objectcache: merge getWithToken() into doGet() for simplicity

Change-Id: I581f866521e1086ca350973d9cdeff6656f48fe8

diff --git a/includes/libs/objectcache/APCBagOStuff.php b/includes/libs/objectcache/APCBagOStuff.php
index 0d4ca11..902cd6a 100644 (file)
--- a/includes/libs/objectcache/APCBagOStuff.php
+++ b/includes/libs/objectcache/APCBagOStuff.php
@@ -72,11 +72,7 @@ class APCBagOStuff extends BagOStuff {
                }
        }
 
-       protected function doGet( $key, $flags = 0 ) {
-               return $this->unserialize( apc_fetch( $key . self::KEY_SUFFIX ) );
-       }
-
-       protected function getWithToken( $key, &$casToken, $flags = 0 ) {
+       protected function doGet( $key, $flags = 0, &$casToken = null ) {
                $casToken = null;
 
                $blob = apc_fetch( $key . self::KEY_SUFFIX );

diff --git a/includes/libs/objectcache/APCUBagOStuff.php b/includes/libs/objectcache/APCUBagOStuff.php
index c36a1dc..da6544b 100644 (file)
--- a/includes/libs/objectcache/APCUBagOStuff.php
+++ b/includes/libs/objectcache/APCUBagOStuff.php
@@ -39,11 +39,7 @@ class APCUBagOStuff extends APCBagOStuff {
                parent::__construct( $params );
        }
 
-       protected function doGet( $key, $flags = 0 ) {
-               return $this->unserialize( apcu_fetch( $key . self::KEY_SUFFIX ) );
-       }
-
-       protected function getWithToken( $key, &$casToken, $flags = 0 ) {
+       protected function doGet( $key, $flags = 0, &$casToken = null ) {
                $casToken = null;
 
                $blob = apcu_fetch( $key . self::KEY_SUFFIX );

diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php
index bdfed82..5669366 100644 (file)
--- a/includes/libs/objectcache/BagOStuff.php
+++ b/includes/libs/objectcache/BagOStuff.php
@@ -175,13 +175,9 @@ abstract class BagOStuff implements IExpiringStore, LoggerAwareInterface {
         *
         * @param string $key
         * @param int $flags Bitfield of BagOStuff::READ_* constants [optional]
-        * @param int|null $oldFlags [unused]
         * @return mixed Returns false on failure or if the item does not exist
         */
-       public function get( $key, $flags = 0, $oldFlags = null ) {
-               // B/C for ( $key, &$casToken = null, $flags = 0 )
-               $flags = is_int( $oldFlags ) ? $oldFlags : $flags;
-
+       public function get( $key, $flags = 0 ) {
                $this->trackDuplicateKeys( $key );
 
                return $this->doGet( $key, $flags );
@@ -223,22 +219,10 @@ abstract class BagOStuff implements IExpiringStore, LoggerAwareInterface {
        /**
         * @param string $key
         * @param int $flags Bitfield of BagOStuff::READ_* constants [optional]
+        * @param mixed|null &$casToken Token to use for check-and-set comparisons
         * @return mixed Returns false on failure or if the item does not exist
         */
-       abstract protected function doGet( $key, $flags = 0 );
-
-       /**
-        * @note This method is only needed if merge() uses mergeViaCas()
-        *
-        * @param string $key
-        * @param mixed &$casToken
-        * @param int $flags Bitfield of BagOStuff::READ_* constants [optional]
-        * @return mixed Returns false on failure or if the item does not exist
-        * @throws Exception
-        */
-       protected function getWithToken( $key, &$casToken, $flags = 0 ) {
-               throw new Exception( __METHOD__ . ' not implemented.' );
-       }
+       abstract protected function doGet( $key, $flags = 0, &$casToken = null );
 
        /**
         * Set an item
@@ -308,7 +292,7 @@ abstract class BagOStuff implements IExpiringStore, LoggerAwareInterface {
                        $reportDupes = $this->reportDupes;
                        $this->reportDupes = false;
                        $casToken = null; // passed by reference
-                       $currentValue = $this->getWithToken( $key, $casToken, self::READ_LATEST );
+                       $currentValue = $this->doGet( $key, self::READ_LATEST, $casToken );
                        $this->reportDupes = $reportDupes;
 
                        if ( $this->getLastError() ) {
@@ -365,7 +349,7 @@ abstract class BagOStuff implements IExpiringStore, LoggerAwareInterface {
                }
 
                $curCasToken = null; // passed by reference
-               $this->getWithToken( $key, $curCasToken, self::READ_LATEST );
+               $this->doGet( $key, self::READ_LATEST, $curCasToken );
                if ( $casToken === $curCasToken ) {
                        $success = $this->set( $key, $value, $exptime, $flags );
                } else {

diff --git a/includes/libs/objectcache/CachedBagOStuff.php b/includes/libs/objectcache/CachedBagOStuff.php
index 95dda71..8892f73 100644 (file)
--- a/includes/libs/objectcache/CachedBagOStuff.php
+++ b/includes/libs/objectcache/CachedBagOStuff.php
@@ -32,6 +32,7 @@
  *   up going to the HashBagOStuff used for the in-memory cache).
  *
  * @ingroup Cache
+ * @TODO: Make this class use composition instead of calling super
  */
 class CachedBagOStuff extends HashBagOStuff {
        /** @var BagOStuff */
@@ -50,10 +51,10 @@ class CachedBagOStuff extends HashBagOStuff {
                $this->attrMap = $backend->attrMap;
        }
 
-       protected function doGet( $key, $flags = 0 ) {
-               $ret = parent::doGet( $key, $flags );
+       public function get( $key, $flags = 0 ) {
+               $ret = parent::get( $key, $flags );
                if ( $ret === false && !$this->hasKey( $key ) ) {
-                       $ret = $this->backend->doGet( $key, $flags );
+                       $ret = $this->backend->get( $key, $flags );
                        $this->set( $key, $ret, 0, self::WRITE_CACHE_ONLY );
                }
                return $ret;

diff --git a/includes/libs/objectcache/EmptyBagOStuff.php b/includes/libs/objectcache/EmptyBagOStuff.php
index e0f4364..ffe3a4c 100644 (file)
--- a/includes/libs/objectcache/EmptyBagOStuff.php
+++ b/includes/libs/objectcache/EmptyBagOStuff.php
@@ -27,7 +27,9 @@
  * @ingroup Cache
  */
 class EmptyBagOStuff extends BagOStuff {
-       protected function doGet( $key, $flags = 0 ) {
+       protected function doGet( $key, $flags = 0, &$casToken = null ) {
+               $casToken = null;
+
                return false;
        }
 

diff --git a/includes/libs/objectcache/HashBagOStuff.php b/includes/libs/objectcache/HashBagOStuff.php
index d1f312a..d24f408 100644 (file)
--- a/includes/libs/objectcache/HashBagOStuff.php
+++ b/includes/libs/objectcache/HashBagOStuff.php
@@ -58,12 +58,10 @@ class HashBagOStuff extends BagOStuff {
                }
        }
 
-       protected function doGet( $key, $flags = 0 ) {
-               if ( !$this->hasKey( $key ) ) {
-                       return false;
-               }
+       protected function doGet( $key, $flags = 0, &$casToken = null ) {
+               $casToken = null;
 
-               if ( $this->expire( $key ) ) {
+               if ( !$this->hasKey( $key ) || $this->expire( $key ) ) {
                        return false;
                }
 
@@ -72,18 +70,9 @@ class HashBagOStuff extends BagOStuff {
                unset( $this->bag[$key] );
                $this->bag[$key] = $temp;
 
-               return $this->bag[$key][self::KEY_VAL];
-       }
-
-       protected function getWithToken( $key, &$casToken, $flags = 0 ) {
-               $casToken = null;
+               $casToken = $this->bag[$key][self::KEY_CAS];
 
-               $value = $this->doGet( $key );
-               if ( $value !== false ) {
-                       $casToken = $this->bag[$key][self::KEY_CAS];
-               }
-
-               return $value;
+               return $this->bag[$key][self::KEY_VAL];
        }
 
        public function set( $key, $value, $exptime = 0, $flags = 0 ) {

diff --git a/includes/libs/objectcache/MemcachedBagOStuff.php b/includes/libs/objectcache/MemcachedBagOStuff.php
index b0fbece..71e3331 100644 (file)
--- a/includes/libs/objectcache/MemcachedBagOStuff.php
+++ b/includes/libs/objectcache/MemcachedBagOStuff.php
@@ -50,13 +50,7 @@ class MemcachedBagOStuff extends BagOStuff {
                ];
        }
 
-       protected function doGet( $key, $flags = 0 ) {
-               $casToken = null;
-
-               return $this->getWithToken( $key, $casToken, $flags );
-       }
-
-       protected function getWithToken( $key, &$casToken, $flags = 0 ) {
+       protected function doGet( $key, $flags = 0, &$casToken = null ) {
                return $this->client->get( $this->validateKeyEncoding( $key ), $casToken );
        }
 

diff --git a/includes/libs/objectcache/MemcachedPeclBagOStuff.php b/includes/libs/objectcache/MemcachedPeclBagOStuff.php
index 62a57b6..489f001 100644 (file)
--- a/includes/libs/objectcache/MemcachedPeclBagOStuff.php
+++ b/includes/libs/objectcache/MemcachedPeclBagOStuff.php
@@ -138,7 +138,7 @@ class MemcachedPeclBagOStuff extends MemcachedBagOStuff {
                return $params;
        }
 
-       protected function getWithToken( $key, &$casToken, $flags = 0 ) {
+       protected function doGet( $key, $flags = 0, &$casToken = null ) {
                $this->debugLog( "get($key)" );
                if ( defined( Memcached::class . '::GET_EXTENDED' ) ) { // v3.0.0
                        $flags = Memcached::GET_EXTENDED;

diff --git a/includes/libs/objectcache/MultiWriteBagOStuff.php b/includes/libs/objectcache/MultiWriteBagOStuff.php
index fb99717..adb9bb8 100644 (file)
--- a/includes/libs/objectcache/MultiWriteBagOStuff.php
+++ b/includes/libs/objectcache/MultiWriteBagOStuff.php
@@ -103,7 +103,7 @@ class MultiWriteBagOStuff extends BagOStuff {
                }
        }
 
-       protected function doGet( $key, $flags = 0 ) {
+       public function get( $key, $flags = 0 ) {
                if ( ( $flags & self::READ_LATEST ) == self::READ_LATEST ) {
                        // If the latest write was a delete(), we do NOT want to fallback
                        // to the other tiers and possibly see the old value. Also, this
@@ -349,4 +349,8 @@ class MultiWriteBagOStuff extends BagOStuff {
        public function makeGlobalKey( $class, $component = null ) {
                return $this->caches[0]->makeGlobalKey( ...func_get_args() );
        }
+
+       protected function doGet( $key, $flags = 0, &$casToken = null ) {
+               throw new LogicException( __METHOD__ . ': proxy class does not need this method.' );
+       }
 }

diff --git a/includes/libs/objectcache/RESTBagOStuff.php b/includes/libs/objectcache/RESTBagOStuff.php
index 3303692..4d8ae59 100644 (file)
--- a/includes/libs/objectcache/RESTBagOStuff.php
+++ b/includes/libs/objectcache/RESTBagOStuff.php
@@ -84,13 +84,7 @@ class RESTBagOStuff extends BagOStuff {
                $this->client->setLogger( $logger );
        }
 
-       protected function doGet( $key, $flags = 0 ) {
-               $casToken = null;
-
-               return $this->getWithToken( $key, $casToken, $flags );
-       }
-
-       protected function getWithToken( $key, &$casToken, $flags = 0 ) {
+       protected function doGet( $key, $flags = 0, &$casToken = null ) {
                $casToken = null;
 
                $req = [

diff --git a/includes/libs/objectcache/RedisBagOStuff.php b/includes/libs/objectcache/RedisBagOStuff.php
index af90c5c..79859db 100644 (file)
--- a/includes/libs/objectcache/RedisBagOStuff.php
+++ b/includes/libs/objectcache/RedisBagOStuff.php
@@ -86,13 +86,9 @@ class RedisBagOStuff extends BagOStuff {
                $this->attrMap[self::ATTR_SYNCWRITES] = self::QOS_SYNCWRITES_NONE;
        }
 
-       protected function doGet( $key, $flags = 0 ) {
+       protected function doGet( $key, $flags = 0, &$casToken = null ) {
                $casToken = null;
 
-               return $this->getWithToken( $key, $casToken, $flags );
-       }
-
-       protected function getWithToken( $key, &$casToken, $flags = 0 ) {
                list( $server, $conn ) = $this->getConnection( $key );
                if ( !$conn ) {
                        return false;

diff --git a/includes/libs/objectcache/ReplicatedBagOStuff.php b/includes/libs/objectcache/ReplicatedBagOStuff.php
index eb3f6f8..70f9096 100644 (file)
--- a/includes/libs/objectcache/ReplicatedBagOStuff.php
+++ b/includes/libs/objectcache/ReplicatedBagOStuff.php
@@ -74,7 +74,7 @@ class ReplicatedBagOStuff extends BagOStuff {
                $this->readStore->setDebug( $debug );
        }
 
-       protected function doGet( $key, $flags = 0 ) {
+       public function get( $key, $flags = 0 ) {
                return ( $flags & self::READ_LATEST )
                        ? $this->writeStore->get( $key, $flags )
                        : $this->readStore->get( $key, $flags );
@@ -160,4 +160,8 @@ class ReplicatedBagOStuff extends BagOStuff {
        public function makeGlobalKey( $class, $component = null ) {
                return $this->writeStore->makeGlobalKey( ...func_get_args() );
        }
+
+       protected function doGet( $key, $flags = 0, &$casToken = null ) {
+               throw new LogicException( __METHOD__ . ': proxy class does not need this method.' );
+       }
 }

diff --git a/includes/libs/objectcache/WinCacheBagOStuff.php b/includes/libs/objectcache/WinCacheBagOStuff.php
index fb64d77..818f6f1 100644 (file)
--- a/includes/libs/objectcache/WinCacheBagOStuff.php
+++ b/includes/libs/objectcache/WinCacheBagOStuff.php
@@ -28,13 +28,7 @@
  * @ingroup Cache
  */
 class WinCacheBagOStuff extends BagOStuff {
-       protected function doGet( $key, $flags = 0 ) {
-               $blob = wincache_ucache_get( $key );
-
-               return is_string( $blob ) ? unserialize( $blob ) : false;
-       }
-
-       protected function getWithToken( $key, &$casToken, $flags = 0 ) {
+       protected function doGet( $key, $flags = 0, &$casToken = null ) {
                $casToken = null;
 
                $blob = wincache_ucache_get( $key );
@@ -43,12 +37,10 @@ class WinCacheBagOStuff extends BagOStuff {
                }
 
                $value = unserialize( $blob );
-               if ( $value === false ) {
-                       return false;
+               if ( $value !== false ) {
+                       $casToken = (string)$blob; // don't bother hashing this
                }
 
-               $casToken = $blob; // don't bother hashing this
-
                return $value;
        }
 
@@ -58,7 +50,7 @@ class WinCacheBagOStuff extends BagOStuff {
                }
 
                $curCasToken = null; // passed by reference
-               $this->getWithToken( $key, $curCasToken, self::READ_LATEST );
+               $this->doGet( $key, self::READ_LATEST, $curCasToken );
                if ( $casToken === $curCasToken ) {
                        $success = $this->set( $key, $value, $exptime, $flags );
                } else {

diff --git a/includes/objectcache/SqlBagOStuff.php b/includes/objectcache/SqlBagOStuff.php
index 4fa9999..088f94e 100644 (file)
--- a/includes/objectcache/SqlBagOStuff.php
+++ b/includes/objectcache/SqlBagOStuff.php
@@ -234,22 +234,34 @@ class SqlBagOStuff extends BagOStuff {
                }
        }
 
-       protected function doGet( $key, $flags = 0 ) {
+       protected function doGet( $key, $flags = 0, &$casToken = null ) {
                $casToken = null;
 
-               return $this->getWithToken( $key, $casToken, $flags );
-       }
+               $blobs = $this->fetchBlobMulti( [ $key ] );
+               if ( array_key_exists( $key, $blobs ) ) {
+                       $blob = $blobs[$key];
+                       $value = $this->unserialize( $blob );
+
+                       $casToken = ( $value !== false ) ? $blob : null;
 
-       protected function getWithToken( $key, &$casToken, $flags = 0 ) {
-               $values = $this->getMulti( [ $key ] );
-               if ( array_key_exists( $key, $values ) ) {
-                       $casToken = $values[$key];
-                       return $values[$key];
+                       return $value;
                }
+
                return false;
        }
 
        public function getMulti( array $keys, $flags = 0 ) {
+               $values = [];
+
+               $blobs = $this->fetchBlobMulti( $keys );
+               foreach ( $blobs as $key => $blob ) {
+                       $values[$key] = $this->unserialize( $blob );
+               }
+
+               return $values;
+       }
+
+       public function fetchBlobMulti( array $keys, $flags = 0 ) {
                $values = []; // array of (key => value)
 
                $keysByTable = [];
@@ -298,7 +310,7 @@ class SqlBagOStuff extends BagOStuff {
                                        if ( $this->isExpired( $db, $row->exptime ) ) { // MISS
                                                $this->debug( "get: key has expired" );
                                        } else { // HIT
-                                               $values[$key] = $this->unserialize( $db->decodeBlob( $row->value ) );
+                                               $values[$key] = $db->decodeBlob( $row->value );
                                        }
                                } catch ( DBQueryError $e ) {
                                        $this->handleWriteError( $e, $db, $row->serverIndex );
@@ -420,7 +432,7 @@ class SqlBagOStuff extends BagOStuff {
                                ],
                                [
                                        'keyname' => $key,
-                                       'value' => $db->encodeBlob( $this->serialize( $casToken ) )
+                                       'value' => $db->encodeBlob( $casToken )
                                ],
                                __METHOD__
                        );

diff --git a/tests/phpunit/includes/libs/objectcache/CachedBagOStuffTest.php b/tests/phpunit/includes/libs/objectcache/CachedBagOStuffTest.php
index d0360a9..f953319 100644 (file)
--- a/tests/phpunit/includes/libs/objectcache/CachedBagOStuffTest.php
+++ b/tests/phpunit/includes/libs/objectcache/CachedBagOStuffTest.php
@@ -11,7 +11,7 @@ class CachedBagOStuffTest extends PHPUnit\Framework\TestCase {
 
        /**
         * @covers CachedBagOStuff::__construct
-        * @covers CachedBagOStuff::doGet
+        * @covers CachedBagOStuff::get
         */
        public function testGetFromBackend() {
                $backend = new HashBagOStuff;
@@ -36,6 +36,7 @@ class CachedBagOStuffTest extends PHPUnit\Framework\TestCase {
                        $cache->set( "key$i", 1 );
                        $this->assertEquals( 1, $cache->get( "key$i" ) );
                        $this->assertEquals( 1, $backend->get( "key$i" ) );
+
                        $cache->delete( "key$i" );
                        $this->assertEquals( false, $cache->get( "key$i" ) );
                        $this->assertEquals( false, $backend->get( "key$i" ) );
@@ -67,7 +68,7 @@ class CachedBagOStuffTest extends PHPUnit\Framework\TestCase {
        }
 
        /**
-        * @covers CachedBagOStuff::doGet
+        * @covers CachedBagOStuff::get
         */
        public function testCacheBackendMisses() {
                $backend = new HashBagOStuff;