[DoctrineBridge][Form] Fix performance regression in EntityType #17990
Conversation
A performance regression was introduced in symfony@2336d5c Before, 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#L216
| @@ -146,7 +146,7 @@ public function loadChoicesForValues(array $values, $value = null) | |||
|
|
|||
| // 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 (!($value instanceof \Closure) && !$this->choiceList && $this->objectLoader && $this->idReader->isSingleId()) { | |||
There was a problem hiding this comment.
👍 but this should be !is_callable($value) as it could be an array or a property path
There was a problem hiding this comment.
@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.
Actually, the new test doesn't pass with !is_callable($value), which means that it breaks the optimization.
That's because the default value set by DoctrineType is callable, so we wouldn't perform the optim by default.
I'm not sure that I really understand when we want the optimization to be performed. One thing I'm sure of is that if no value is passed to choice_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.
Since the IdReader 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.
Thanks it's working 470f319 !
I added a check to know if $value is an array, and a variable for clarity.
| @@ -146,7 +146,9 @@ public function loadChoicesForValues(array $values, $value = null) | |||
|
|
|||
| // 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.
no need to check for isset($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 that isSingleId would be the right one below)
There was a problem hiding this comment.
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 default choice_value, but your check makes much more sense, and so the need for a name is gone.
|
LGTM, thanks @stof |
|
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 in 2336d5c Before, 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#L216 Commits ------- 64c80a6 [DoctrineBridge][Form] Fix performance regression in EntityType
A performance regression was introduced in 2336d5c
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: