Bug #81390 [Com]: mb_detect_encoding() regression

Edit report at https://bugs.php.net/bug.php?id=81390&edit=1 ID: 81390 Comment by: alec at alec dot pl Reported by: alec at alec dot pl Summary: mb_detect_encoding() regression Status: Verified Type: Bug Package: mbstring related PHP Version: 8.1.0beta3 Assigned To: alexdowad Block user comment: N Private report: N New Comment: I guess I'll use a sane list of encodings, however there's still somethings wrong. $test = 'test:test'; $encodings = ['UTF-8', 'SJIS', 'GB2312', 'ISO-8859-1', 'ISO-8859-2', 'ISO-8859-3', 'ISO-8859-4', 'ISO-8859-5', 'ISO-8859-6', 'ISO-8859-7', 'ISO-8859-8', 'ISO-8859-9', 'ISO-8859-10', 'ISO-8859-13', 'ISO-8859-14', 'ISO-8859-15', 'ISO-8859-16', 'WINDOWS-1252', 'WINDOWS-1251', 'EUC-JP', 'EUC-TW', 'KOI8-R', 'BIG-5', 'ISO-2022-KR', 'ISO-2022-JP', 'UTF-16' ]; echo mb_detect_encoding($test, $encodings); returns "UTF-16". Maybe that's one of the issues you described already. Previous Comments: ------------------------------------------------------------------------ [2021-08-27 15:42:35] nikic@php.net It might also make sense to just artificially limit the supported encodings to something closer to PHP-8.0. This functionality has close to zero test coverage right now... ------------------------------------------------------------------------ [2021-08-27 15:32:09] nikic@php.net > Unless we want to bias detection towards ASCII rather than CJK After thinking about this a bit more, I think we might want to do something like this. I just tried detecting UTF-16LE and UTF-16BE, and this basically ends up picking whichever is first even if the string is very "obviously" one or the other based on null bytes. The reverse order can still be meaningfully interpreted as CJK code points. ------------------------------------------------------------------------ [2021-08-27 13:09:52] nikic@php.net There are multiple issues here: 1. We should consider illegal trailing characters in non-strict mode if there are still multiple eligible encodings. Fixed in https://github.com/php/php-src/commit/43cb2548f7fd09ac3471bd71c5d28fbeaa312f2d. This prevents detection of UCS-2 for "test:test", which has an incomplete last character. 2. Some of the "special" encodings currently don't do strict validation. uuencode is one of those and thus ends up accepting everything. The filter should probably be fixed, though I believe we want to drop support for these "encodings" anyway. 3. However, the real issue here is user error. You're throwing a big bucket of ambiguous encodings at mbstring, and asking it to pick something. This kinda worked out before because in PHP 8.0 only a limited set of encodings supported encoding detection. In PHP 8.1 all encodings support detection, including multi-byte encodings. The string "test:tes" looks nice as UTF-8, but is also "瑥獴㩴敳" in UCS-2. Unless we want to bias detection towards ASCII rather than CJK, both of these are sensible choices. UCS-2 is earlier in the encoding list, and doesn't have punctuation besides. If you want to limit detection to only UTF-8 and ISO-8859 style encodings, then you should specify that in your encoding list. If you don't want to get back UCS-2 for something that is valid UCS-2, don't specify UCS-2. I think the only thing we could do here is to exclude various special encodings from detection (like https://gist.github.com/nikic/7cab20f7286c2b9437276c4fa43f6fb4), but apart from that I think that things are working correctly here. Maybe Alex has some more thoughts on this. ------------------------------------------------------------------------ [2021-08-27 09:12:12] cmb@php.net Even mb_check_encoding() fails in the same way: <https://3v4l.org/klWd0/rfc>. ------------------------------------------------------------------------ [2021-08-26 17:20:40] alec at alec dot pl $test = 'test:test'; $encodings = array_diff(mb_list_encodings(), ['UUENCODE', 'wchar']); echo mb_detect_encoding($test, $encodings, true); returns HTML-ENTITIES, this makes no sense. ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=81390 -- Edit this bug report at https://bugs.php.net/bug.php?id=81390&edit=1