Intl: Add a new IntlListFormatter class#18519
Intl: Add a new IntlListFormatter class#18519BogdanUngureanu wants to merge 16 commits intophp:masterfrom
Conversation
I wouldn't need an RFC if this is a straight forward addition to the existing Intl API which just maps to ICU in a straight forward fashion. I would like to see some smaller improvements, though:
The prefix makes sense to me. Using the |
|
Hi @TimWolla! Thanks for the suggestions - great ideas! I've pushed a commit that makes the class final and enabled those flags. |
TimWolla
left a comment
There was a problem hiding this comment.
Some more Stub nits. Please do not add PHPDoc comments, unless they allow you to express something that the types themselves cannot.
Thanks for the quick response! I'll make the changes tomorrow. :) |
|
windows CI failure is related otherwise |
| intl_convert_utf8_to_utf16(&ustr, &ustr_len, ZSTR_VAL(str_val), ZSTR_LEN(str_val), &status); | ||
| if (U_FAILURE(status)) { | ||
| for (uint32_t j = 0; j < i; j++) { | ||
| efree((void *)items[j]); |
There was a problem hiding this comment.
nit: do you get a warning during your build if you remove the (void *) cast ? unsure it s really necessary.
There was a problem hiding this comment.
yep, I get a warning:
/listformatter_class.c:158:17: warning: passing 'const UChar *' (aka 'const unsigned short *') to parameter of type 'void *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
There was a problem hiding this comment.
the const qualifier ... can items just be mutable and eventually doing the cast at ulistfmt_format ?
There was a problem hiding this comment.
hmm... I'm not sure if that's simpler because we also need to convert it to utf16
|
As first round of reviews, look good, I ll play a bit with your branch I think tomorrow. |
|
@kocsismate no worries, I'm able to work on it only in the evenings anyway. :) |
|
seems the most important points had been covered 👍🏼, I ll look at it sometime this week-end. |
ndossche
left a comment
There was a problem hiding this comment.
LGTM, wait for others for final confirmation
| ZEND_HASH_FOREACH_VAL(ht, val) { | ||
| zend_string *str_val; | ||
|
|
||
| str_val = zval_get_string(val); |
There was a problem hiding this comment.
One optimization that you could do (optional) is using zval_get_tmp_string instead. Although it probably doesn't matter too much here
| int32_t resultLength; | ||
| UChar *result = NULL; | ||
|
|
||
| resultLength = ulistfmt_format(LISTFORMATTER_OBJECT(obj), items, itemLengths, count, NULL, 0, &status); |
There was a problem hiding this comment.
quick question, do you think there is a benefit of using ulistfmt_formatStringsToResult beside the "no need to calculate the buffer first" ? any drawback ? Think more of use case to apply in php rather than api conveniency. Cheers.
There was a problem hiding this comment.
Note I m not holding the PR merge at all, just asking out of curiosity/trying to see a possible future improvement :)
There was a problem hiding this comment.
I'm not sure what would be the advantage (besides not calculating the buffer). The drawback is that it's supported since ICU 64 and PHP Intl supports 57 and up. I guess we could conditionally use it for >= ICU 64, but personally I would keep the code simple :)
There was a problem hiding this comment.
I would say this ... ulistfmt_formatStringsToResult is not necessarily interesting by itself but what you can get out of it however is what is called field positions via a UFieldPositionIterator, going through it with ufieldpositer_next ... you can get nice additional infos ... but again my question was more to give food for thoughts ;)
TimWolla
left a comment
There was a problem hiding this comment.
Didn't check the details of the implementation, but the API looks good to me (it's consistent with the rest of ext/intl, just fixing some of the larger gotchas).
|
Hey everyone, thank you so much for the reviews! It's great seeing so many ✅! @devnexen what would be the next step to move forward with this PR? :) |
after the last nitpicks, that should be all if you want you can update UPGRADING yourself or I will. |
|
Alright, thanks @devnexen. I've pushed a commit that removes that condition and adds the php ending tag in phpt files. As for updating the UPGRADING, I'll leave it to you if you can :) |
|
Thanks ! |
| } | ||
|
|
||
| if (locale_len > INTL_MAX_LOCALE_LEN) { | ||
| zend_argument_value_error(1, "Locale string too long, should be no longer than %d characters", INTL_MAX_LOCALE_LEN); |
There was a problem hiding this comment.
| zend_argument_value_error(1, "Locale string too long, should be no longer than %d characters", INTL_MAX_LOCALE_LEN); | |
| zend_argument_value_error(1, "must be less than or equal to %d characters", INTL_MAX_LOCALE_LEN); |
(the new error message is negotiable, but the current message is slightly off)
This PR adds support for ICU's ListFormatter implementation by implementing a new IntlListFormatter class in PHP.
The class supports ANDs, ORs and UNIT format types. However, they are only available if ICU version is 67 or above. On older versions of ICU, we only allow TYPE_AND and WIDTH_WIDE - we need this because, from what I've understood, the minimum version ICU supported is 57, which was bumped from 50 last year.
The class API is pretty simple:
will display
I have some questions here: