8000 [RFC] Split the Intl polyfill from the Intl CLDR features · Issue #37758 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content
8000

[RFC] Split the Intl polyfill from the Intl CLDR features #37758

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
stof opened this issue Aug 6, 2020 · 8 comments · Fixed by symfony/polyfill#310 or #38974
Closed

[RFC] Split the Intl polyfill from the Intl CLDR features #37758

stof opened this issue Aug 6, 2020 · 8 comments · Fixed by symfony/polyfill#310 or #38974
Labels
Help wanted Issues and PRs which are looking for volunteers to complete them. Intl RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@stof
Copy link
Member
stof commented Aug 6, 2020 •
edited by javiereguiluz
Loading

Description
The symfony/intl package provides 2 independent sets of features:

  • polyfills for various ext-intl classes exposing ICU features (IntlDateFormatter, IntlNumberFormatter, Collator and Locale)
  • public APIs exposing some CLDR data that is not available through ext-intl. That's the \Symfony\Component\Intl\Intl class in older Symfony versions, and its replacements in newer versions (\Symfony\Component\Intl\{Currencies,Locales,Locale,Languages,Countries,Scripts,Timezones})

When we created the symfony polyfills (many years later), we created a symfony/polyfill-intl-icu, which is semantically meant to provide the first half, but is actually implemented by requiring symfony/intl. This has several effects:

  • it does not make it clear what the intended usage is (in the modern way, packages needing only polyfills for ICU classes should depend on symfony/polyfill-intl-icu)
  • packages requiring symfony/polyfill-intl-icu actually receive the full symfony/intl package, which is by far the biggest contributor to the size of vendors among Symfony components (and bigger than many packages in the ecosystem) due to the size of the CLDR data, which is not needed for their use case.
  • polyfills are using a very specific architecture (all the implementation goes in an internal abstract class used as parent), which is very common in symfony/polyfills but can be confusing when finding it in symfony/symfony (see [Intl] IntlDateFormatter is abstract, yet it promotes instantiation #37756 for instance, which is caused by such confusion)
  • if you use the replace trick to remove unnecessary polyfills because your project has a requirement on ext-intl, you will still get the polyfill implementation in your vendors (which might confuse the IDE due to double class definitions)

Proposal
My proposal is to actually move the implementation of ICU polyfills to symfony/polyfill-intl-icu directly (the only change needed would be the namespace used for internal classes), and then change the order of the dependency. symfony/polyfill-intl-icu would be standalone, and symfony/intl would require symfony/polyfill-intl-icu (with the new version containing the polyfill as lowest bound).
This would give clear responsibilities to both packages, one providing the polyfill and the other one providing extra features.

The only impact would be on projects requiring symfony/polyfill-intl-icu and actually using the symfony/intl APIs without noticing they don't define a requirement for them, as symfony/intl might not be installed anymore. But the symfony/polyfill-intl-icu package does not document that these APIs become available due to the implementation details.
@stof stof added Intl RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Aug 6, 2020
@nicolas-grekas
Copy link
Member

That'd make sense, help wanted!

@nicolas-grekas nicolas-grekas added the Help wanted Issues and PRs which are looking for volunteers to complete them. label Aug 6, 2020
@stof
Copy link
Member Author
stof commented Aug 6, 2020 •
edited
Loading

Do we want to make some complex git magic to preserve the git history of the various polyfill files in the polyfill repository (this would probably result into a branch starting from an separate root in the history, and merged when merging the code, as we don't want to rewrite the history of symfony/polyfill) or are we OK with a dumb copy in the new repo resulting into a single import commit (which might reference the commit from symfony/symfony from which it is imported and allowing to find the older history) ?

@stof
Copy link
Member Author
stof commented Aug 6, 2020 •
edited
Loading

There is one thing I wonder: to avoid duplicate class definitions, we might need to make symfony/polyfill-intl-icu 1.19+ (or whatever the release including this ends up being) conflict with symfony/intl versions shipping the polyfill. If that's indeed the case (I need to check the behavior of Composer 1.x and 2.x regarding this to confirm it), would it be OK to apply the change in 3.4 so that all maintained branches are updated to rely on symfony/polyfill-intl-icu for the polyfill implementation ?
AFAICT, the only changes between 3.4 and master in the polyfills part of symfony/intl are:

  • fixing the ::create method to use late static binding to create an instance of the child class. This is actually a bug fix, as \Collator::create() would not create a \Collator instance when using the polyfill from 3.4 but an instance of the internal parent class (same for formatters). I don't know why this has not been applied in older branches but it looks like a mistake.
  • marking the internal implementation classes abstract (which is probably how the previous bug was discovered), which is OK as they are internal and they would change namespace anyway in the symfony/polyfill repo.
  • adding scalar typehints in the methods. This should probably not be kept in symfony/polyfill to keep supporting older PHP version (the current behavior is that it potentially installs an unmaintained version of symfony/intl on old PHP versions)

@stof
Copy link
Member Author
stof commented Sep 16, 2020

@nicolas-grekas any answer ?

@nicolas-grekas
Copy link
Member

I gave this a try. I have a locally rewritten history of the polyfill classes. But I have one blocker: NumberFormatter uses the data from Currencies, which is only found in symfony/intl.
Looks like a dead end. WDYT?

@nicolas-grekas nicolas-grekas mentioned this issue Oct 31, 2020
Merged
2 tasks
@nicolas-grekas
Copy link
Member

See symfony/polyfill#310

@stof
Copy link
Member Author
stof commented Oct 31, 2020

@nicolas-grekas the fallback NumberFormatter implements English only, and only needs 2 info about the currencies: the symbol and the fraction digits. We could bundle that (much smaller) info in the polyfill package (without exposing it to the outside, as that's what Currencies from symfony/intl does).

@stof
Copy link
Member Author
stof commented Nov 3, 2020

@nicolas-grekas should this really be closed ? I think it requires changes in symfony/symfony too.

@nicolas-grekas nicolas-grekas mentioned this issue Nov 3, 2020
Merged
nicolas-grekas added a commit that referenced this issue Nov 11, 2020
@nicolas-grekas
feature #38974 [Intl] deprecate polyfills in favor of symfony/polyfil…
7e3855d 
…l-intl-icu (nicolas-grekas)

This PR was merged into the 5.x branch.

Discussion
----------

[Intl] deprecate polyfills in favor of symfony/polyfill-intl-icu

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | Fix #37758
| License       | MIT
| Doc PR        | -

Follows symfony/polyfill#310
/cc @stof

Commits
-------

6ad0169 [Intl] deprecate polyfills in favor of symfony/polyfill-intl-icu
@javiereguiluz javiereguiluz mentioned this issue Dec 18, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Help wanted Issues and PRs which are looking for volunteers to complete them. Intl RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Milestone
No milestone
2 participants
@nicolas-grekas @stof
0