Skip to content
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

[DoctrineBridge] Deprecated implicit optimization in DoctrineChoiceLoader #30962

Merged
merged 1 commit into from Apr 7, 2019

Conversation

Projects
None yet
7 participants
@HeahDude
Copy link
Member

commented Apr 7, 2019

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets ~
License MIT
Doc PR ~

#EUFOSSA

Big thanks to @stof for the help with writing the test!

Description

It happens that the IdReader is created by the DoctrineType and cached for each entity class case.
But the type already resolves whether or not it should use it, only when we can optimize query thanks to a single id field when defining the choice_value option: https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php#L188.

This PR is a first step to optimize the choice loading process.

@HeahDude HeahDude force-pushed the HeahDude:form/id-reader branch from a73bb62 to 7bb6fa7 Apr 7, 2019

@webmozart

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

Can you add some information on what the goal of this PR is?

@HeahDude HeahDude force-pushed the HeahDude:form/id-reader branch from 7bb6fa7 to 764599b Apr 7, 2019

@HeahDude

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2019

@webmozart I've updated the description.

@stof

This comment has been minimized.

Copy link
Member

commented Apr 7, 2019

@webmozart simplifying the DoctrineChoiceLoader in a follow-up PR refactoring the caching of choice loaders (which is currently broken for most cases except the doctrine one).
Anyone using the DoctrineType does not rely on this implicit optimization, as DoctrineType already does the check.

@stof

stof approved these changes Apr 7, 2019

@xabbuh xabbuh added this to the next milestone Apr 7, 2019

@HeahDude HeahDude force-pushed the HeahDude:form/id-reader branch from 764599b to 62fb01e Apr 7, 2019

@HeahDude HeahDude force-pushed the HeahDude:form/id-reader branch from 62fb01e to 287c39b Apr 7, 2019

@xabbuh

xabbuh approved these changes Apr 7, 2019

@fabpot

fabpot approved these changes Apr 7, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 7, 2019

Thank you @HeahDude.

@fabpot fabpot merged commit 287c39b into symfony:master Apr 7, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 7, 2019

feature #30962 [DoctrineBridge] Deprecated implicit optimization in D…
…octrineChoiceLoader (HeahDude)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[DoctrineBridge] Deprecated implicit optimization in DoctrineChoiceLoader

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | yes <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | ~
| License       | MIT
| Doc PR        | ~

#EUFOSSA

Big thanks to @stof for the help with writing the test!

## Description
It happens that the `IdReader` is created by the `DoctrineType` and cached for each entity class case.
But the type already resolves whether or not it should use it, only when we can optimize query thanks to a single id field when defining the `choice_value` option: https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php#L188.

This PR is a first step to optimize the choice loading process.

Commits
-------

287c39b [DoctrineBridge] Deprecated implicit optimization in DoctrineChoiceLoader

@HeahDude HeahDude deleted the HeahDude:form/id-reader branch Apr 7, 2019

stof added a commit that referenced this pull request Apr 7, 2019

minor #30966 [DoctrineBridge] Deprecated using IdReader when optimiza…
…tion is not possible (HeahDude)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[DoctrineBridge] Deprecated using IdReader when optimization is not possible

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | yes <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | ~
| License       | MIT
| Doc PR        | ~

Follow up of #30962. (Review only the second commit until #30962 is merged).

Commits
-------

a234c89 [DoctrineBridge] Deprecated using IdReader when optimization is not possible

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.