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

[Form] remove useless copy in ChoiceType#18316

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:2.7fromHeahDude:minor-choice_type-remove_code

Mar 30, 2016

Merged

[Form] remove useless copy in ChoiceType#18316

fabpot merged 1 commit intosymfony:2.7fromHeahDude:minor-choice_type-remove_code

Mar 30, 2016

Conversation

Copy link

Contributor

HeahDude commentedMar 26, 2016

Q	A
Branch?	2.7+
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	symfony/symfony-docs#6393

ChoiceListFactoryInterface expected$groupBy to be a callable or null, not an array (ref).

The factory defaultsgroupBy toChoiceListInterface::getStructuredValues() (ref).

Hence, the copy of the choices array and the recursive flip are useless and may be slowing down performances imho (especially withEntityType).

[Form] remove useless code in ChoiceType

562f5e4

`ChoiceListFactoryInterface` expected `$group_by` to be a callable ornull not an array.The factory defaults `group_by` to`ChoiceListInterface::getStructuredValues`.

carsonbot added the Status: Needs Review label

Mar 26, 2016

Copy link

ContributorAuthor

HeahDude commentedMar 26, 2016

Note to the merger: sorry for the conflict in 3.0 :/

Copy link

Contributor

Tobion commentedMar 26, 2016

👍 Good catch. This piece of code does not seem to have any effect as agroup_by with an array is ignored.

Status: Reviewed

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

Mar 26, 2016

Copy link

ContributorAuthor

HeahDude commentedMar 26, 2016

@Tobion thanks for the review :) The effect I'm interested in is a comparison with backfire profiling!

Copy link

ContributorAuthor

HeahDude commentedMar 26, 2016

This is due tothis.

Copy link

ContributorAuthor

HeahDude commentedMar 26, 2016

Ref thischange in#15061.

javiereguiluz added the Form label

Mar 29, 2016

Copy link

Member

fabpot commentedMar 30, 2016

Thank you@HeahDude.

fabpot merged commit562f5e4 intosymfony:2.7

Mar 30, 2016

fabpot added a commit that referenced this pull request

Mar 30, 2016

minor#18316[Form] remove useless copy in ChoiceType (HeahDude)

2e6982a

This PR was merged into the 2.7 branch.Discussion----------[Form] remove useless copy in ChoiceType| Q             | A| ------------- | ---| Branch?       | 2.7+| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | n/a| License       | MIT| Doc PR        |symfony/symfony-docs#6393`ChoiceListFactoryInterface` expected `$groupBy` to be a callable or null, not an array ([ref](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/ChoiceList/Factory/ChoiceListFactoryInterface.php#L111)).The factory defaults `groupBy` to `ChoiceListInterface::getStructuredValues()` ([ref](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php#L122)).Hence, the copy of the choices array and the recursive flip are useless and may be slowing down performances imho (especially with `EntityType`).Commits-------562f5e4 [Form] remove useless code in ChoiceType