NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Form] Fix ChoiceType to effectively set and use translator#41534

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

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:5.3frommarek-binkowski-sim:ticket_41497

Oct 10, 2021

Merged

[Form] Fix ChoiceType to effectively set and use translator#41534

fabpot merged 1 commit intosymfony:5.3frommarek-binkowski-sim:ticket_41497

Oct 10, 2021

Conversation

Copy link

Contributor

marek-binkowski-sim commentedJun 3, 2021•
edited by fabpot
Loading

[Component][Form][ChoiceType] constructor line order changed to set translator before early return;

[Component][Form][CoreExtension] a second argument $translator has been added for instantiation of [Component][Form][ChoiceType]

Q	A
Branch?	5.2
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix#41497
License	MIT
Doc PR	-

[Component][Form][ChoiceType] The bug probably came to life after a merge of two commits from different branches: 5.1 and 4.4. The commit in branch 5.1 contained a conditional early return related with the choiceListFactory:

if ($this->choiceListFactoryinstanceof CachingFactoryDecorator) {return;}

The code from the branch 4.4 was there to set the translator, but it wasn't effective when the constructor early returned in the line above.

$this->translator =$translator;

The fix was to switch [Component][Form][ChoiceType] constructor lines related with the translator above the early return related with choiceListFactory.

As a second parameter $translator has been added to the [Component][Form][ChoiceType] constructor, the translator needs to be injected by the CoreExtension so that ChoiceType could set and use it. This is why a second argument is added now in [Component][Form][CoreExtension] loadTypes for Type\ChoiceType:

newType\ChoiceType($this->choiceListFactory,$this->translator),

marek-binkowski-sim requested review fromxabbuh andyceruto ascode owners

June 3, 2021 21:50

carsonbot added Status: Needs Review Bug Form labels

Jun 3, 2021

carsonbot added this to the5.2 milestone

Jun 3, 2021

Copy link

carsonbot commentedJun 3, 2021

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (seehttps://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
Features and deprecations must be submitted against the 5.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

marek-binkowski-sim mentioned this pull request

Jun 3, 2021

ChoiceType doesn't translate it's error messages#41497

Closed

marek-binkowski-sim changed the title~~[Form] Fix ChoiceType Extension to effectively set and use translator~~[Form] Fix ChoiceType to effectively set and use translator

Jun 3, 2021

Copy link

carsonbot commentedJun 4, 2021

Hey!

I think@fancyweb has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

Kocal reviewed

Jun 5, 2021

View reviewed changes

src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.phpShow resolvedHide resolved

xabbuh reviewed

Jun 6, 2021

View reviewed changes

src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.phpShow resolvedHide resolved

marek-binkowski-sim force-pushed theticket_41497 branch 2 times, most recently from00dc970 to8a3d977Compare

June 9, 2021 20:47

marek-binkowski-sim requested a review fromxabbuh

June 9, 2021 20:58

fabpot modified the milestones:5.2,5.3

Jul 26, 2021

fabpot requested changes

Aug 26, 2021

View reviewed changes

src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.phpShow resolvedHide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Aug 26, 2021

marek-binkowski-sim force-pushed theticket_41497 branch from8a3d977 tof378d8fCompare

October 5, 2021 17:35

[Form] Fix ChoiceType Extension to effectively set and use the transl…

2fdec3e

…atorChoiceType constructor line order changed to set translator before early return; CoreExtension injects translator to ChoiceType

marek-binkowski-sim force-pushed theticket_41497 branch fromf378d8f to2fdec3eCompare

October 5, 2021 17:45

xabbuh changed the base branch from5.2 to5.3

October 8, 2021 13:09

xabbuh approved these changes

Oct 8, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels

Oct 8, 2021

Copy link

Member

stof commentedOct 8, 2021

@fabpot why have you asked to revert the constructor typehint ? I don't see the reason to throw a TypeError ourselves instead of letting PHP doing it.