mb_convert_encoding does not support windows-1250. #1037 #8327
Conversation
|
Thanks for opening your first pull request in this repository! ✌️ |
Astinus-Eberhard
force-pushed
the
bug/1037/iconv_as_fallback_for_mb_convert_encoding
branch
from
6903309 to
a47ca70
Compare
| try { | ||
| $data = mb_convert_encoding($data, 'UTF-8', $charset); | ||
| } catch (\Throwable $ex) { | ||
| $data = iconv($charset, 'UTF-8//TRANSLIT', $data); |
There was a problem hiding this comment.
is translit generally available or does it depend on the compilation options of iconv? we had UTF8//IGNORE in the past and it was troublesome
There was a problem hiding this comment.
No idea, but used a lot in so many projects & environments and I haven't spotted a single issue. I found both //TRANSLIT and //IGNORE options everywhere I read anything about Iconv. Honestly I've never even think this can failed (at least with clear from and to encodings and without autodetections).
There was some problems in some Iconv implementations, for example on some docker alphine images, but IMHO nothing so unusual & there was some fixes for it (mainly updates binaries from edges).
This PR is as a suggestion, if someone has a better experience with character encoding then please review and replace.
There was a problem hiding this comment.
#3908 was the older thread about //IGNORE issues.
There was a problem hiding this comment.
Honestly I see no risk here, cause this change does even matter only if there was an exception thrown, which is currently not handled properly, preventing some users from reading emails with corrupted encodings, which IMHO is more annoying than potential risk of misleading characters replacement. If under any environment this will fail, then again, now it is failing anyway too, so nothing change really.
There was a problem hiding this comment.
The risk is that there are possibly environments that currently work and will break with this change.
Could you check if Alpine's iconv handles //TRANSLIT without any issues?
#3908 (comment) for the specific issues we had earlier.
If translit works with all iconv implementations I'm okay with this change 👍
Thanks
|
When it willl be merged |
Astinus-Eberhard
force-pushed
the
bug/1037/iconv_as_fallback_for_mb_convert_encoding
branch
from
46947cb to
a47ca70
Compare
Signed-off-by: Astinus Eberhard <astinus.eberhard@gmail.com>
Astinus-Eberhard
force-pushed
the
bug/1037/iconv_as_fallback_for_mb_convert_encoding
branch
from
a47ca70 to
1230144
Compare
|
Does anyone think that this PR can fix my issue anonaddy/anonaddy#485 |
|
Closing due to lack of clarity regarding the support of //TRANSLIT across platforms #8327 (comment) |
No description provided.