Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

mb_convert_encoding does not support windows-1250. #1037 #8327

Closed
wants to merge 1 commit into from
Closed

mb_convert_encoding does not support windows-1250. #1037 #8327

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/Model/IMAPMessage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,11 @@ private function loadBodyData(Horde_Mime_Part $p, $partNo): string {
// data is utf-8 by default.
$charset = $p->getContentTypeParameter('charset');
if ($charset !== null && strtoupper($charset) !== 'UTF-8') {
$data = mb_convert_encoding($data, 'UTF-8', $charset);
try {
$data = mb_convert_encoding($data, 'UTF-8', $charset);
} catch (\Throwable $ex) {
$data = iconv($charset, 'UTF-8//TRANSLIT', $data);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3908 was the older thread about //IGNORE issues.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
return (string)$data;
}
Expand Down
Loading