Skip to content

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

Closed
micti wants to merge 4 commits into
php:masterfrom
micti:master
Closed

micti wants to merge 4 commits into
php:masterfrom
micti:master

Conversation

@micti

@micti micti commented Apr 5, 2015

Copy link
Copy Markdown
Contributor

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

@smalyshev

Copy link
Copy Markdown
Contributor

Could you please add a test?

@smalyshev smalyshev added the Bug label Apr 13, 2015
@micti

micti commented Apr 13, 2015

Copy link
Copy Markdown
Contributor Author

Just added test for 2 bugs #69374 & #69398

@weltling

Copy link
Copy Markdown
Contributor

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

Thanks.

@micti

micti commented Apr 11, 2016

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
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

weltling commented Jun 2, 2016

Copy link
Copy Markdown
Contributor

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