Bug 69079 - enhancement for mb_substitute_character

[masakielastic]

This pull reques adds function for checking the argument of range.

[smalyshev]

This looks like PHP, not C :)

[smalyshev]

Please fix the build, now it fails.

[yohgaki]

I wish to have mb_chr()/mb_ord() someday. Thank you. Please fix build, then Stas or I merge it after checking code.

[yohgaki]

Could you fetch current master branch and merge them?
I changed your other patch to optimize a little. There should be conflict because of this. Thank you.

[masakielastic]

@yohgaki sorry for late reply. I spent my summer vacation and thanks for your efforts.

[krakjoe]

Merged f9a435a

Thanks

PS. sorry about the long wait.

[nikic]

After investigating #1100 (comment) a bit more, I've noticed that this PR has a similar issue. However in this case it's not just a design problem: the codepoint check does not match the way it ends up being used. The substitute_character is always treated as a Unicode codepoint by the mbfl_convert module, but the check introduced here makes the same unicode/non-unicode distinction as in #1100.

As an example, consider this code:

<?php

$enc = 'EUC-JP-2004';
mb_internal_encoding($enc);

mb_substitute_character(0x8fa1ef); // EUC-JP-2004 encoding of U+50AA
var_dump(bin2hex(mb_convert_encoding("\x8d", $enc, $enc)));

mb_substitute_character(0x50aa);
var_dump(bin2hex(mb_convert_encoding("\x8d", $enc, $enc)));

Under PHP 7.1 this produces:

Warning: mb_substitute_character(): Unknown character.
string(2) "3f"
string(6) "8fa1ef"

Under PHP 7.2 with this patch it produces:

string(0) ""
Warning: mb_substitute_character(): Unknown character.
string(0) ""

The behavior in PHP 7.1 is correct, because mb_substitute_character() accepts a Unicode codepoint -- passing U+50AA is accepted and properly encodes to 0x8FA1EF in EUC-JP-2004. The behavior in PHP 7.2 is broken, as passing 0x8fa1ef passes the check in mb_substitute_character(), but is later ignored in mbfl_convert because it's not a legal unicode codepoint, while passing 0x50aa doesn't work either, because the check in mb_substitute_character() rejects it.

However even if this is fixed, there is still a more fundamental problem here: The check is performed against the internal_encoding. However the substitute_character must be legal in the target encoding of the encoding conversion (note: mb_convert_encoding uses the internal encoding as the default source encoding, not target encoding). As such, it is not actually possible to check whether the substitute character is going to be legal -- aside from the very general check that was already implemented previously.

[nikic]

I've fixed this in a8a9e93. The check is now again a simple range check. This still resolves the original report from bug #69079 -- the only real problem there was that our range checks were restricted to the BMP.