Skip to content

Conversation

@micti
Copy link
Contributor

@micti micti commented Apr 5, 2015

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

@smalyshev
Copy link
Contributor

Could you please add a test?

@smalyshev smalyshev added the Bug label Apr 13, 2015
@micti
Copy link
Contributor Author

micti commented Apr 13, 2015

Just added test for 2 bugs #69374 & #69398

@weltling
Copy link
Contributor

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

Thanks.

@micti
Copy link
Contributor Author

micti 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
Copy link
Contributor

@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
Copy link
Contributor

weltling 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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants