Skip to content

[Intl] Simplify API #28846

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
Merged
merged 1 commit into from
Apr 15, 2019
Merged

[Intl] Simplify API #28846

merged 1 commit into from
Apr 15, 2019

Conversation

ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Oct 12, 2018 •
edited
Loading

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.

Thoughts?

@ro0NL ro0NL mentioned this pull request Oct 16, 2018
Closed
@nicolas-grekas nicolas-grekas added this to the next milestone Oct 17, 2018
@ro0NL ro0NL mentioned this pull request Dec 13, 2018
Merged
nicolas-grekas
nicolas-grekas reviewed Mar 16, 2019
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ro0NL
Copy link
Contributor Author

ro0NL 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 mentioned this pull request Mar 25, 2019
Closed
webmozart
webmozart suggested changes Apr 6, 2019
View reviewed changes
Copy link
Contributor

@webmozart webmozart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ro0NL
Copy link
Contributor Author

ro0NL commented Apr 8, 2019

@webmozart thanks for review, it's done :) i was a bit confused by locales vs. currency codes (e.g. not locale codes nor currencies). But "code" is indeed part of their domain terminology, so is language code, script code etc. But not locales :)

Last but not least for local variables, e.g. $currency we mean the "code" and not the name or other. Works for me 👍

Ill fix the remaning deprecations asap.

@ro0NL ro0NL requested a review from xabbuh as a code owner April 8, 2019 18:17
@webmozart
Copy link
Contributor

@ro0NL Exactly! :) It takes some wrapping your mind around, but once you understand the domain language of CLDR, this naming makes total sense.

As for $currency et al., I'd even go as far as naming that $currencyCode (otherwise - is it the code? The name? A Currency object?). But I know this may go a little far and am really fine with either way. :)

@ro0NL ro0NL mentioned this pull request Apr 8, 2019
Closed
@ro0NL ro0NL changed the title [Intl][WIP] Simplify API [Intl] Simplify API Apr 8, 2019
@ro0NL
Copy link
Contributor Author

ro0NL commented Apr 11, 2019

status: ready :)

javiereguiluz
javiereguiluz approved these changes Apr 11, 2019
View reviewed changes
Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this a lot! Thanks Roland.

@fabpot
Copy link
Member

fabpot commented Apr 15, 2019

Thank you @ro0NL.

@fabpot fabpot merged commit d6b67d4 into symfony:master Apr 15, 2019
fabpot added a commit that referenced this pull request Apr 15, 2019
@fabpot
feature #28846 [Intl] Simplify API (ro0NL)
5d3d1a0 
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 intl branch April 15, 2019 12:50
@ro0NL ro0NL mentioned this pull request Apr 28, 2019
Merged
fabpot added a commit that referenced this pull request Apr 29, 2019
@fabpot
bug #31301 [Intl] Fix LocaleDataGenerator (ro0NL)
73b0ca0 
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 mentioned this pull request May 9, 2019
Merged
@jzawadzki jzawadzki mentioned this pull request Nov 27, 2019
Merged
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Nov 29, 2019
@OskarStark
minor #12720 Changed names of Intl classes in form types docs (jzawad…
03f6f1f 
…zki)

This PR was submitted for the 5.0 branch but it was merged into the 4.3 branch instead (closes #12720).

Discussion
----------

Changed names of Intl classes in form types docs

As per symfony/symfony#28846 we changed Intl classes to be more specific. Looks like some docs left with old naming.

Commits
-------

f5f64e1 Changed names of Intl classes in form types docs
@TomasVotruba TomasVotruba mentioned this pull request Apr 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof requested changes

@fabpot fabpot fabpot approved these changes

@javiereguiluz javiereguiluz javiereguiluz approved these changes

@webmozart webmozart webmozart approved these changes

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
Feature Intl Status: Reviewed
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
7 participants
@ro0NL @webmozart @fabpot @javiereguiluz @nicolas-grekas @stof @carsonbot