Tests for: Setting prefix_key is not use by memcache

Thanks @egruel. The patch seems to fix the issue. Tested on a multisite installation with 3 sites.

As for @egruel's comment, settings this as RTBC.

This looks good, aside from one thing, can you add a separator between the prefix and the key please? so:

$full_key = urlencode($this->prefix . '-' . $key);

This code must have been lost in the port to 8.x somehow.

Thanks, this has been committed and pushed!

Seems missed prefixing in hashed version. Now :

    if (strlen($full_key) > 250) {
      $full_key = urlencode(hash($this->hashAlgorithm, $key));
    }

Instead of :

    if (strlen($full_key) > 250) {
      $full_key = urlencode(hash($this->hashAlgorithm, $this->prefix . '-' . $key));
    }

Good catch! Seems we need some additional test coverage to get this stuff...

Seems that patch need update.

Committed the fix, leaving this open for tests.

There's a problem with the implementation of the hashed keys here; at the moment when a key is going to be too long:

    // Memcache only supports key lengths up to 250 bytes.  If we have generated
    // a longer key, we shrink it to an acceptable length with a configurable   
    // hashing algorithm. Sha1 was selected as the default as it performs          
    // quickly with minimal collisions.                                            
    if (strlen($full_key) > 250) {                                                 
      $full_key = urlencode(hash($this->hashAlgorithm, $this->prefix . '-' . $key));
    }

So the prefix and the key are both hashed, and we end up with keys which look something like this:

6324dd4dce0c83b817c2d1298253b92f6264c69c

decd642fbfed9cdfd816238375ceedbae2e5c220

Whereas in the D7 branch, #1613622: Memcache not using urlencode on $prefix or $bin in dmemcache_key() when strlen($full_key) > 250 led to the keys being hashed like this:

    if (strlen($full_key) > $maxlen) {                                              
      $full_keys[$k] = urlencode($prefix[$k] . $bin) . '-' . hash(variable_get('memcache_key_hash_algorithm', 'sha1'), $key);
      $full_keys[$k] .= '-' . substr(urlencode($key), 0, ($maxlen - 1) - strlen($full_keys[$k]) - 1);
    }

So the prefix is preserved before the hash, and whatever characters remain within the key length limit are filled up with the original key.

Re-implementing that pattern in the D8 code gives us keys more like this:

prefixexample_-175236b40b657bc302d03aaa1ba9548404c37f9e-render-entity_view%3Ablock%3Abartik_help%3A%5Blanguages%3Alanguage_interface%5D%3Den%3A%5Broute%5D%3Duser.login35786c7117b4e38d0f169239752ce71158266ae2f6e4aa230fbbb87bd699c0e3%3A%5Btheme%5D%3Dba

prefixexample_-ed31b469a42c56e40eeda12f3e1d800d6fe74944-dynamic_page_cache-response%3A%5Blanguages%3Alanguage_interface%5D%3Den%3A%5Brequest_format%5D%3Dhtml%3A%5Broute%5D%3Duser.login35786c7117b4e38d0f169239752ce71158266ae2f6e4aa230fbbb87bd699c0e3%3

The shorter keys may be more efficient, but the lack of any human-readable information in the fully-hashed keys is not helpful when it comes to debugging / performance analysis.

I propose that the longer more detailed keys are adopted in the D8 branch too; here's a patch which does that.

If shorter keys are desirable for performance reasons, the key length limit could be made configurable (like it is in D7).

Agree - this should be the best way to handle the hash, keeping the meta data. But does not the key get to long with this patch?

does not the key get to long with this patch?

Not unless I've made a mistake in re-implementing the code from the D7 branch :)

I may have confused things by replacing the real prefix in my example keys.

Here's how the logic should work:

php > $prefix = str_repeat('A', 10);
php > $key = str_repeat('X', 300);
php > $full_key = $prefix . '-' . $key;
php > print $full_key;
AAAAAAAAAA-XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
php > print strlen($full_key);
311
php > if (strlen($full_key) > 250) {
php { $new_key = urlencode($prefix . '-' . hash('sha1', $key));
php { $new_key .= '-' . substr(urlencode($key), 0, (250 - 1) - strlen($new_key) - 1);
php { }
php > print $new_key;
AAAAAAAAAA-bbbdaf306be6968b2bc9d0c4ff022df06be96fef-XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
php > print strlen($new_key);
249

So IIUC the last concatenation adds on as many characters of the original key as possible up to a total of 249 chars, keeping the whole thing within the limit.

Certainly a good candidate for a test though :)

Marking as a beta blocker.

This should be a separate issue. This was kept open for tests, not to implement a different change.

I've opened #2995457: Human readable hashed memcache keys. Updating the title to indicate this issue is for test coverage only.