CacheCollector can result in an exception is cid + classname is too long to be used as a lock name

I think this is a major bug given that we have no limit on cid length.

We actually have a test now that is throwing this exception, but not failing. #2535774: Error handler swallows catchable fatal errors surfaces this failing.

Probably not the best test in the world, but it illustrates the hidden exception.

This exception is actually not due to cid length, but rather to invalid characters. Hashing here would solve both length and characters.

This adds a more appropriate integration test of the cache collector. I added a @todo because I'm not sure why a cache get would still work after the destruct method is called. This method is what writes to the semaphore table (by way of lock), so we want that in the test.

This adds a cid/lock normalization method. It is copied directly from DatabaseBackend, and might make more sense as a static utility method somewhere.

Nevermind my review ... you copied existing code from the cache backend, so both points are not valid.

1. +++ b/core/lib/Drupal/Core/Cache/CacheCollector.php
   @@ -265,6 +266,30 @@ protected function updateCache($lock = TRUE) {
   +    // Nothing to do if the ID is a US ASCII string of 255 characters or less.
   +    $cid_is_ascii = mb_check_encoding($cid, 'ASCII');
   

   IMHO we usually don't require to have mb enabled, right? I guess when mb is not enabled we could just go with the hash directly?

gg

2. +++ b/core/lib/Drupal/Core/Cache/CacheCollector.php
   @@ -265,6 +266,30 @@ protected function updateCache($lock = TRUE) {
   +    // Return a string that uses as much as possible of the original cache ID
   +    // with the hash appended.
   +    $hash = Crypt::hashBase64($cid);
   

   Do we really need a cryptographic hash? Isn't it enough to go with something like sha512?

See #2560123: mbstring_check_encoding() in cache database backend doesn't actually work ...

+++ b/core/modules/system/src/Tests/Menu/MenuRouterTest.php
@@ -204,6 +204,7 @@ protected function doTestExoticPath() {
+    $this->assertNoText(t('The website encountered an unexpected error. Please try again later.'));

Let's change this to assert a 200 instead. Asserting that there is no error text is not a reliable way to provide coverage as it can yield false positives.

Discussed with @xjm, and we agree that fixing conditions that trigger exceptions is a great thing to do prior to release, so long as doing so isn't disruptive, which this doesn't appear to be.

re #15: That test was mainly meant to illustrate the error existed (in our current tests, before tests for this issue were specifically added). I'll reroll to check for a 200.

Nevermind, the original error isn't caught by checking for a 200 (that's one reason it has remained hidden in our tests). It properly fails once #2535774: Error handler swallows catchable fatal errors is applied. We can either leave this explicit check for the error message, or remove it entirely since there are now explicit tests here?

(Setting to RTBC to get xjm's attention)

Given this unblocks the error handler issue, committed/pushed to 8.0.x as is.

Let's improve the test coverage in the other issue though.