[Form] Fix ChoiceType to effectively set and use translator #41534
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
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! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
|
Hey! I think @fancyweb has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
00dc970 to
8a3d977
Compare
8a3d977 to
f378d8f
Compare
…ator ChoiceType constructor line order changed to set translator before early return; CoreExtension injects translator to ChoiceType
f378d8f to
2fdec3e
Compare
|
@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. |
|
@stof Because we are trying to limit the changes done in already released branches. |
|
Thank you @marek-binkowski-sim. |
[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]
[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:
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.
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: