Memcache get with space in the key returns `false`

[shish]

<?php
require_once "vendor/autoload.php";
$memcache = new \Memcached();
$memcache->addServer('127.0.0.1', 11211);
$c = new \Sabre\Cache\Memcached($memcache);
var_export($c->get('sdfasdf'));
var_export($c->get('sdfas df'));

NULL
false

This doesn't seem to match the PSR-16 spec:

     * @return mixed The value of the item from the cache, or $default in case of cache miss.
     *
     * @throws \Psr\SimpleCache\InvalidArgumentException
     *   MUST be thrown if the $key string is not a legal value.
     */
    public function get($key, $default = null);

I think either keys with spaces in the name are legal, in which case a missing key should return NULL, or they are not legal, in which case the code should throw an exception; either way it shouldn't be returning false

[DeepDiver1975]

not a problem of this library - https://stackoverflow.com/questions/5826768/can-memcached-keys-contain-spaces

[shish]

Ok, I am very confused, please help me to understand?

• This library says: "Read PSR-16 for the API. We follow it to the letter."
• The memcache docs say: "keys with spaces are not legal values."
• The PSR-16 doc says: "InvalidArgumentException MUST be thrown if the $key string is not a legal value."

I can't see how a library can be following PSR-16 to the letter, when it reacts to an invalid key by returning false :S

[phil-davis]

https://www.php.net/manual/en/memcached.getresultcode.php

If the key is invalid then probably the call to $this->memcached->getResultCode() returns BAD_KEY_PROVIDED.

The existing code only checks for NOTFOUND

There are lots of other codes that can be returned, for example if the Memcached server is down, or does something else unexpected.

So it is probably useful that the get in this library returns null if the key was a valid key format and length, the Memcached server is running normally, and the key does not exist. But returns "something different" if "something less expected happened" - it returns false.

Then the caller can detect the difference between "the key is not found" and "something worse happened".

Maybe we should add to the @return description of function get() to mention that it can return false.

Note: set() returns a boolean - presumably if someone passes a key with whitespace to set then it will return false and the key-value pair will not be added to the cache.

[shish]

That docstring is taken directly from PSR-16 - it seems weird to be saying "Read PSR-16 for the API. We follow it to the letter" and then go ahead and change your own copy of the docs (which people aren't reading, because you told them to just read the PSR) to be different from PSR-16...?

~

FWIW, PSR-16 also says:

If it is not possible to return the exact saved value for any reason, implementing libraries MUST respond with a cache miss rather than corrupted data.

which sounds like a sensible behaviour for "something unexpected happened during getResultCode()"

~

PSR-16 also says:

Implementing libraries MUST support all serializable PHP data types, including:

• Booleans - True and False.
• Null - The null value (although it will not be distinguishable from a cache miss when reading it back out).

Note that it does not say

Implementing libraries MUST support all serializable PHP data types, including:

• Booleans - True and False. (although false will not be distinguishable from an internal library error when reading it back out)
• Null - The null value (although it will not be distinguishable from a cache miss when reading it back out).

~

The more I read this spec, the more convinced I am that "following the spec to the letter" means:

• if possible, return the exact value that was passed to set()
• if there's an invalid key, InvalidArgumentException MUST be thrown
• if there's any other error that prevents get() from working properly, implementing libraries MUST respond with a cache miss (ie null)

If y'all don't want to follow PSR-16 to the letter, then I can switch to a library that does; but given "Read PSR-16 for the API. We follow it to the letter" in your README, I made the assumption that you are aiming to follow it and would appreciate a divergence from the spec being pointed out ^^;;

[shish]

Then the caller can detect the difference between "the key is not found" and "something worse happened".

This sounds like a valuable feature, but returning "false" (a totally valid cache value) to indicate:

• everything is fine, you stored the value false in the cache and you want to get the value false back out of the cache
• or the server is down
• or the client is misconfigured
• or the cache key is invalid
• and I'm not going to tell you which of the above situations is happening, have fun debugging! :D

seems both breaking the spec and not as useful as it could be. If we want to give the user some useful information about error conditions, how about:

• throw InvalidArgumentException for invalid keys (which the spec says is a hard requirement)
• return null for "cache miss, or general error" (which again, the spec says is a hard requirement)
• add a new method which wraps $memcached->getResultCode(), allowing the user to know the difference between "cache miss because the server doesn't have the data" and "cache miss because the server is down"

Adding a new method doesn't seem ideal if you want to implement the spec and only the spec, but since the spec doesn't seem to cover "how should the user tell the difference between cache miss and error?", adding a new method on top of the spec seems less-bad than breaking the spec?

[phil-davis]

See PR #53 - maybe something like that meets the spec better?

[shish]

53 appears match my understanding of the spec ❤️

~

Although I have just spotted another glitch in the spec itself :D

• "null is indistinguishable from a cache miss"
• "a cache miss should return $default" (which is normally null, but the user can specify a different default)

So in fact null is distinguishable from a cache miss -- if you call $cache->get("missingKey", "myDefault") and you get the response null, then you know that null was stored, because otherwise it would have returned "myDefault"

[phil-davis]

Change done in #53 and release as https://github.com/sabre-io/cache/releases/tag/2.0.1