grapheme_extract should pass over invalid surrogate halves

[dmsnell]

See #17404

Many systems incorrectly encode surrogate halves from a UTF-16 stream into UTF-8 as two three-byte characters instead of the proper four-byte sequence. These are invalid charaters in UTF-8 and should be skipped when decoding with grapheme_extract but it’s not currently handling these properly.

If offset does not point to the first byte of a UTF-8 character,
the start position is moved to the next character boundary.

For example, U+1F170 (d83c dd70) should encode as F0 9F 85 B0, but when applying the UTF-8 encoder invalidly to d83c, the output would be ED A0 BD. This entire span of bytes is invalid UTF-8.

grapheme_extract( "\xED\xA0\xBDa", 1, GRAPHEME_EXTR_COUNT, 0, $next );
// returns "\xED", an invalid UTF-8 byte sequence
// $next === 1, pointing into the middle of the invalid sequence

Instead, it should return “a” and point $next to the end of the string.

[youkidearitai]

My understand is surrogate pair that only UTF-16. UTF-8 can cover all code points.
Therefore, this result of $next is 1. Second byte (\xA0) is only invalid byte.

[dmsnell]

UTF-8 can cover all code points.

This is almost correct, as UTF-8 encodes Unicode Scalar Values, which prohibit the unassigned code points in the surrogate range.

Because surrogate code points are not Unicode scalar values, any UTF-8 byte sequence that would otherwise map to code points U+D800..U+DFFF is ill-formed.
UTF-8

Further, both %ED and %A0 (and %BD for this matter) are invalid UTF-8 sequences which should not be returned from this function because they are not grapheme clusters, extended grapheme clusters, or even code points. They are simply invalid bytes on their own.

Particularly since the documentation states that grapheme_extract operates on UTF-8 sequences, it should not return invalid UTF-8 as if it were a character, and should skip over all invalid bytes, of which the surrogate range cannot be represented.

[cmb69]

Hmm, the current behavior does not really make sense, but I'm not sure what the proper behavior would be. The grapheme_extract() man page states:

Function to extract a sequence of default grapheme clusters from a text buffer, which must be encoded in UTF-8

Now, invalid UTF-8 is not UTF-8.

If offset does not point to the first byte of a UTF-8 character, the start position is moved to the next character boundary.

This may still make sense even if we assume valid UTF-8, because a user might just start at a wrong position.

And then we have: https://3v4l.org/iLSL9. Given that these are not (valid) code points, what am I missing?

[dmsnell]

what am I missing?

@cmb69 I believe it’s the ->getPartsIterator()

$bi = IntlBreakIterator::createCodePointInstance();
$bi->setText("A\xED\xA0\xBDa");
foreach ($bi->getPartsIterator() as $cp) {
    var_dump($bi->getErrorMessage(), bin2hex($cp));
}

this produces

string(12) "U_ZERO_ERROR"
string(2) "41"
string(12) "U_ZERO_ERROR"
string(2) "ed"
string(12) "U_ZERO_ERROR"
string(2) "a0"
string(12) "U_ZERO_ERROR"
string(2) "bd"
string(12) "U_ZERO_ERROR"
string(2) "61"

https://3v4l.org/73D52

[dmsnell]

invalid UTF-8 is not UTF-8.

This is a fair point, and if it comes to the function rejecting these inputs that would be tolerable, though less useful.

Doing so would require that the string have already been pre-scanned for having a valid UTF-8 encoding, or scan through before reading the first grapheme/code-point/bytes.

Given that this is inherently a streaming function, it’s a useful property to return valid sequences where they exist without having to read the entire string first and without rejecting a valid prefix because of later problems. Being able to identify those invalid byte sequences also helps, giving user-space code the choice of whether to replace the sequence with U+FFFD for display, or pass through the invalid bytes unaltered, or remove them.

[cmb69]

I believe it’s the ->getPartsIterator()

That doesn't really matter here. Iterating over the "CodePointIterator" gives the byte offsets (int is converted to string when passed to bin2hex()), and single bytes are reported as code points, although they are clearly invalid. Is this a bug in ICU (unlikely), or in PHP, or do we deliberate do this? If the latter, I think that grapheme_extract() should do the same (i.e. behave as it is behaving now). But I still think this behavior does not make much sense.

This is a fair point, and if it comes to the function rejecting these inputs that would be tolerable, though less useful.

I can see valid arguments for both approaches, but I'm unsure what I'd prefer. I would like some feedback from others.