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.

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

Fix #69374 IntlDateFormatter::formatObject returns wrong utf8 value #1219

Closed
wants to merge 4 commits into from

Conversation

3 participants
@micti
Copy link
Contributor

commented Apr 5, 2015

We can use simple ICU UnicodeString constructor or UnicodeString::fromUTF8

@smalyshev

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2015

Could you please add a test?

@smalyshev smalyshev added the Bugfix label Apr 13, 2015

@micti

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2015

Just added test for 2 bugs #69374 & #69398

@weltling

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2016

The ext/intl/tests/bug69374.phpt fails for me. Why they are to skip for ICU < 50?

Thanks.

@micti

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2016

@weltling About ICU version, the test result is based on Unicode CLDR (ICU) version. At the time I found these bugs, I used ICU version > 50, so I skipped older version, I'm not sure about older version result. Thanks.

@weltling

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2016

@micti fine then. I was checking with 57.1 where it fails. It could be good that the data was changed (happens frequently with ICU updates). But anyway, the correct skipif part would look like if (version_compare(INTL_ICU_VERSION, '50.1.2') < 0) die('skip for ICU >= 51.1.2'); (thus, from 50 and up).

From the test, this is what i get with your patch

001+ tháng 04, 2015
002+ 2015ë…„ 4월
001- tháng 04, 2015
002- 2015년 4월

When i switch to UnicodeString::fromUTF8, but that's most likely due to the data changes in ICU.

002+ 2015년 Apr
002- 2015년 4월

What concerns me is, that we currently don't enforce UTF-8 with ICU (though it is a default encoding). So using fromUTF8 only is probably not a solution. And the first variant doesn't seem to work.

Maybe a solution could be to use fromUTF-8 first, and then fall back to the old variant on failure. But there are probably other draw backs we don't know about.

Thanks.

@weltling

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2016

Merged under usage of UnicodeString::fromUTF8(sp) with 0112b64 and 933de3a. We indeed force UTF-8 usage in many intl classes, and this seems to be the most convenient way here as well.

Thanks.

@weltling weltling closed this Jun 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.