[Intl] Add PHP 8.5 IntlListFormatter to ICU polyfill#532
[Intl] Add PHP 8.5 IntlListFormatter to ICU polyfill#532Ayesh wants to merge 1 commit intosymfony:1.xfrom
IntlListFormatter to ICU polyfill#532Conversation
Ayesh
force-pushed
the
feat/intllistformatter
branch
2 times, most recently
from
fa352fc to
3b446ce
Compare
|
// cc @BogdanUngureanu, I could use your inputs/review here :) |
| ]); | ||
| } | ||
|
|
||
| protected function getListPattern(): array { |
src/Intl/Icu/IntlListFormatter.php
Outdated
| private $width; | ||
|
|
||
| protected static $listPatterns = [ | ||
| 'en' => [ |
There was a problem hiding this comment.
Maybe make it clear that this is en_US since it's using the Oxford comma?
| ) { | ||
| $exceptionClass = PHP_VERSION_ID >= 80000 ? \ValueError::class : \InvalidArgumentException::class; | ||
| if ($locale !== 'en' && strpos($locale, 'en') !== 0) { | ||
| throw new $exceptionClass('Invalid locale, only "en" and "en-*" locales are supported'); |
There was a problem hiding this comment.
I have some mixed feelings about this. Since it's using the en-US set, for en-gb it would return a different result than the real class.
|
This should be added in symfony/polyfill-intl-icu (with the other formatters) instead of being a new package IMO
Extending the internal class is not covered by BC and is not a supported use case. |
|
@Ayesh do you plan to finish this PR (by taking review comments into account) ? |
|
Thank you for your review Christophe. I will try and submit fixed addressing the reviewed points by this weekend. |
Ayesh
force-pushed
the
feat/intllistformatter
branch
from
3b446ce to
ad6b26c
Compare
Ayesh
changed the title
IntlListFormatter as installable polyfillIntlListFormatter to ICU polyfill
Ayesh
force-pushed
the
feat/intllistformatter
branch
2 times, most recently
from
0518c84 to
c888ccc
Compare
|
Thank you for your reviews. I changed the PR, so that this now proposes to add I also made it so that the class is self-contained, and not extendable anymore (making |
| return strtr($pattern[2], ['{0}' => (string) $strings[0], '{1}' => (string) $strings[1]]); | ||
| } | ||
|
|
||
| if ($itemCount === 3) { |
There was a problem hiding this comment.
You could remove this case. It can be covered by the same code that for 4 items or more (the for loop will simply do no iterations, which is not an error)
| */ | ||
| class IntlListFormatterTest extends TestCase | ||
| { | ||
| public function testUnsupportedLocales() |
There was a problem hiding this comment.
Testing support locales should be done in a separate test than testUnsupportedLocales to make the intent of the tests clear.
Adds a new polyfill to `symfony/polyfill-intl-icu` that provides the functionality of the new `IntlListFormatter` to PHP 7.2 and later. - [ICU listPatterns](https://github.com/unicode-org/cldr-json/blob/main/cldr-json/cldr-misc-full/main/en/listPatterns.json) - [php-src commit](php/php-src@3f7545245) - [PHP.Watch: IntlListFormatter](https://php.watch/versions/8.5/IntlListFormatter) Closes symfonyGH-530.
Ayesh
force-pushed
the
feat/intllistformatter
branch
from
c888ccc to
0156b53
Compare
3 participants
Adds a new polyfill to
symfony/polyfill-intl-icuthat provides the functionality of the newIntlListFormatterto PHP 7.2 and later.Closes GH-530.