Prevent load entities when pass empty choices for ModelType #6548
Conversation
|
Indeed, can you add tests @kirya-dev ? |
| @@ -128,7 +128,7 @@ public function loadChoiceList($value = null) | |||
| if (!$this->choiceList) { | |||
| if ($this->query) { | |||
| $entities = $this->modelManager->executeQuery($this->query); | |||
| } elseif (\is_array($this->choices) && \count($this->choices) > 0) { | |||
| } elseif (\is_array($this->choices)) { | |||
There was a problem hiding this comment.
Given what is currently checked in the test, I think this is the only change required in this PR.
There was a problem hiding this comment.
Not only. If we keep ModelType with default option choices with empty than on default entities dosent loaded
There was a problem hiding this comment.
Then, that isn't checked in the test. I mean, if I change only this line and run simple-phpunit --filter ModelTypeTest::testGetOptions, the test is passing.
There was a problem hiding this comment.
Must no passing becouse require method findBy calls once or never
There was a problem hiding this comment.
@phansys @VincentLanglet whyt?
There was a problem hiding this comment.
When you're passing explicitly null in the test, it's working indeed.
But what if you're not passing a choice.
$options = $optionResolver->resolve(['model_manager' => $modelManager]);
It should keep calling the findBy, but it won't anymore @phansys.
That's why I agree with @kirya-dev.
There was a problem hiding this comment.
I didn't analyzed the change, but if that is fine, I think the test must cover the case (the test provided in this PR is passing even if these changes are not applied).
There was a problem hiding this comment.
@kirya-dev Can you add a test with $options = $optionResolver->resolve(['model_manager' => $modelManager]); ?
There was a problem hiding this comment.
Of course. Added!
|
Thanks @kirya-dev |
Subject
I am targeting this branch, because would like make better our admin.
Closes #6517.
Changelog