Transliteration a string containing an unknown character (e.g. 0x80) is not valid

This attached patch does solve the issue to provide your expected outcomes but should have someone who is very confident about their UTF8 character encoding review it. Code inspiration to handle this from this comment on php.net.

Essentially the issue appears to be that preg_split('//', $string, 0, PREG_SPLIT_NO_EMPTY); fails to split the string for your examples.

Here is where I am not confident as the patch causes a change to the expected result of an existing test as you can see.

I am not sure if the expected result of $string = chr(0xF8) . chr(0x80) . chr(0x80) . chr(0x80) . chr(0x80); is intended to be '?' instead of '?????' denoting the 5 unknown characters. Not sure if that was an incorrectly set up test or the actual intension (ie, 5 invalid characters resulting in a single '?').

I have a feeling the result of the single '?' was simply because preg_split was failing to split the string so that was the outcome and the test was trying to ensure that that remained the outcome. In reality, if a user passes a string of 5 unknown characters, getting 5 '?'s back seems more logical, particularly in more normal cases like the issue description where valid characters are mixed in.

So a lot of question marks here both literally and figuratively!

To be honest, I think it does not particularly matter whether this or the related issue goes first: whichever does I am happy to update the other patch - feel free to disagree of course!

And the patch, sorry!

Fixed patch, removed test code.

Thanks - yeah, I need to update the patch now that this was committed:
https://www.drupal.org/project/drupal/issues/3000630

They affect more or less the same lines of code. Will try to get to this in the next days.

Nice bug find!

+++ b/core/lib/Drupal/Component/Transliteration/PhpTransliteration.php
@@ -107,8 +107,18 @@ public function removeDiacritics($string) {
-    // Split into Unicode characters and transliterate each one.
-    foreach (preg_split('//u', $string, 0, PREG_SPLIT_NO_EMPTY) as $character) {
+    $characters = [];
+
+    // Split string into array handling unknown characters.
+    $strlen = mb_strlen($string);
+    while ($strlen) {
+      $characters[] = mb_substr($string, 0, 1, 'UTF-8');
+      $string = mb_substr($string, 1, $strlen, 'UTF-8');
+      $strlen = mb_strlen($string);
+    }
+
+    // Transliterate each character.
+    foreach ($characters as $character) {

Rather than removing the preg_split() we can enforce UTF-8 by doing

$string = mb_convert_encoding($string, 'UTF-8', 'UTF-8');

Before the preg_split()... this will replace all unknown UTF chars with the ? and the new tests will pass. Less function calls.

@Krzysztof Domański good point. We should add test coverage of that here then.

Nice, thanks for moving this forward! Good call with the mb_convert_encoding instead of the loop. This looks good and works for me.

The current approach has problems when max length is used. Also I don't it makes much sense when the $unknown_character is a character that should also be transliterated. So what this means is that we should do the replacement to bring back the original question marks before transliterating.

Patch attached also improves the commentary and removes the risk of a hash clash by using a hash based on the provided string. This uses PHP's hash() function directly to keep the transliteration component dependency free.

More random-ness.

Committed and pushed f9e7921bc8 to 8.7.x and 040e6275a0 to 8.6.x. Thanks!