mbstring_check_encoding() in cache database backend doesn't actually work

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -407,7 +407,11 @@ protected function catchException(\Exception $e, $table_name = NULL) {
+    } else {

The else { needs to be on a new line.

Edit: here's the reference: https://www.drupal.org/coding-standards#controlstruct

This error cropped up for me on a fresh install after switching to a new PHP version (mbstring disabled by default). Patch in #7 resolved it.

Having written that original line of code, I think the patch is fine - mb_check_encoding() is faster so we default to that, and fall back to a regular expression (as promised in the installer)

This should have a unit test though - we can put function_exists('mb_check_encoding') in a separate method and mock that.

1. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -42,6 +42,11 @@ class DatabaseBackend implements CacheBackendInterface {
   +  protected $useMbstring;
   
   @@ -58,6 +63,9 @@ public function __construct(Connection $connection, CacheTagsChecksumInterface $
   +    $this->useMbstring = function_exists('mb_check_encoding');
   

   Let's stick to adding a method that wraps function_exists('mb_check_encoding') rather than adding another property?

2. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -58,6 +63,9 @@ public function __construct(Connection $connection, CacheTagsChecksumInterface $
   +    // check if mb_check_encoding function is present
   

   We start comments with a capital and end with a full stop.

3. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -421,6 +429,25 @@ protected function normalizeCid($cid) {
   +   * Checks to see if a given string is only made up of ascii characters
   

   Capital/full stop.

4. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -421,6 +429,25 @@ protected function normalizeCid($cid) {
   +   * Uses mb_check_encoding if present, otherwise falls back to preg_match
   

   Missing newline before long description.

5. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -421,6 +429,25 @@ protected function normalizeCid($cid) {
   +   *   the text to check
   

   Capital/full stop.

6. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -421,6 +429,25 @@ protected function normalizeCid($cid) {
   +   *   true if $text only contains ascii characters, otherwise false
   

   Capital/full stop.

7. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -421,6 +429,25 @@ protected function normalizeCid($cid) {
   +    if ($this->useMbstring) {
   

   if ($this->mbStringExists())?

8. +++ b/core/tests/Drupal/Tests/Core/Cache/DatabaseBackendTest.php
   @@ -0,0 +1,91 @@
   +class DatabaseBackendTest extends UnitTestCase
   +{
   

   { goes on the same line as the class

9. +++ b/core/tests/Drupal/Tests/Core/Cache/DatabaseBackendTest.php
   @@ -0,0 +1,91 @@
   +    /**
   

   indentation is by 2 spaces

10. +++ b/core/tests/Drupal/Tests/Core/Cache/DatabaseBackendTest.php
    @@ -0,0 +1,91 @@
    +     * Tests if the correct function is choosen
    

    chosen.

11. +++ b/core/tests/Drupal/Tests/Core/Cache/DatabaseBackendTest.php
    @@ -0,0 +1,91 @@
    +        // make method and variable accessible through reflection
    ...
    +        // create classes
    ...
    +            // ascii only, should return same
    ...
    +            // utf-8, should return hash
    ...
    +            // ascii, over 255 long, should return hash padded with original string to a 255 length
    ...
    +            // utf-8, over 255 long, should return hash
    ...
    +        // make method and variable accessible through reflection
    ...
    +        // create classes
    ...
    +            // run checks
    ...
    +        // now force usage of preg
    

    Capital/full stop.

12. +++ b/core/tests/Drupal/Tests/Core/Cache/DatabaseBackendTest.php
    @@ -0,0 +1,91 @@
    +     * Tests that the DatabaseBackend normalizeCid works both with or without mbstring.
    

    Short description should be 80 chars max.

I tried to summarize from the comments the recommended approach based off of TomCan's and stefan_r's review.

I think the correct status is currently Needs work.

Fixed remarks in #14 and moved tests as suggested in #15.

Discussed this in person with @borisson_ at the sprint in BCN. Tests will be added and we'll get rid of useMbstring.

Discussed this yesterday with dawehner, instead of using reflection we're now using a subclass. Test currently fails though.

Interdiff is bigger than the patch so only uploading a patch.

Is that the correct patch? It still has a reflection class and has useMbstring?

@mradcliffe, thanks! This should be the correct patch.

1. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -421,6 +421,36 @@ protected function normalizeCid($cid) {
   +   * @return bool
   

   This is different from the "@return boolean" above. It may have the

2. +++ b/core/modules/system/src/Tests/Cache/DatabaseBackendUnitTest.php
   @@ -53,4 +54,99 @@ public function testSetGet() {
   +class MockedDatabaseBackend extends DatabaseBackend {
   

   The database system has the precedent of using the Drupal\Tests\Core\Database\Stub namespace. Is that something that we want to replicate instead of having the stub in the test file? Would other tests find it useful? The MockedDatabaseBackend class in the test is fairly localized in its requirements to test the specific return values required for testing the existence of mbString so it may not have value enough on its own.

According to the test failure in https://www.drupal.org/pift-ci-job/53967 the mb_check_encoding doesn't help anway, so we should maybe reconsider to hash it always.

@dawehner what do you mean, doesn't help?

Hashing always would be a simpler solution, if I remember correctly the conditional hashing was to help with debugging or perhaps for performance reasons, maybe @catch would remember?

@dawehner what do you mean, doesn't help?

Well, it seems to not hash it, see the error message, even https://3v4l.org/N9u3X indicates it should

Not hashing all the time was for debugging I think, but if it's unreliable, then we can skip the debugging aid - there are other ways to debug cache items.

#7 worked for me.. Thanks!

There's also a reference to mb_check_encoding in normalizeLockName in core/lib/Drupal/Core/Cache/CacheCollector.php

protected function normalizeLockName($cid) {
    // Nothing to do if the ID is a US ASCII string of 255 characters or less.
    $cid_is_ascii = mb_check_encoding($cid, 'ASCII');
...

So we probably need a generic function to take care of checking whether mb_check_encoding is available.

I haven't spent enough time with D8 to suggest the best place for such a function though.

So based on some simple benchmarks, preg_match() seems to be about twice as slow as mb_check_encoding(). But as @dawehner pointed out, mb_check_encoding() is actually wrong in the example string from him. So what's the point of being faster if the result is wrong? ;)

What's interesting is that on PHP 5, hashing is slower than preg_match, but on PHP7, that's actually not the case. So from a performance perspective, on PHP7, it almost isn't worth checking, the only argument to do so is making debugging easier.

$ php bench/mbstring.php 
mb_check_encoding:
Total time: 0.9808s
Per call: 0.002ms
preg_match:
Total time: 1.7637s
Per call: 0.0035ms
hashing:
Total time: 1.9511s
Per call: 0.0039ms
$ php7 bench/mbstring.php 
mb_check_encoding:
Total time: 0.6707s
Per call: 0.0013ms
preg_match:
Total time: 1.303s
Per call: 0.0026ms
hashing:
Total time: 0.8716s
Per call: 0.0017ms

So I personally use cids in the database backend for debugging at least against Drupal 7 - makes it very easy to see which cache entries are being created and how many etc.

You can get that from writing out somewhere but a lot of extra work compared to running some queries. So for me I'd go for the preg_match().

Once we require PHP 7 we might want to revisit, but to get very good performance, you shouldn't be using database caching anyway.

Not sure per the above whether this should be using mb_check_encoding() anyway, but if we are providing a polyfill for that function in PHP (which at least the latest patch is doing) then I would have expected that to live in \Drupal\Component\Utility\Unicode as a new checkEncoding() method

We already pull in the polyfill component of symfony, so we could leverage https://github.com/symfony/polyfill/blob/master/src/Mbstring/Mbstring.php even I still think, its pointless to do so, when it doesn't work in the first place.

@dawehner/@Berdir I still dont understand why this is wrong?

mb_check_encoding is merely supposed to check if the string is an ascii string, which it does a good job of in the 3v4l example -- it's not supposed to hash anything. Only if the string is not ascii do we need to hash it, but that happens elsewhere.

I'm Agreed with @stefan the 3v4l example seems correct to me as well.

So having vendor/symfony/polyfill-mbstring/Mbstring.php we should change the approach to fix this problem? we would need to test if the extension is loaded and if it is not the case then include this class?

@stefan.r
Well, we somehow have to explain the weird test failure above, don't we?

Per #2547419: Drupal requires mbstring PHP extension during install phase we now ship Symfony's mbstring polyfill with core, so this should no-longer fatal. But if the check itself isn't working we probably still need to do something here.

I just upgraded from Drupal 10 to Drupal 11.2.3, and I still get the error:

Deprecated function: mb_check_encoding(): Calling mb_check_encoding() without argument is deprecated in Drupal\Core\Cache\DatabaseBackend->normalizeCid() (line 501 of core/lib/Drupal/Core/Cache/DatabaseBackend.php).
Drupal\Core\Cache\DatabaseBackend->normalizeCid() (Line: 120)
Drupal\Core\Cache\DatabaseBackend->getMultiple() (Line: 110)
Drupal\Core\Cache\DatabaseBackend->get() (Line: 326)
Drupal\Core\Cache\CacheCollector->lazyLoadCache() (Line: 144)
Drupal\Core\Cache\CacheCollector->get() (Line: 112)

The patch for Drupal 11.2.3, possible Drupal 11.x

And the snippet to include it in composer.json "cweagans" patching under "drupal/core" section:
"2560123: Fix mb_check_encoding deprecation in DatabaseBackend.php": "https://www.drupal.org/files/issues/2025-08-10/cache_databasebackend-2560123-53.patch",

mb_check_encoding() shouldn't be called without arguments - that suggests NULL is being passed as a cache ID?