@ArchBlood @stof fyi

Just my two cents;

1. Key Validation

PSR-16 Requirement:

Keys must be strings of at most 64 characters, containing only A-Z, a-z, 0-9, _, and . (dot).

Fix: Update checkKey() method to follow the spec:

if (!is_string($key) || $key === '' || !preg_match('/^[A-Za-z0-9_.]{1,64}$/', $key)) {
    throw new InvalidArgumentException("Invalid key: '$key'. Must match /^[A-Za-z0-9_.]{1,64}$/");
}

2. has() method logic

Issue: Using get() can return the fallback default (null), which could falsely indicate a cache miss.

Fix: Check for key existence and TTL independently:

public function has(string $key): bool
{
    $key = $this->checkKey($key);

    return isset($this->cache[$key]) &&
        ($this->cache[$key]['ttl'] === null || $this->cache[$key]['ttl'] > time());
}

3. fromIterable() Validation

PSR-16 Requires: getMultiple(), setMultiple(), and deleteMultiple() MUST validate input as array or Traversable, and validate each key.

Fix: Call checkKey() inside getMultiple(), setMultiple(), and deleteMultiple() for each key:

foreach ($this->fromIterable($keys) as $key) {
    $key = $this->checkKey($key);
    ...
}

4. Avoid Unused checkKeyArray()

If you're not using checkKeyArray(), it's safe to remove it.

5. PSR-16 Exceptions

You should throw Psr\SimpleCache\InvalidArgumentException, not PHP’s built-in one.

Fix: Create a custom exception that implements the correct interface:

use Psr\SimpleCache\InvalidArgumentException as PsrInvalidArgumentException;

class CacheException extends \InvalidArgumentException implements PsrInvalidArgumentException {}

And use it in checkKey():

throw new CacheException("...");

✅ Optional: Improvements

➕ Add expiration check abstraction

Extract TTL check to its own method:

protected function isExpired(?int $ttl): bool
{
    return $ttl !== null && $ttl <= time();
}

✅ Summary of Required Fixes

Note: Not sure on the use case for why changing methods from public to protected is needed here.

1. Key Validation

👍 agree

2. has() method logic

Issue: Using get() can return the fallback default (null), which could falsely indicate a cache miss.

Fix: Check for key existence and TTL independently:

public function has(string $key): bool
{
    $key = $this->checkKey($key);

    return isset($this->cache[$key]) &&
        ($this->cache[$key]['ttl'] === null || $this->cache[$key]['ttl'] > time());
}

👍

The docs are kinda ambiguous: Determines whether an item is present in the cache.
and this leads me to the fact that item MIGHT be present in the cache but not necessarily valid. But since has() only returns boolean then I deduce that it HAS to check for ttl as well.

I think you are correct, because you code also satisfies: @throws \Psr\SimpleCache\InvalidArgumentException * MUST be thrown if the $key string is not a legal value.

Also It is recommended that has() is only to be used for cache warming type purposes. So cache warming should always know if an item is expired or not.

3. fromIterable() Validation

PSR-16 Requires: getMultiple(), setMultiple(), and deleteMultiple() MUST validate input as array or Traversable, and validate each key.

Fix: Call checkKey() inside getMultiple(), setMultiple(), and deleteMultiple() for each key:

foreach ($this->fromIterable($keys) as $key) {
    $key = $this->checkKey($key);
    ...
}

👍

Looks like getMultiple, setMultiple and deleteMultiple have to throw \Psr\SimpleCache\InvalidArgumentException if key is invalid.

4. Avoid Unused checkKeyArray()

If you're not using checkKeyArray(), it's safe to remove it.

👍

5. PSR-16 Exceptions

You should throw Psr\SimpleCache\InvalidArgumentException, not PHP’s built-in one.

👍

Fix: Create a custom exception that implements the correct interface:

use Psr\SimpleCache\InvalidArgumentException as PsrInvalidArgumentException;

class CacheException extends \InvalidArgumentException implements PsrInvalidArgumentException {}

And use it in checkKey():

throw new CacheException("...");

That would be clean, but I'm kinda afraid of not interfering with somebody implementing a PSR-16 compatible cache system. Most likely I'll use the alias in code so I can see clearly what exception I'm throwing.

✅ Optional: Improvements

➕ Add expiration check abstraction

Extract TTL check to its own method:

protected function isExpired(?int $ttl): bool
{
    return $ttl !== null && $ttl <= time();
}

👍

✅ Summary of Required Fixes

Area Fix Required? Description
Key Validation ✅ Enforce PSR-compliant key rules
TTL Expiry Handling ✅ has() must not rely on get()
Iterable Key Checks ✅ Validate all keys in *Multiple() methods
Custom PSR Exception ✅ Throw correct PSR exception
Cleanup Unused Methods ⚠️ Remove checkKeyArray() if unused

Note: Not sure on the use case for why changing methods from public to protected is needed here.

I'm thinking that Cache class is basically a suggested in-memory solution, but typically you want to create your own based on Redis or other storage when you inject your own cache system. I don't think anybody cares to extend Cache.

Note: Not sure on the use case for why changing methods from public to protected is needed here.

I'm thinking that Cache class is basically a suggested in-memory solution, but typically you want to create your own based on Redis or other storage when you inject your own cache system. I don't think anybody cares to extend Cache.

I was just afraid that changing these methods would affect the implementation of the interface itself. 🤔

I was just afraid that changing these methods would affect the implementation of the interface itself. 🤔

@ArchBlood I checked Cache so now every method from Psr\SimpleCache\CacheInterface is still public, but the helpers and the rest are protected. Not sure I understand this.

Btw great comments 🙇 , really made me throw a hard look over the entire cache related code. Will come back today with the code changes.

I was just afraid that changing these methods would affect the implementation of the interface itself. 🤔

@ArchBlood I checked Cache so now every method from Psr\SimpleCache\CacheInterface is still public, but the helpers and the rest are protected. Not sure I understand this.

Btw great comments 🙇 , really made me throw a hard look over the entire cache related code. Will come back today with the code changes.

I look forward to the changes, and I'm sure that everyone that uses this will appreciate all of the hard work as well!

That would be clean, but I'm kinda afraid of not interfering with somebody implementing a PSR-16 compatible cache system. Most likely I'll use the alias in code so I can see clearly what exception I'm throwing.

your own exception class would be in your own namespace, so there is no interference.
throwing an exception that does not implement the PSR-16 interface would be what could interfere with error handling of code expecting PSR-16.

fyi: Still wip, even if all tests pass

You might want to add https://packagist.org/packages/cache/integration-tests in your dev dependencies to use this shared testsuite for PSR-16 (and PSR-6 but this is not relevant here) to ensure your implementation follows the PSR-16 rules properly.

@stof Thank you, checking this right now after I refactored Cache class, looks like it fails at all tests atm 🗡️

Failed asserting that exception of type "TypeError" matches expected exception "Psr\SimpleCache\InvalidArgumentException". Message was: "Detection\Cache\Cache::has(): Argument #1 ($key) must be of type string, array given, called in /home/sghita/work/Mobile-Detect/vendor/cache/integration-tests/src/SimpleCacheTest.php on line 541" at

I think these TypeErrors are fine, because types were introduced in PHP 8.0 so the code doesn't expect to be passed other key values than string

@Tynael thanks for the review, addressed all

I've also found testBasicUsageWithLongKey from vendor/cache/integration-tests/src/SimpleCacheTest.php which sets a huge cache key but I think that's wrong because it's against the PSR-16 spec (UTF-8 encoding and a length of up to 64 characters.) and can cause data too long issues

fyi: Still wip, even if all tests pass

You might want to add https://packagist.org/packages/cache/integration-tests in your dev dependencies to use this shared testsuite for PSR-16 (and PSR-6 but this is not relevant here) to ensure your implementation follows the PSR-16 rules properly.

@stof Thank you, checking this right now after I refactored Cache class, looks like it fails at all tests atm 🗡️

Failed asserting that exception of type "TypeError" matches expected exception "Psr\SimpleCache\InvalidArgumentException". Message was: "Detection\Cache\Cache::has(): Argument #1 ($key) must be of type string, array given, called in /home/sghita/work/Mobile-Detect/vendor/cache/integration-tests/src/SimpleCacheTest.php on line 541" at

I think these TypeErrors are fine, because types were introduced in PHP 8.0 so the code doesn't expect to be passed other key values than string

@Tynael thanks for the review, addressed all

You need to remove the scalar type hint string from the public methods, and instead validate the type manually in checkKey(). That way, you can throw the correct PSR-16 exception.

Example has():

public function has($key): bool
{
    $this->checkKey($key);

    if (!isset($this->cache[$key])) {
        return false;
    }

    if ($this->cache[$key]['ttl'] === null || $this->cache[$key]['ttl'] > time()) {
        return true;
    }

    $this->deleteSingle($key);
    return false;
}

Example checkKey():

protected function checkKey(mixed $key): string
{
    if (!is_string($key)) {
        throw new CacheInvalidArgumentException("Invalid key type: 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 max 64 characters.");
    }

    return $key;
}

@ArchBlood no way I'm removing scalar types, they are mentioned in the original interface

are you using AI-tools to check the changes?

@ArchBlood no way I'm removing scalar types, they are mentioned in the original interface

are you using AI-tools to check the changes?

I ran a check through GitHub on a private action in a HumHub-specific container to identify any issues. I'm in complete agreement if you'd prefer not removing scalar types and relying on using checkKey() to throw an exception for strings, should keep it in compliance either way tbh.

Addressed all your comments @stof @Tynael 🙇