[WIP] added back ICU classes from symfony/Icu #11884
Conversation
|
|
||
| public function __construct(StructuredBundleReaderInterface $reader) | ||
| { | ||
| $this->stubbed = IcuData::isStubbed(); |
There was a problem hiding this comment.
This is useless. IcuData::isStubbed() will always be true now, given there is a single version of the class
There was a problem hiding this comment.
ah no, I missed the class_alias hack replacing the class defined here by the IntlData one. But I find this quite weird
There was a problem hiding this comment.
That's weird. As we are going to remove the intl dep on IntlData, I will change this strategy.
|
I think the integration of IntlData should reworked to avoid the class_alias hack, which is not readable. It will be necessary to solve symfony/intl-data#1 anyway |
| @@ -24,14 +24,14 @@ | ||
| } | ||
| ], | ||
| "require": { | ||
| "php": ">=5.3.3", | ||
| "symfony/icu": "~1.0-RC" |
There was a problem hiding this comment.
I think we should insert a hard dependency for symfony/intl-data for all versions < 2.6. symfony/intl-data does not depend on ext-intl, so it's installable by anyone. Furthermore, the BC break (installing intl-data manually) is then fixed.
There was a problem hiding this comment.
I don't want to do that because it would mean that the size of a default Symfony installation will grow a lot. That's what we wanted to avoid by moving data into another repository.
There was a problem hiding this comment.
my suggestion would be to add the requirement in 2.3 to 2.5 to avoid breaking existing apps, and then removing it from 2.6, asking users to require it explicitly when they upgrade to the new Symfony version.
There was a problem hiding this comment.
@stof Yes, that's what I meant.
|
see #11920 |
This PR was merged into the 2.3 branch. Discussion ---------- [Intl] Integrated ICU data into Intl component #1 | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #11447, #10807 | License | MIT | Doc PR | - This PR is an alternative implementation to #11884. It depends on ~~#11906~~ and ~~#11907~~ being merged first (~~these are included in the diff until after a merge+rebase~~ merged+rebased now). With this PR, the ICU component becomes obsolete. The ICU data is bundled with Intl in two different formats: JSON and the binary ICU resource bundle format (version 2) readable by PHP's `\ResourceBundle` class. For a performance comparison between the two, see my [benchmark](/webmozart/json-res-benchmark). ~~The data is contained in two zip files: json.zip (2.6MB) and rb-v2.zip (3.8MB). The handler~~ ```php \Symfony\Component\Intl\Composer\ScriptHandler::decompressData() ``` ~~needs to be added as Composer hook and decompresses the data after install/update.~~ The data is included as text/binary now. Git takes care of the compression. Before this PR can be merged, I would like to find out what the performance difference between the two formats is in real applications. For that, I need benchmarks from some real-life applications which use ICU data - e.g. in forms (language drop-downs, country selectors etc.) - for both the JSON and the binary data. You can force either format to be used by hard-coding the return value of `Intl::detectDataFormat()` to `Intl::JSON` and `Intl::RB_V2` respectively. I'll also try to create some more realistic benchmarks. If JSON is not significantly slower/takes up significantly more memory than the binary format, we can drop the binary format altogether. Commits ------- be819c1 [Intl] Integrated ICU data into Intl component
The problem that this PR addresses is well explained in #11447 and the counterpart of this PR is the creation of the new
symfony/intl-datarepository, which is discussed in symfony/icu#12. As explained there, there is a BC break as people will now need to explicitly add thesymfony/intl-datadependency to get the ICU data.