-
-
Notifications
You must be signed in to change notification settings - Fork 895
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
Fix Doctrine query for nested subresources #1608
Fix Doctrine query for nested subresources #1608
Conversation
fe65701
to
ac17260
Compare
ac17260
to
3ef7c9e
Compare
* | ||
* @ORM\Column(type="integer") | ||
* @ORM\Id | ||
* @ORM\GeneratedValue(strategy="AUTO") |
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.
not needed
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.
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.
Thought strategy="AUTO"
was the default value.
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.
Oh you meant that, OK!
$previousQueryBuilder = $qb; | ||
} else { | ||
$previousQueryBuilder->andWhere($qb->expr()->in($previousAlias, $qb->getDQL())); | ||
} |
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.
this is recursive already no? Not sure what was going on wrong here 😸
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, this was not recursive (despite the comment!).
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.
Should've been ? 😄
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.
Yup! :D
15f4dd0
to
3ef7c9e
Compare
*/ | ||
private function buildQuery(array $identifiers, array $context, QueryNameGenerator $queryNameGenerator, QueryBuilder $previousQueryBuilder, string $previousAlias, int $remainingIdentifiers, QueryBuilder $topQueryBuilder = null): QueryBuilder | ||
{ | ||
if (!$remainingIdentifiers) { |
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.
0 < --$remainingIdentifiers
then we can avoid --
below and the $remainingIdentifiers - 1
operation?
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.
I think it's clearer this way. Because if --
is done here it's more the position of the identifier and not the remaining ones. And you have to add the operation $remainingIdentifiers + 1
where there is $remainingIdentifiers
. WDYT?
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.
ok then maybe 0 === $remainingIdentifiers
instead of !
?
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.
If you prefer. Then not 0 >= $remainingIdentifiers
?
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.
Or even $remainingIdentifiers <= 0
(I don't really like Yoda style for comparison)?
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.
Dunno it's fine like this as well.
$qb->select($joinAlias) | ||
->from($identifierResourceClass, $alias) | ||
->innerJoin("$alias.$previousAssociationProperty", $joinAlias); | ||
|
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.
for cosmetics, new line is useless here imo \o/
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.
Sure :)
Thanks @alanpoulain |
Fix the build of the Doctrine query for subresources nested in more than 2 levels of depth.
Use a recursive function instead of a while loop.