fix(cache): restore PSR-16 v1/v2 LSP compatibility (#989)

.github/workflows/4.8.x-test.yml

@@ -53,3 +53,31 @@ jobs:
       - name: Run tests
         run: XDEBUG_MODE=coverage && phpunit -v -c tests/phpunit.xml --coverage-text --strict-coverage --stop-on-risky
         shell: bash
+
+  # Verifies that Cache remains LSP-compatible with every supported major of
+  # psr/simple-cache. If a future PSR-16 revision narrows the interface in a way
+  # that breaks our widened signatures, this job fatals at class load.
+  # See https://github.com/serbanghita/Mobile-Detect/issues/989.
+  psr16-compat:
+    runs-on: ubuntu-latest
+    strategy:
+      fail-fast: false
+      matrix:
+        php-version: [8.2, 8.4]
+        psr-simple-cache: ['^1.0', '^2.0', '^3.0']
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v3
+
+      - name: Setup PHP ${{ matrix.php-version }}
+        uses: shivammathur/setup-php@verbose
+        with:
+          php-version: ${{ matrix.php-version }}
+          extensions: mbstring, intl
+          tools: composer:latest
+
+      - name: Install with psr/simple-cache ${{ matrix.psr-simple-cache }}
+        run: composer require "psr/simple-cache:${{ matrix.psr-simple-cache }}" --update-with-dependencies --no-interaction
+
+      - name: Run tests
+        run: vendor/bin/phpunit -c tests/phpunit.xml

composer.json

@@ -15,7 +15,7 @@
     ],
     "require": {
         "php": ">=8.0",
-        "psr/simple-cache": "3.0.0"
+        "psr/simple-cache": "^1.0 || ^2.0 || ^3.0"
     },
     "require-dev": {
         "friendsofphp/php-cs-fixer": "3.95.1",

src/Cache/Cache.php

@@ -9,23 +9,36 @@
 use DateTime;
 
 use function is_int;
+use function is_iterable;
+use function is_string;
 use function time;
 
 /**
- * In-memory cache implementation of PSR-16
- * @See https://www.php-fig.org/psr/psr-16/
+ * In-memory cache implementation of PSR-16.
+ *
+ * @see https://www.php-fig.org/psr/psr-16/
+ *
+ * Public method parameters are intentionally left without scalar type
+ * declarations (return types are kept). This keeps the class
+ * Liskov-compatible with PSR-16 v1/v2 as well as v3, so it loads cleanly
+ * in hosts (e.g. WordPress sites) where another plugin's autoloader has
+ * already registered an older `Psr\SimpleCache\CacheInterface`.
+ * Re-narrowing these parameters will break that compatibility. See
+ * https://github.com/serbanghita/Mobile-Detect/issues/989.
  */
 class Cache implements CacheInterface
 {
     protected array $cache = [];
 
     /**
-     * @inheritdoc
+     * @param string $key
+     * @param mixed  $default
+     * @return mixed
      * @throws CacheInvalidArgumentException
      */
-    public function get(string $key, mixed $default = null): mixed
+    public function get($key, mixed $default = null): mixed
     {
-        $this->checkKey($key);
+        $key = $this->checkKey($key);
 
         if (isset($this->cache[$key])) {
             if ($this->cache[$key]['ttl'] === null || $this->cache[$key]['ttl'] > time()) {
@@ -39,12 +52,15 @@ public function get(string $key, mixed $default = null): mixed
     }
 
     /**
-     * @inheritdoc
+     * @param string                $key
+     * @param mixed                 $value
+     * @param int|DateInterval|null $ttl
      * @throws CacheInvalidArgumentException
      */
-    public function set(string $key, mixed $value, int|DateInterval|null $ttl = null): bool
+    public function set($key, mixed $value, $ttl = null): bool
     {
-        $this->checkKey($key);
+        $key = $this->checkKey($key);
+        $ttl = $this->checkTtl($ttl);
 
         // From https://www.php-fig.org/psr/psr-16/ "Definitions" -> "Expiration"
         // If a negative or zero TTL is provided, the item MUST be deleted from the cache if it exists, as it is expired already.
@@ -64,20 +80,20 @@ public function set(string $key, mixed $value, int|DateInterval|null $ttl = null
         return true;
     }
 
-    /** @inheritdoc */
-    public function delete(string $key): bool
+    /**
+     * @param string $key
+     * @throws CacheInvalidArgumentException
+     */
+    public function delete($key): bool
     {
-        $this->checkKey($key);
+        $key = $this->checkKey($key);
         $this->deleteSingle($key);
 
         return true;
     }
 
     /**
      * Deletes the cache item from memory.
-     *
-     * @param string $key Cache key
-     * @return void
      */
     private function deleteSingle(string $key): void
     {
@@ -93,12 +109,12 @@ public function clear(): bool
     }
 
     /**
-     * @inheritdoc
+     * @param string $key
      * @throws CacheInvalidArgumentException
      */
-    public function has(string $key): bool
+    public function has($key): bool
     {
-        $this->checkKey($key);
+        $key = $this->checkKey($key);
 
         if (isset($this->cache[$key])) {
             if ($this->cache[$key]['ttl'] === null || $this->cache[$key]['ttl'] > time()) {
@@ -111,33 +127,49 @@ public function has(string $key): bool
         return false;
     }
 
-    /** @inheritdoc */
-    public function getMultiple(iterable $keys, mixed $default = null): iterable
+    /**
+     * @param iterable<string> $keys
+     * @param mixed            $default
+     * @throws CacheInvalidArgumentException
+     */
+    public function getMultiple($keys, mixed $default = null): iterable
     {
-        $data = [];
+        $keys = $this->checkIterable($keys, 'keys');
 
+        $data = [];
         foreach ($keys as $key) {
             $data[$key] = $this->get($key, $default);
         }
 
         return $data;
     }
 
-    /** @inheritdoc */
-    public function setMultiple(iterable $values, int|DateInterval|null $ttl = null): bool
+    /**
+     * @param iterable<string, mixed> $values
+     * @param int|DateInterval|null   $ttl
+     * @throws CacheInvalidArgumentException
+     */
+    public function setMultiple($values, $ttl = null): bool
     {
-        $return = [];
+        $values = $this->checkIterable($values, 'values');
+        $ttl = $this->checkTtl($ttl);
 
+        $return = [];
         foreach ($values as $key => $value) {
             $return[] = $this->set($key, $value, $ttl);
         }
 
         return $this->checkReturn($return);
     }
 
-    /** @inheritdoc */
-    public function deleteMultiple(iterable $keys): bool
+    /**
+     * @param iterable<string> $keys
+     * @throws CacheInvalidArgumentException
+     */
+    public function deleteMultiple($keys): bool
     {
+        $keys = $this->checkIterable($keys, 'keys');
+
         foreach ($keys as $key) {
             $this->delete($key);
         }
@@ -146,10 +178,14 @@ public function deleteMultiple(iterable $keys): bool
     }
 
     /**
+     * @param mixed $key
      * @throws CacheInvalidArgumentException
      */
-    protected function checkKey(string $key): string
+    protected function checkKey($key): string
     {
+        if (!is_string($key)) {
+            throw new CacheInvalidArgumentException('Cache key must be a string.');
+        }
 
         if ($key === '' || !preg_match('/^[A-Za-z0-9_.]{1,64}$/', $key)) {
             throw new CacheInvalidArgumentException("Invalid key: '$key'. Must be alphanumeric, can contain _ and . and can be maximum of 64 chars.");
@@ -158,10 +194,35 @@ protected function checkKey(string $key): string
         return $key;
     }
 
-    /**  */
-    protected function getTTL(DateInterval|int|null $ttl): ?int
+    /**
+     * @param mixed $ttl
+     * @throws CacheInvalidArgumentException
+     */
+    protected function checkTtl($ttl): int|DateInterval|null
+    {
+        if ($ttl !== null && !is_int($ttl) && !($ttl instanceof DateInterval)) {
+            throw new CacheInvalidArgumentException('TTL must be null, int, or DateInterval.');
+        }
+
+        return $ttl;
+    }
+
+    /**
+     * @param mixed $iterable
+     * @return iterable<mixed>
+     * @throws CacheInvalidArgumentException
+     */
+    protected function checkIterable($iterable, string $argName): iterable
     {
+        if (!is_iterable($iterable)) {
+            throw new CacheInvalidArgumentException(sprintf('%s must be iterable.', ucfirst($argName)));
+        }
+
+        return $iterable;
+    }
 
+    protected function getTTL(DateInterval|int|null $ttl): ?int
+    {
         if ($ttl instanceof DateInterval) {
             return (new DateTime())->add($ttl)->getTimestamp() - time();
         }

tests/CacheTest.php

@@ -396,4 +396,77 @@ public function testEvictExpiredOnEmptyCache(): void
 
         $this->assertEquals(0, $evicted);
     }
+
+    /**
+     * Regression test for https://github.com/serbanghita/Mobile-Detect/issues/989
+     *
+     * Cache must remain LSP-compatible with PSR-16 v1 (untyped parameters) so it
+     * loads cleanly in hosts where another plugin has registered an older
+     * Psr\SimpleCache\CacheInterface. Re-adding scalar type declarations on the
+     * listed parameters will break that at class load with a fatal error.
+     */
+    public function testPublicMethodParametersAreUntypedForPsr16V1Compatibility(): void
+    {
+        // [method => parameter index] pairs that MUST have no type declaration.
+        $untypedSlots = [
+            ['get', 0],              // $key
+            ['set', 0],              // $key
+            ['set', 2],              // $ttl
+            ['delete', 0],           // $key
+            ['has', 0],              // $key
+            ['getMultiple', 0],      // $keys
+            ['setMultiple', 0],      // $values
+            ['setMultiple', 1],      // $ttl
+            ['deleteMultiple', 0],   // $keys
+        ];
+
+        foreach ($untypedSlots as [$method, $index]) {
+            $param = (new \ReflectionMethod(Cache::class, $method))->getParameters()[$index];
+            self::assertFalse(
+                $param->hasType(),
+                sprintf(
+                    'Cache::%s() parameter $%s must have no type declaration for PSR-16 v1 LSP compatibility, got: %s',
+                    $method,
+                    $param->getName(),
+                    (string) $param->getType()
+                )
+            );
+        }
+    }
+
+    public function testSetWithInvalidTtlTypeThrowsException(): void
+    {
+        $this->expectException(CacheInvalidArgumentException::class);
+        $this->cache->set('isMobile', true, 'not-a-valid-ttl');
+    }
+
+    public function testSetMultipleWithNonIterableThrowsException(): void
+    {
+        $this->expectException(CacheInvalidArgumentException::class);
+        $this->cache->setMultiple('not-iterable');
+    }
+
+    public function testGetMultipleWithNonIterableThrowsException(): void
+    {
+        $this->expectException(CacheInvalidArgumentException::class);
+        $this->cache->getMultiple('not-iterable');
+    }
+
+    public function testDeleteMultipleWithNonIterableThrowsException(): void
+    {
+        $this->expectException(CacheInvalidArgumentException::class);
+        $this->cache->deleteMultiple('not-iterable');
+    }
+
+    public function testGetWithNonStringKeyThrowsException(): void
+    {
+        $this->expectException(CacheInvalidArgumentException::class);
+        $this->cache->get(123);
+    }
+
+    public function testSetWithNonStringKeyThrowsException(): void
+    {
+        $this->expectException(CacheInvalidArgumentException::class);
+        $this->cache->set(['notAKey'], 'value');
+    }
 }