Add Locale::getDisplayKeyword() and Locale::getDisplayKeywordValue()#22264
Conversation
|
Given the voting status of this RFC (17 agree; 0 disagree; 1 Abstain, ends in 6/23 22:00 UTC) It is very likely to be accepted. Thus, I am opening this PR to be ready for review. |
|
The RFC is now accepted (18 yes 0 no 2 abstain) |
|
I had another look, code wise it s good. the test coverage could be improved, e.g. ICU display values are hardcoded without version being guarded. |
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
Can't comment on the implementation itself or the tests, I abstained from the RFC because I didn't understand it.
| Z_PARAM_PATH_OR_NULL(disp_loc_name, disp_loc_name_len) | ||
| ZEND_PARSE_PARAMETERS_END(); | ||
|
|
||
| if (loc_name_len > ULOC_FULLNAME_CAPACITY) { |
There was a problem hiding this comment.
Nit: loc_name is bounded but keyword_name is not, in either branch. Unlike the locale-name overflow (bug 67397), uloc_getDisplayKeyword{,Value} grows the output buffer in the loop and echoes unknown keywords back, so I do not think an overlong keyword can crash. Noting it for symmetry; fine to leave as-is.
Now I change it to only verifies that the returned values are non-empty strings, and that the explicit(default) display locale paths agree I am merging this as the alpha 1 release is around the corner and it's good to get this in for public testing, since the implementation has been reviewed several times and is good enough code wise. We can adjust tests in the future (ideally I want to write new tests in bulk in the future because the test coverage of the intl extension is bad overall (82.97%)) |
|
I would have preferred if regexes, at least, were used in the tests instead. |
| ut_run(); | ||
| ?> | ||
| --EXPECTREGEX-- | ||
| string\([1-9][0-9]*\) "[^"\n]+" |
There was a problem hiding this comment.
I meant ; there is probably a safe subset of characters to expect within a value ?
There was a problem hiding this comment.
I see. "[A-Za-z]+( [A-Za-z]+)*" should be safer but it could only test the spaces 🤔
There was a problem hiding this comment.
not quite what I had in mind, e.g. previously was hardcoded "Gregorian Calendar", it might be safe to assume "Gregorian" token is going to be a safe assumption
Updated the Intl section to clarify the addition of new functions for retrieving display names for locale keywords and their values.
|
Thank you! |
5 participants
RFC: https://wiki.php.net/rfc/getdisplaykeyword_and_getdisplaykeywordvalue
No need to review until the voting process ends.