Edit report at https://bugs.php.net/bug.php?id=81390&edit=1
ID: 81390
Updated by: alexdowad@php.net
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:
Thanks to Alec for the report! Some comments:
The new legacy encoding detection code, which (as Nikita mentioned) is intended to work with all
supported encodings, uses a couple of simple heuristics:
- If the input string is not valid in a candidate encoding, that encoding is immediately rejected.
- When the input string is converted to a candidate encoding, each control character or codepoint
in Unicode's Private Use Area counts for 10 "demerits" against the candidate
- Each punctuation character counts for 1 "demerit", since punctuation is much less
common in natural language strings than letters (also, when Shift-JIS or ISO-2022 strings are
misinterpreted as ASCII, they tend to have large numbers of punctuation characters).
We can easily add more heuristics, and refine the existing ones. We will _never_ get anything close
to 100% accuracy; frankly, even with human intelligence, it is not always possible to figure out
what the intended encoding of some random string is.
We should favor heuristics which will improve detection accuracy in a wide range of situations, and
which are fast to evaluate.
Here are a couple of ideas:
- Consider completely banning uuencode, base64, QPrint, 'HTML entities',
'7-bit', and '8-bit' from being returned as the detected text encoding.
- (Line 46 of mbfl_encoding.h is interesting; it shows that the original author of MBString
recognized that these are not really 'text encodings' in the same sense that the other
supported encodings are.)
- Rank the other supported encodings according to the likelihood that they will be encountered
'in the wild', and favor those higher on the list when more than one candidate is
possible.
Here's another thought. Two common scenarios when encoding detection returns the wrong result:
1) The string is changed into all or almost all CJK characters, which are usually _very rare_ CJK
characters.
2) (This is what happens when a CJK string is mistakenly detected as being ASCII or a
'european' encoding) The string is changed into a mishmash of letters and punctuation,
with very few spaces.
This implies that it might be helpful to classify CJK characters as 'common' and
'rare', perhaps using something like a Bloom filter. For strings with a high proportion of
'european' characters, maybe we should expect a good number of spaces (unless the string
is just a single word).
I think that finding PUA codepoints is a fairly good indicator that a candidate encoding might be
wrong, but rather than the current test, we could simply check for a range of values (i.e. c >=
PUA_MIN && c <= PUA_MAX). This might help to speed things up, since we are also seeing
reports that the new encoding detection code is too slow for some users.
Previous Comments:
------------------------------------------------------------------------
[2021-08-27 17:02:28] alec at alec dot pl
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.
------------------------------------------------------------------------
[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>.
------------------------------------------------------------------------
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