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

Failing test for GH8443 #8855

Merged
merged 2 commits into from
Aug 3, 2021
Merged

Conversation

piowin
Copy link
Contributor

@piowin piowin commented Jul 23, 2021

A test suit for #8443.

Closes #8443

piowin and others added 2 commits July 23, 2021 21:27
Continuation of a problem from
doctrine#6812
The same problem appears if you add WITH condition to the joined entity with discriminator
@greg0ire
Copy link
Member

The same problem appears if you add WITH condition to the joined entity with discriminator

As opposed to what? #6812 is also about WITH, so I'm confused…

@piowin
Copy link
Contributor Author

piowin commented Jul 23, 2021

The same problem appears if you add WITH condition to the joined entity with discriminator

As opposed to what? #6812 is also about WITH, so I'm confused…

You're right. In both issues the problem was a result of using a with condition. I think the code of SqlWalker::walkJoinAssociationDeclaration could be simplfy a little and the #8443 wouldn't be necessary, but then the code doesn't pass some tests of the SelectSqlGeneratorTest, even though the sql syntax is correct.

@greg0ire
Copy link
Member

I'll rephrase my question: what is the difference between both issues? Since the first patch did not fix the second issue, there must be some difference somewhere, right?

@piowin
Copy link
Contributor Author

piowin commented Jul 23, 2021

The sqls are generated in different methods with different logic.

@greg0ire
Copy link
Member

Ok, why is that?

@piowin
Copy link
Contributor Author

piowin commented Jul 23, 2021

I haven't designed the SqlWalker, but I see that, in the terms of the EBNF of DQL, the logic of the generation of Join is more complex when we join JoinAssociationDeclaration than when we join RangeVariableDeclaration . Some of the logic is similar and maybe it would be possible to put that logic in a separate method.

@greg0ire
Copy link
Member

when we join JoinAssociationDeclaration than when we join RangeVariableDeclaration

Ah, so that's the difference! How do each of these look like in DQL?

@piowin
Copy link
Contributor Author

piowin commented Jul 24, 2021

A JoinAssociationDeclaration is a path to an entity's field (the field can be a collection of entities, a single entity or a simple state field) plus an alias: p.friend AS f (p must be defined before and that's one of the differences with the next one).

A RangeVariableDeclaration is a path to a class plus an alias: \MyNamespace\Person AS p. A RangeVariableDeclaration is the obligatory component that must be placed immediately after the "FROM" clause.

A JoinAssociationDeclaration can be only used as a joined component while A RangeVariableDeclaration can be used as a joined component and a starting point/base of an IdentificationVariableDeclaration which is a building block of the "FROM" clause.

How does the method responsible for generating joins work?
When the sql for a Join is being generated, if the Join is a JoinAssociationDeclaration, it simply delegates to the method that is responsible for generating JoinAssociationDeclaration, that's all. But if the Join is a RangeVariableDeclaration, it generates optional conditions for the RangeVariableDeclaration, like a filter condition, a "with" condition or a discriminator condition. The generation of each conidtion has its unique logic. Also the sql for the RangeVariableDeclaration is being generated by the method that is responsible for that (the method is invoked also when we genarate IdentificationVariableDeclaration, see above). The conditions are added to the sql and the result is returned.

The logic of the generation of the optional brackets (the subject of our discussion) is put in the method that is reponsible for generating JoinAssociationDeclaration, if the Join is a JoinAssociationDeclaration, and is put in the method that is responsible for generating joins, if the Join is a RangeVariableDeclaration.

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Very clear! Thank you!

@greg0ire greg0ire merged commit c1c3c89 into doctrine:2.9.x Aug 3, 2021
@greg0ire
Copy link
Member

greg0ire commented Aug 3, 2021

Thanks @piowin!

@greg0ire greg0ire added this to the 2.9.4 milestone Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants