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

[Form] Add choice_translation_locale option for Intl choice types #26825

Merged
merged 2 commits into from Apr 22, 2018

Conversation

Projects
None yet
6 participants
@yceruto
Contributor

yceruto commented Apr 5, 2018

Q A
Branch? master (4.1)
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #26737
License MIT
Doc PR symfony/symfony-docs#...

This PR adds possibility to show the list of elements for a custom locale different to the current one and proposes also deprecate the ChoiceLoaderInterface implementation from all of them, moving it to a separate class.

See related issue #26737 for full description and use-case.

After:

$formBuilder->add('country', CountryType::class, [
    'choice_translation_locale' => 'es',
]);

Alternative of #23629

TODO:

  • Update UPGRADE-*.md and src/**/CHANGELOG.md files.
  • Add some tests.

@yceruto yceruto changed the title from Add choice_translation_locale option for Intl choice types to [Form] Add choice_translation_locale option for Intl choice types Apr 5, 2018

@ogizanagi ogizanagi added this to the 4.2 milestone Apr 5, 2018

@ogizanagi ogizanagi added the Form label Apr 5, 2018

// BC layer
$self = clone $this;
$self->choiceLoader = $choiceLoader;

This comment has been minimized.

@yceruto

yceruto Apr 5, 2018

Contributor

Instead of deprecating each ChoiceLoaderInterface methods (trigerring a lot of messages) shouldn't be enough just one here? deprecating the usage of this form type as choice loader? wdyt?

This comment has been minimized.

@ogizanagi

ogizanagi Apr 7, 2018

Member

Can't we always return the new loader here and just add deprecations to the ChoiceLoaderInterface's methods without modifying them? We indeed target to remove the ChoiceLoaderInterface from all of these types, right?

This comment has been minimized.

@yceruto

yceruto Apr 8, 2018

Contributor

mmh.. I guess that was what we did in https://github.com/symfony/symfony/pull/23648/files#diff-c6198934d4f35bc102ff362d40b71285R61, but isn't it a BC break?

I mean, if we return the new loader now then, we're ignoring any custom behavior for someone overriding loadChoiceList() method without previous advice, no? ping @ro0NL ?

This comment has been minimized.

@ro0NL

ro0NL Apr 8, 2018

Contributor

I guess it's a pragmatic choice; switching an implementation detail. Technically no API was broken 😅

Maybe to overcome preserve the legacy loader somehow, like return new CallbackLegacyChoiceLoader($callable, $this).

we're ignoring any custom behavior for someone overriding loadChoiceList()

Im really not sure how big an issue this is, in practice. In theory you're right.

This comment has been minimized.

@ro0NL

ro0NL Apr 8, 2018

Contributor

That way you can check if a API method is overridden on the legacy loader and deprecate from there, while keep calling it.

This comment has been minimized.

@ogizanagi

ogizanagi Apr 8, 2018

Member

I agree with @ro0NL. Not sure it is worth it.

This comment has been minimized.

@yceruto

yceruto Apr 8, 2018

Contributor

Agree, in that case I prefer @ro0NL's version, I did the changes, thanks!

@ogizanagi

Thanks for taking care of this @yceruto :)

use Symfony\Component\OptionsResolver\OptionsResolver;
class CountryType extends AbstractType implements ChoiceLoaderInterface
{
/**
* Country loaded choice list.
* BC layer.

This comment has been minimized.

@ogizanagi

ogizanagi Apr 7, 2018

Member

By marking it @deprecated instead, even if it's private, would help a bit when reading these classes from an IDE as the property usages in this class will be striked out.

// BC layer
$self = clone $this;
$self->choiceLoader = $choiceLoader;

This comment has been minimized.

@ogizanagi

ogizanagi Apr 7, 2018

Member

Can't we always return the new loader here and just add deprecations to the ChoiceLoaderInterface's methods without modifying them? We indeed target to remove the ChoiceLoaderInterface from all of these types, right?

@yceruto

This comment has been minimized.

Contributor

yceruto commented Apr 8, 2018

@ogizanagi thank you for this review.

Status: Waiting consensus on depreciation path :)
Status: Needs Work

@yceruto

This comment has been minimized.

Contributor

yceruto commented Apr 8, 2018

Status: Needs Review

@yceruto

This comment has been minimized.

Contributor

yceruto commented Apr 18, 2018

Just squashing commits, ready on my side.

@fabpot

fabpot approved these changes Apr 20, 2018

small CS change needed

*/
public function loadChoiceList($value = null)
{
@trigger_error(sprintf('Method "%s" is deprecated since Symfony 4.2. Use "choice_loader" option instead.', __METHOD__), E_USER_DEPRECATED);

This comment has been minimized.

@fabpot

fabpot Apr 20, 2018

Member

I prefer one sentence for deprecations: ... since Symfony 4.2, use ...

This comment has been minimized.

@yceruto

yceruto Apr 20, 2018

Contributor

Fixed.

Form
----
* Deprecated `ChoiceLoaderInterface` implementation in `CountryType`, `LanguageType`, `LocaleType` and `CurrencyType`. Use the "choice_loader" option instead.

This comment has been minimized.

@xabbuh

xabbuh Apr 20, 2018

Member

"choice_loader" should be enclosed by backticks too

This comment has been minimized.

@xabbuh

xabbuh Apr 20, 2018

Member

Deprecated the ChoiceLoaderInterface implementation [...]

This comment has been minimized.

@yceruto

yceruto Apr 20, 2018

Contributor

Fixed, thank you.

use Symfony\Component\OptionsResolver\OptionsResolver;
class CountryType extends AbstractType implements ChoiceLoaderInterface
class CountryType extends AbstractType

This comment has been minimized.

@xabbuh

xabbuh Apr 20, 2018

Member

I would not drop the implements here. Technically, that's a BC break.

This comment has been minimized.

@yceruto

yceruto Apr 20, 2018

Contributor

Reverted.

@yceruto

This comment has been minimized.

Contributor

yceruto commented Apr 20, 2018

Done.

@fabpot

fabpot approved these changes Apr 22, 2018

@fabpot fabpot modified the milestones: next, 4.1 Apr 22, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Apr 22, 2018

Thank you @yceruto.

@fabpot fabpot merged commit 9592fa6 into symfony:master Apr 22, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 22, 2018

feature #26825 [Form] Add choice_translation_locale option for Intl c…
…hoice types (yceruto, fabpot)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[Form] Add choice_translation_locale option for Intl choice types

| Q             | A
| ------------- | ---
| Branch?       | master (4.2)
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #26737
| License       | MIT
| Doc PR        | symfony/symfony-docs#...

This PR adds possibility to show the list of elements for a custom locale different to the current one and proposes also deprecate the `ChoiceLoaderInterface` implementation from all of them, moving it to a separate class.

See related issue #26737 for full description and use-case.

After:
```php
$formBuilder->add('country', CountryType::class, [
    'choice_translation_locale' => 'es',
]);
```

Alternative of #23629

TODO:

- [x] Update `UPGRADE-*.md` and `src/**/CHANGELOG.md` files.
- [x] Add some tests.

Commits
-------

9592fa6 moved feature to 4.1
e250dfa Add choice_translation_locale option for Intl choice types

@yceruto yceruto deleted the yceruto:choice_translation_locale branch Apr 22, 2018

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment