Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 2 commits intosymfony:masterfromyceruto:choice_translation_locale
Apr 22, 2018

Conversation

@yceruto
Copy link
Member

@ycerutoyceruto commentedApr 5, 2018•
edited by fabpot
Loading

QA
Branch?master (4.1)
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?yes
Tests pass?yes
Fixed tickets#26737
LicenseMIT
Doc PRsymfony/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 theChoiceLoaderInterface 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:

  • UpdateUPGRADE-*.md andsrc/**/CHANGELOG.md files.
  • Add some tests.

@ycerutoyceruto changed the titleAdd choice_translation_locale option for Intl choice types[Form] Add choice_translation_locale option for Intl choice typesApr 5, 2018
@ycerutoycerutoforce-pushed thechoice_translation_locale branch fromc81bf25 tob26715fCompareApril 5, 2018 21:37
@ogizanagiogizanagi added this to the4.2 milestoneApr 5, 2018
@ycerutoycerutoforce-pushed thechoice_translation_locale branch fromb26715f to1480bbdCompareApril 5, 2018 22:18
// BC layer
$self =clone$this;
$self->choiceLoader =$choiceLoader;

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

@ycerutoycerutoApr 8, 2018•
edited
Loading

Choose a reason for hiding this comment

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

mmh.. I guess that was what we did inhttps://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 overridingloadChoiceList() method without previous advice, no? ping@ro0NL ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Maybe to overcome preserve the legacy loader somehow, likereturn 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ogizanagiogizanagi left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this@yceruto :)

{
/**
*Country loaded choice list.
*BC layer.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

yceruto reacted with thumbs up emoji
// BC layer
$self =clone$this;
$self->choiceLoader =$choiceLoader;

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@ycerutoycerutoforce-pushed thechoice_translation_locale branch 2 times, most recently fromb4c0658 to59d4d00CompareApril 8, 2018 00:29
@yceruto
Copy link
MemberAuthor

@ogizanagi thank you for this review.

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

@yceruto
Copy link
MemberAuthor

Status: Needs Review

@yceruto
Copy link
MemberAuthor

Just squashing commits, ready on my side.

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

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

small CS change needed

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

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Fixed.

Form
----

* Deprecated`ChoiceLoaderInterface` implementation in`CountryType`,`LanguageType`,`LocaleType` and`CurrencyType`. Use the "choice_loader" option instead.
Copy link
Member

Choose a reason for hiding this comment

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

"choice_loader" should be enclosed by backticks too

Copy link
Member

Choose a reason for hiding this comment

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

Deprecated theChoiceLoaderInterface implementation [...]

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Fixed, thank you.

useSymfony\Component\OptionsResolver\OptionsResolver;

class CountryTypeextends AbstractTypeimplements ChoiceLoaderInterface
class CountryTypeextends AbstractType
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Reverted.

@ycerutoycerutoforce-pushed thechoice_translation_locale branch 2 times, most recently from82381e4 toe250dfaCompareApril 20, 2018 15:41
@yceruto
Copy link
MemberAuthor

Done.

@fabpotfabpotforce-pushed thechoice_translation_locale branch from3f197b6 to9592fa6CompareApril 22, 2018 06:17
@fabpotfabpot modified the milestones:next,4.1Apr 22, 2018
@fabpot
Copy link
Member

Thank you@yceruto.

@fabpotfabpot merged commit9592fa6 intosymfony:masterApr 22, 2018
fabpot added a commit that referenced this pull requestApr 22, 2018
…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#23629TODO:- [x] Update `UPGRADE-*.md` and `src/**/CHANGELOG.md` files.- [x] Add some tests.Commits-------9592fa6 moved feature to 4.1e250dfa Add choice_translation_locale option for Intl choice types
@ycerutoyceruto deleted the choice_translation_locale branchApril 22, 2018 16:13
@fabpotfabpot mentioned this pull requestMay 7, 2018
symfony-splitter pushed a commit to symfony/form that referenced this pull requestMay 29, 2019
…nterface in intl forms (yceruto)This PR was merged into the 5.0-dev branch.Discussion----------[Form] Remove deprecated implementation of ChoiceLoaderInterface in intl forms| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -See previous PRsymfony/symfony#26825 (4.1)Commits-------b22cbf41c0 Remove deprecated implementation of ChoiceLoaderInterface in intl forms
nicolas-grekas added a commit that referenced this pull requestMay 29, 2019
…nterface in intl forms (yceruto)This PR was merged into the 5.0-dev branch.Discussion----------[Form] Remove deprecated implementation of ChoiceLoaderInterface in intl forms| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -See previous PR#26825 (4.1)Commits-------b22cbf4 Remove deprecated implementation of ChoiceLoaderInterface in intl forms
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@xabbuhxabbuhxabbuh left review comments

@fabpotfabpotfabpot approved these changes

+2 more reviewers

@ro0NLro0NLro0NL left review comments

@ogizanagiogizanagiogizanagi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

6 participants

@yceruto@fabpot@ro0NL@xabbuh@ogizanagi@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp