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.

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

[Intl] New Intl API #9206

Closed
wants to merge 48 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@webmozart
Contributor

webmozart commented Oct 3, 2013

Q A
Bug fix? yes
New feature? no
BC breaks? yes
Deprecations? yes
Tests pass? yes
Fixed tickets #5802
License MIT
Doc PR TODO

The following description is outdated. This PR has turned into an overhaul of the Intl API to basically rebuild ICU4J/ICU4C (the official ICU APIs) under PHP. Once I'm done, I'll update the text below.

This PR contains a new, extensive integration test suite for the Intl component that check whether the methods of the resource bundles work correctly and consistently in practice. When I wrote this test suite, I discovered numerous bugs that have been fixed in this PR.

I had to do two major BC breaks:

  1. I changed the Intl::get*Bundle()->get*() methods to throw exceptions instead of just returning null on errors. Otherwise it would have been impossible to fix the existing bugs. Consequently, anybody using these methods directly with invalid languages, locales etc. will now experience an uncaught exception. People using the Form layer only should not be affected.

  2. I added a new method getLocaleAliases() to LocaleBundleInterface.

I also did two BC breaks in our internal code, which shouldn't affect our users:

  1. I changed the default value of $fallback in the protected AbstractBundle::readEntry() method from false to true. The method is just a proxy to StructuredBundleReader::readEntry(), which also has the default true for this parameter. We had numerous bugs in the past that were caused by this parameter being false by default. This BC break obviously only affects people who extended AbstractBundle (I'd be surprised if anyone did).

  2. I removed the internal interfaces CompilationContextInterface and StubbingContextInterface.

The fixed bugs had mainly to do with locale aliases. When locales get renamed in ICU, the old locale remains as an alias for (think: pointer to) the new locale. For example, the locale "mo" is an alias for "ro_MD". That locale falls back to "ro" if entries can't be found. Previously, the fallback from "mo" to "ro" did not work since aliases were ignored.

Additionally, the .res files for aliases (i.e. pointers to the new actual locales) were not generated for the LocaleBundle, so locale aliases there could not be accessed at all (e.g. Intl::getLocaleBundle()->getLocaleName('en', 'mo') -> bang).

The PR also contains improvements in efficiency. When accessing the Intl::get*Bundle()->getLocales(), the bundles won't scan the filesystem for available locales anymore (slow), but read a cached entry from a pregenerated misc.res file (speedy gonzales).

The new test suite (see the *ConsistencyTest) is a very nice documentation of the current state of the ICU data, so go and check it out. Some tests are very slow, so I created a new group icu-consistency for them, which is excluded by default.

TODO:

  • Test and upgrade symfony/Icu 1.2.x
  • Test and upgrade symfony/Icu 1.1.x
  • Test and upgrade symfony/Icu 1.0.x
  • Make sure no BC breaks leak through to the Form component
  • Update Intl component documentation
*/
public function __construct($genrb = 'genrb', $envVars = '')
{
exec('which ' . $genrb, $output, $status);

This comment has been minimized.

@stloyd

stloyd Oct 3, 2013

Contributor

Why not using symfony/process?

This comment has been minimized.

@webmozart

webmozart Oct 3, 2013

Contributor

Good question :) but out of scope of this PR (this class was just renamed, the above code remained the same).

@stloyd

View changes

src/Symfony/Component/Intl/ResourceBundle/Transformer/Rule/CurrencyBundleTransformationRule.php Outdated
@@ -37,16 +50,44 @@ public function getBundleName()
*/
public function beforeCompile(CompilationContextInterface $context)
{
$tempDir = sys_get_temp_dir().'/icu-data-currencies-source';

This comment has been minimized.

@stloyd

stloyd Oct 3, 2013

Contributor

I guess you should use realpath() to prevent issues with some OSX.

This comment has been minimized.

@webmozart

webmozart Oct 4, 2013

Contributor

Can you explain these issues?

if (!$array instanceof \ArrayAccess && !is_array($array)) {
return null;
// Use array_key_exists() for arrays, isset() otherwise
if (is_array($array) && !array_key_exists($index, $array) || !is_array($array) && !isset($array[$index])) {

This comment has been minimized.

@stloyd

stloyd Oct 3, 2013

Contributor

Did you mean:

if ((is_array($array) && !array_key_exists($index, $array)) || (!is_array($array) && !isset($array[$index]))) {

This comment has been minimized.

@webmozart

webmozart Oct 4, 2013

Contributor

|| has a lower precedence than &&. The two examples are equivalent.

@stof

View changes

src/Symfony/Component/Intl/Intl.php Outdated
}
return self::$currencyBundle;
return static::$currencyBundle;

This comment has been minimized.

@stof

stof Oct 3, 2013

Member

Using late static binding is a bad idea as the property is private.

If someone overwrite the class and use the extended one, he will have to redefine all static properties because ExtendedIntl would not be allowed to access the private property of the parent class

This comment has been minimized.

@stof

stof Oct 3, 2013

Member

and the same is of course true for all other properties of the class

This comment has been minimized.

@webmozart

webmozart Oct 4, 2013

Contributor

Thank you! I didn't know that.

* @return array An array with aliases as keys and aliased locales as
* values.
*/
public function getLocaleAliases();

This comment has been minimized.

@stof

stof Oct 3, 2013

Member

this should be mentionned as a BC break too

This comment has been minimized.

@webmozart

webmozart Oct 4, 2013

Contributor

documented

@stof

View changes

src/Symfony/Component/Intl/ResourceBundle/Transformer/CompilationContext.php Outdated
*/
private $localeScanner;
public function __construct($sourceDir, $binaryDir, Filesystem $filesystem, BundleCompilerInterface $compiler, LocaleScanner $localeScanner, $icuVersion)

This comment has been minimized.

@stof

stof Oct 3, 2013

Member

this is a BC break

@stof

View changes

src/Symfony/Component/Intl/ResourceBundle/Transformer/CompilationContextInterface.php Outdated
*
* @return \Symfony\Component\Intl\ResourceBundle\Scanner\LocaleScanner The locale scanner.
*/
public function getLocaleScanner();

This comment has been minimized.

@stof

stof Oct 3, 2013

Member

this is also a BC break (but it may be OK as this code is only used to update the icu component IIRC)

webmozart added some commits Oct 2, 2013

[Intl] Changed Intl methods to throw NoSuchEntriesExceptions when non…
…-existing languages, currencies, etc. are accessed
[Intl] The available locales of each resource bundle are now stored i…
…n a generic misc.res file to improve reading performance
[Intl] Improved LocaleBundleTransformationRule to not generate duplic…
…ate locale names when fallback (en_GB->en) is possible
[Intl] StructuredBundleReader now catches NoSuchLocaleExceptions and …
…continues at fallback locale (unless disabled)
@pvgnd

This comment has been minimized.

pvgnd commented Dec 16, 2013

What is the status of this overhaul ?
Is there a workaround on Symfony 2.3 or Symfony 2.4 to be able to use Intl ?

@stof

This comment has been minimized.

Member

stof commented Jan 24, 2014

@bschussek can you comment about the status ?

And it will need to be rebased

@webmozart

This comment has been minimized.

Contributor

webmozart commented Jan 24, 2014

@stof Currently on hold due to other, more important, issues. I'd however be interested in feedback about the new API. I designed the API to reflect ICU4Js API (more or less), i.e. classes with static methods, such as:

$currencies = Currency::getCurrencies();
$names = Currency::getNames();

if (Language::exists('de_DE')) // etc.

Now we all know that static methods are boo. However, these methods are used to access "global system information", so I'm not sure whether it makes sense to always pass objects around for accessing this global information.

Opinions?

/cc @pierrejoye

*
* @throws InvalidArgumentException If the currency or the locale is invalid
*
* @api

This comment has been minimized.

@stof

stof Jan 24, 2014

Member

All the @api tags should be removed IMO. We should not decide to enforce full BC on them before they are even final.

*/
public static function exists($currency)
{
if (null === self::$lookupTable) {

This comment has been minimized.

@stof

stof Jan 24, 2014

Member

self::$lookupTable is not defined

@stof

This comment has been minimized.

Member

stof commented Jan 24, 2014

@bschussek Teh issue I see with using a static API is that it means that code using it cannot be tested in isolation anymore, because you cannot mock the static calls being done in the middle.

@fabpot

This comment has been minimized.

Member

fabpot commented Mar 3, 2014

@webmozart If we want this PR to be included in 2.5, we should finish it asap.

@pierrejoye

This comment has been minimized.

pierrejoye commented Mar 3, 2014

@stof what is the problem actually? I do not see much point to create an instance for only checking the existence of a locale (or other ICU related data).

@webmozart webmozart added the Feature label Aug 5, 2014

webmozart added a commit that referenced this pull request Sep 12, 2014

bug #11905 [Intl] Removed non-working $fallback argument from ArrayAc…
…cessibleResourceBundle (webmozart)

This PR was merged into the 2.3 branch.

Discussion
----------

[Intl] Removed non-working $fallback argument from ArrayAccessibleResourceBundle

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

The code in question didn't actually work. This was extracted from #9206.

Commits
-------

5feda5e [Intl] Removed non-working $fallback argument from ArrayAccessibleResourceBundle

webmozart added a commit that referenced this pull request Sep 12, 2014

minor #11914 [Intl] Added exception handler to command line scripts (…
…webmozart)

This PR was merged into the 2.3 branch.

Discussion
----------

[Intl] Added exception handler to command line scripts

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Extracted from #9206.

Commits
-------

9052efc [Intl] Added exception handler to command line scripts

webmozart added a commit that referenced this pull request Sep 12, 2014

minor #11913 [Intl] Updated icu.ini up to ICU 53 (webmozart)
This PR was merged into the 2.3 branch.

Discussion
----------

[Intl] Updated icu.ini up to ICU 53

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Extracted from #9206.

Commits
-------

260e2fe [Intl] Updated icu.ini up to ICU 53

webmozart added a commit that referenced this pull request Sep 15, 2014

bug #11906 [Intl] Fixed a few bugs in TextBundleWriter (webmozart)
This PR was merged into the 2.3 branch.

Discussion
----------

[Intl] Fixed a few bugs in TextBundleWriter

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

See the included test cases for more information. This code was extracted from #9206.

Commits
-------

7b4a35a [Intl] Fixed a few bugs in TextBundleWriter

webmozart added a commit that referenced this pull request Sep 15, 2014

bug #11907 [Intl] Improved bundle reader implementations (webmozart)
This PR was merged into the 2.3 branch.

Discussion
----------

[Intl] Improved bundle reader implementations

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

This PR extracts bundle reader improvements from #9206.

The code is internal and used for resource bundle generation only, so I did not care about BC too much.

Commits
-------

c3cce5c [Intl] Improved bundle reader implementations
@fabpot

This comment has been minimized.

Member

fabpot commented Dec 24, 2014

@webmozart Is it still relevant?

@fabpot fabpot closed this Feb 5, 2015

@webmozart webmozart referenced this pull request Mar 30, 2016

Open

Simplify access to CLDR/ICU data #18368

0 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment