Intl: Add a new IntlListFormatter class

[BogdanUngureanu]

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:

$formatter = new IntlListFormatter('EN_US', IntlListFormatter::TYPE_AND, IntlListFormatter::WIDTH_WIDE);
echo $formatter->format([1,2,3]);

will display

1, 2, and 3

I have some questions here:

• Do I need to create an RFC for this new addition?
• I haven't used a namespace and used Intl prefix for consistency sake. Should it be behind a Intl namespace? I haven't seen a class that uses it so I thought I should go with the prefix.

[TimWolla]

Do I need to create an RFC for this new addition?

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:

• Making the class final and /** @not-serializable */. This allows you to avoid the “not properly constructed” checks.
• /** @strict-properties */ to further lock down the class against misuse.

I haven't used a namespace and used Intl prefix for consistency sake. Should it be behind a Intl namespace? I haven't seen a class that uses it so I thought I should go with the prefix.

The prefix makes sense to me. Using the Intl namespace should properly happen when designing an Intl API that isn't just a straight forward mapping to ICU, but also provides a nice API (e.g. enums, proper class and exception hierarchy, …).

[BogdanUngureanu]

Hi @TimWolla! Thanks for the suggestions - great ideas! I've pushed a commit that makes the class final and enabled those flags.

[TimWolla]

Some more Stub nits. Please do not add PHPDoc comments, unless they allow you to express something that the types themselves cannot.

[BogdanUngureanu]

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. :)

[devnexen]

windows CI failure is related otherwise

[devnexen]

nit: do you get a warning during your build if you remove the (void *) cast ? unsure it s really necessary.

[BogdanUngureanu]

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]

[devnexen]

the const qualifier ... can items just be mutable and eventually doing the cast at ulistfmt_format ?

[BogdanUngureanu]

hmm... I'm not sure if that's simpler because we also need to convert it to utf16

[devnexen]

As first round of reviews, look good, I ll play a bit with your branch I think tomorrow.

[BogdanUngureanu]

@kocsismate no worries, I'm able to work on it only in the evenings anyway. :)

[devnexen]

seems the most important points had been covered 👍🏼, I ll look at it sometime this week-end.

[ndossche]

LGTM, wait for others for final confirmation

[ndossche]

One optimization that you could do (optional) is using zval_get_tmp_string instead. Although it probably doesn't matter too much here

[devnexen]

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.

[devnexen]

Note I m not holding the PR merge at all, just asking out of curiosity/trying to see a possible future improvement :)

[BogdanUngureanu]

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 :)

[devnexen]

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 ;)

[devnexen]

good as is, played long enough with your branch. @TimWolla to double check the stubs parts.

[TimWolla]

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).

[BogdanUngureanu]

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? :)

[devnexen]

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.

[BogdanUngureanu]

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 :)

[devnexen]

Thanks !

[kocsismate]

Suggested change

	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)

[BogdanUngureanu]

@kocsismate I've opened #19282 for it.