[DoctrineBridge] Fixed submitting invalid ids when using queries with limit #34900
Conversation
HeahDude
commented
Dec 9, 2019
HeahDude
commented
| Q | A |
|---|---|
| Branch? | 3.4 |
| Bug fix? | yes |
| New feature? | no |
| Deprecations? | no |
| Tickets | Fix #31559 |
| License | MIT |
| Doc PR | ~ |
HeahDude
force-pushed
the
fix-doctrine-loader-optim
branch
from
61b4aac
to
02a883b
Compare
| @@ -55,6 +55,26 @@ public function getEntities() | |||
| */ | |||
| public function getEntitiesByIds($identifier, array $values) | |||
| { | |||
| if (null !== $this->queryBuilder->getMaxResults()) { | |||
| $metadata = $this->queryBuilder->getEntityManager()->getClassMetadata(current($this->queryBuilder->getRootEntities())); | |||
There was a problem hiding this comment.
The only other way I see to fix this is to throw an exception here and to catch it in the DoctrineChoiceLoader to let it fall back on loading the list. But I would prefer to have it part of the EntityLoaderInterface contract rather than leaking too much in there.
src/Symfony/Bridge/Doctrine/Form/ChoiceList/ORMQueryBuilderLoader.php
Outdated
Show resolved
Hide resolved
HeahDude
force-pushed
the
fix-doctrine-loader-optim
branch
from
02a883b
to
e4b8380
Compare
src/Symfony/Bridge/Doctrine/Form/ChoiceList/ORMQueryBuilderLoader.php
Outdated
Show resolved
Hide resolved
HeahDude
force-pushed
the
fix-doctrine-loader-optim
branch
from
e4b8380
to
835077e
Compare
HeahDude
force-pushed
the
fix-doctrine-loader-optim
branch
from
835077e
to
09bb1e4
Compare
|
We could also consider the original issue as unsupported: doing a LIMIT query for a choice list looks very strange to me. |
|
I agree, I also felt that way. But we might consider some kind of "paginated" choices as a valid use case, or whatever reason leading to open the related issue. Finally the expected behavior covered here by the test seems a legit bug fix to me, but we could still deprecate that |
|
Also note that forbidding the usage in such case does lead to adding complexity and/or throwing or new specialized exception form the entity loader to catch in the DoctrineChoiceLoader as I propose in my comment above. |
|
If we were to forbid it purely and simply I think this should better be done when normalizing the |
|
Thank you @HeahDude. |
…ueries with limit (HeahDude) This PR was merged into the 3.4 branch. Discussion ---------- [DoctrineBridge] Fixed submitting invalid ids when using queries with limit | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | Fix #31559 <!-- prefix each issue number with "Fix #", if any --> | License | MIT | Doc PR | ~ <!-- Replace this notice by a short README for your feature/bugfix. This will help people understand your PR and can be used as a start for the documentation. Additionally (see https://symfony.com/roadmap): - Always add tests and ensure they pass. - Never break backward compatibility (see https://symfony.com/bc). - Bug fixes must be submitted against the lowest maintained branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too.) - Features and deprecations must be submitted against branch master. --> Commits ------- 09bb1e4 [DoctrineBridge] Fixed submitting invalid ids when using queries with limit
| $metadata = $this->queryBuilder->getEntityManager()->getClassMetadata(current($this->queryBuilder->getRootEntities())); | ||
|
|
||
| foreach ($this->getEntities() as $entity) { | ||
| if (\in_array(current($metadata->getIdentifierValues($entity)), $values, true)) { |
There was a problem hiding this comment.
Is it ok with strict in_array? There is comming string variable from EntityType, but getIdentifierValues return integer from entity (if I am using strict_types). Then I get always error by comparing string vs integer.
There was a problem hiding this comment.
Thank you @jirinapravnik, I've opened #35609 to fix this.
…offset (HeahDude) This PR was merged into the 3.4 branch. Discussion ---------- [DoctrineBridge] Fixed submitting ids with query limit or offset | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | Fix #34900 (comment) <!-- prefix each issue number with "Fix #", if any --> | License | MIT | Doc PR | ~ <!-- required for new features --> <!-- Replace this notice by a short README for your feature/bugfix. This will help people understand your PR and can be used as a start for the documentation. Additionally (see https://symfony.com/roadmap): - Always add tests and ensure they pass. - Never break backward compatibility (see https://symfony.com/bc). - Bug fixes must be submitted against the lowest maintained branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too.) - Features and deprecations must be submitted against branch master. --> Commits ------- 9bb1940 [DoctrineBridge] Fixed submitting ids with query limit or offset
