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

Consider usage of setFetchMode when checking for simultaneous usage of fetch-mode EAGER and WITH condition #11174

Open
wants to merge 1 commit into
base: 2.17.x
Choose a base branch
from

Conversation

Abbraxar
Copy link

Concern issue #11128

…f fetch-mode EAGER and WITH condition.

This fixes a bug that arises when an entity relation is mapped with fetch-mode EAGER but setFetchMode LAZY (or anything that is not EAGER) has been used on the query. If the query use WITH condition, an exception is incorrectly raised (Associations with fetch-mode=EAGER may not be using WITH conditions).
Fixes doctrine#11128
@aprat84
Copy link

aprat84 commented Apr 26, 2024

Is this to be considered? It may solve a BC introduced in #8391

Copy link
Member

@beberlei beberlei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR, this is indeed an oversight and the fix looks good.

I have two improvements for the PR.

use Doctrine\ORM\Query;
use Doctrine\Tests\OrmFunctionalTestCase;

class GH11128Test extends OrmFunctionalTestCase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding a new test-case and model, you can just add another test to the existing by adapting for example EagerFetchCollectionTest::testSubselectFetchJoinWithNotAllowed

@@ -1047,7 +1047,7 @@ public function walkJoinAssociationDeclaration($joinAssociationDeclaration, $joi
}
}

if ($relation['fetch'] === ClassMetadata::FETCH_EAGER && $condExpr !== null) {
if (($this->query->getHint('fetchMode')[$assoc['sourceEntity']][$assoc['fieldName']] ?? $relation['fetch']) === ClassMetadata::FETCH_EAGER && $condExpr !== null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (($this->query->getHint('fetchMode')[$assoc['sourceEntity']][$assoc['fieldName']] ?? $relation['fetch']) === ClassMetadata::FETCH_EAGER && $condExpr !== null) {
$fetchMode = $this->query->getHint('fetchMode')[$assoc['sourceEntity']][$assoc['fieldName']] ?? $relation['fetch'];
if ($fetchMode === ClassMetadata::FETCH_EAGER && $condExpr !== null) {

@aprat84
Copy link

aprat84 commented May 2, 2024

@Abbraxar would appreciate if you could finish this PR. If you don't have time I could do it!

@Abbraxar
Copy link
Author

Abbraxar commented May 3, 2024

Hi @aprat84 !
I'm in vacations for a week so if you can continue this, feel free to do so.

@aprat84
Copy link

aprat84 commented May 6, 2024

Can't edit this PR, so cloned it: #11445.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants