Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DoctrineBridge][Form] Fix performance regression in EntityType#17990
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
[DoctrineBridge][Form] Fix performance regression in EntityType#17990
Uh oh!
There was an error while loading.Please reload this page.
Conversation
A performance regression was introduced insymfony@2336d5cBefore, the default behaviour of the `DoctrineLoader` was toonly fetch the entities selected in the submitted form.After, the optimization was only performed when the `choice_value`option was set to `null`.However, the `DoctrineType` sets a non-null default value to `choice_value`,which means that the default behaviour was not using the optimizationanymore.This commit restores the default behaviour (while keeping the previouscommit intent).References:-https://github.com/symfony/symfony/blob/v2.7.10/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php#L149-https://github.com/symfony/symfony/blob/v2.7.10/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php#L216
| // Optimize performance in case we have an object loader and | ||
| // a single-field identifier | ||
| if (null ===$value && !$this->choiceList &&$this->objectLoader &&$this->idReader->isSingleId()) { | ||
| if (!($valueinstanceof \Closure) && !$this->choiceList &&$this->objectLoader &&$this->idReader->isSingleId()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 but this should be!is_callable($value) as it could be an array or a property path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@HeahDude It couldn't be a property path, since property paths are replaced by closures by PropertyAccessDecorator. However, it could be an array callable, hence your comment is legit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Understood! thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually, the new test doesn't pass with!is_callable($value), which means that it breaks the optimization.
That's because thedefault value set byDoctrineType is callable, so we wouldn't perform the optim by default.
I'm not sure that I really understandwhen we want the optimization to be performed. One thing I'm sure of is that if no value is passed tochoice_value, we want it. Based on this I tried the following
$isDefaultValue =is_array($value) &&is_callable($value) &&$value[0]instanceof \Symfony\Bridge\Doctrine\Form\ChoiceList\IdReader &&$value[1] ==='getIdValue';if ((null ===$value ||$isDefaultValue) && !$this->choiceList &&$this->objectLoader &&$this->idReader->isSingleId()) {// optimize !}
Which works, but is quite fragile I think.
Is there a better way to know that$value is actually the default value? Or a better way to know that the optimization can be performed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Since theIdReader is for internal use, I think the following should be enough :
$optimize =null ===$value || (isset($value[0]) &&$value[0]instanceof IdReader);if ($optimize && !$this->choiceList &&$this->objectLoader &&$this->idReader->isSingleId()) {// optimize !}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks it's working470f319 !
I added a check to know if$value is an array, and a variable for clarity.
| // Optimize performance in case we have an object loader and | ||
| // a single-field identifier | ||
| if (null ===$value && !$this->choiceList &&$this->objectLoader &&$this->idReader->isSingleId()) { | ||
| $optimize =null ===$value ||$isDefaultValue =is_array($value) &&isset($value[0]) &&$value[0]instanceof IdReader; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
no need to check forisset($value[0]) actually. An array callable must always have it.
$isDefaultValue seems unused here.
and if you want an even safer check, you may use$value[0] === $this->idReader (as passing a different idReader would also force us to disable the optimization as we could not ensure thatisSingleId would be the right one below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
kimlai@aa3425e This is perfect, thanks!
I used$isDefaultValue simply to name the boolean, because I thought that it was hard to understand that we were testing to see if$value was the defaultchoice_value, but your check makes much more sense, and so the need for a name is gone.
HeahDude commentedMar 3, 2016
LGTM, thanks@stof |
fabpot commentedMar 3, 2016
Thank you@kimlai. |
…yType (kimlai)This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes#17990).Discussion----------[DoctrineBridge][Form] Fix performance regression in EntityType| Q | A| ------------- | ---| Branch | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |A performance regression was introduced in2336d5cBefore, the default behaviour of the `DoctrineLoader` was to only fetch the entities selected in the submitted form.After, the optimization was only performed when the `choice_value` option was set to `null`.However, the `DoctrineType` sets a non-null default value to `choice_value`, which means that the default behaviour was not using the optimization anymore.This commit restores the default behaviour (while keeping the previous commit intent).References:-https://github.com/symfony/symfony/blob/v2.7.10/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php#L149-https://github.com/symfony/symfony/blob/v2.7.10/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php#L216Commits-------64c80a6 [DoctrineBridge][Form] Fix performance regression in EntityType
A performance regression was introduced in2336d5c
Before, the default behaviour of the
DoctrineLoaderwas to only fetch the entities selected in the submitted form.After, the optimization was only performed when the
choice_valueoption was set tonull.However, the
DoctrineTypesets a non-null default value tochoice_value, which means that the default behaviour was not using the optimization anymore.This commit restores the default behaviour (while keeping the previous commit intent).
References: