Skip to content
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] Simplify API #28846

Merged
merged 1 commit into from Apr 15, 2019

Conversation

@ro0NL
Copy link
Contributor

commented Oct 12, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #18368
License MIT
Doc PR symfony/symfony-docs#11221

Simplifies the Intl API. It greatly reduces the no. of boilerplate classes in this component. Very over complicated, much wow :)

Solving (IMHO):

class LanguageBundle extends LanguageDataProvider implements LanguageBundleInterface

Which seems very over complicated just to provide static data.

// before
Intl::getLanguageBundle()->getLanguageName() // string | null

// after
Languages::getName() // string
Languages::exists() // bool

I left out Canonicalization on puropose, that's a new topic to me.

  • Languages
  • Locales
  • Currencies
  • Regions
  • Scripts
  • Timezones (#28831)
  • Update constraints
  • Update form types

Thoughts?

@nicolas-grekas
Copy link
Member

left a comment

Thanks, here are some random comments :)
Works for me generally speaking.
Would need some tests+UPGRADE/CHANGELOG entries

Show resolved Hide resolved src/Symfony/Component/Intl/AbstractStaticBundle.php Outdated
Show resolved Hide resolved src/Symfony/Component/Intl/AbstractStaticBundle.php Outdated
Show resolved Hide resolved src/Symfony/Component/Intl/AbstractStaticBundle.php Outdated
Show resolved Hide resolved src/Symfony/Component/Intl/Data/Provider/LanguageDataProvider.php Outdated
Show resolved Hide resolved src/Symfony/Component/Intl/Intl.php Outdated
Show resolved Hide resolved src/Symfony/Component/Intl/Intl.php Outdated
Show resolved Hide resolved src/Symfony/Component/Intl/Intl.php Outdated
Show resolved Hide resolved src/Symfony/Component/Intl/ResourceBundle/LanguageBundle.php Outdated
Show resolved Hide resolved src/Symfony/Component/Intl/ResourceBundle/LanguageBundleInterface.php Outdated

@ro0NL ro0NL force-pushed the ro0NL:intl branch 4 times, most recently from c10d200 to 30e6c9c Mar 17, 2019

@ro0NL ro0NL force-pushed the ro0NL:intl branch 10 times, most recently from f6cc336 to 33b9234 Mar 25, 2019

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

i've moved all data bundles, ready for first round of review.

The new intl-data tests are passing locally 👍, i've copied from Symfony\Component\Intl\Tests\Data\Provider so perhaps remove the legacy tests already? as they are somewhat big (but not sure we care).

@ro0NL ro0NL force-pushed the ro0NL:intl branch 2 times, most recently from 09d157d to 8294594 Mar 25, 2019

@webmozart
Copy link
Contributor

left a comment

Great work @ro0NL :) Thanks a lot!

I'd finish this PR as is (it's pretty much done) and add your further work to separate PRs.

Show resolved Hide resolved src/Symfony/Component/Intl/AbstractStaticBundle.php Outdated
Show resolved Hide resolved src/Symfony/Component/Intl/Currencies.php Outdated
Show resolved Hide resolved src/Symfony/Component/Intl/Languages.php Outdated
Show resolved Hide resolved src/Symfony/Component/Intl/Locales.php Outdated
Show resolved Hide resolved src/Symfony/Component/Intl/Locales.php Outdated
Show resolved Hide resolved src/Symfony/Component/Intl/Locales.php Outdated
Show resolved Hide resolved src/Symfony/Component/Intl/Regions.php Outdated
Show resolved Hide resolved src/Symfony/Component/Intl/Scripts.php Outdated

@ro0NL ro0NL changed the title [Intl][WIP] Simplify API [Intl] Simplify API Apr 8, 2019

@ro0NL ro0NL force-pushed the ro0NL:intl branch 5 times, most recently from e9c85f4 to a85fe85 Apr 8, 2019

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

status: ready :)

@javiereguiluz
Copy link
Member

left a comment

I like this a lot! Thanks Roland.

@fabpot

fabpot approved these changes Apr 15, 2019

@fabpot fabpot force-pushed the ro0NL:intl branch from e961c68 to d6b67d4 Apr 15, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

Thank you @ro0NL.

@fabpot fabpot merged commit d6b67d4 into symfony:master Apr 15, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 15, 2019

feature #28846 [Intl] Simplify API (ro0NL)
This PR was squashed before being merged into the 4.3-dev branch (closes #28846).

Discussion
----------

[Intl] Simplify API

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #18368
| License       | MIT
| Doc PR        | symfony/symfony-docs#11221

Simplifies the Intl API. It greatly reduces the no. of boilerplate classes in this component. Very over complicated, much wow :)

Solving (IMHO):

```php
class LanguageBundle extends LanguageDataProvider implements LanguageBundleInterface
```

Which seems very over complicated just to provide static data.

```php
// before
Intl::getLanguageBundle()->getLanguageName() // string | null

// after
Languages::getName() // string
Languages::exists() // bool
```

I left out Canonicalization on puropose, that's a new topic to me.

- [x] Languages
- [x] Locales
- [x] Currencies
- [x] Regions
- [x] Scripts
- [ ] Timezones (#28831)
- [x] Update constraints
- [x] Update form types

Thoughts?

Commits
-------

d6b67d4 [Intl] Simplify API

@ro0NL ro0NL deleted the ro0NL:intl branch Apr 15, 2019

fabpot added a commit that referenced this pull request Apr 29, 2019

bug #31301 [Intl] Fix LocaleDataGenerator (ro0NL)
This PR was merged into the 4.3-dev branch.

Discussion
----------

[Intl] Fix LocaleDataGenerator

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Forgotten in #28846

The `getName()` method for scripts/regions/languages is stilled needed during locale generation.

Commits
-------

137ce3f [Intl] Fix LocaleDataGenerator

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.