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

Fixed parser rejecting valid DQL #7053

Closed
wants to merge 2 commits into from
Closed

Conversation

carnage
Copy link
Contributor

@carnage carnage commented Feb 9, 2018

Doctrine 2.5 allows the following as valid DQL:

"DELETE FROM Namespace\Entity"

Doctrine 2.6 rejects this as a syntax error.

The bug appears to be on the changed line, as the code didn't match the logic specified in the comment. I've not got a massive amount of knowledge on how the parser/lexer is built but changing this to a >= means that the above DQL is accepted as valid and appears to fix the problem.

@Ocramius
Copy link
Member

Ocramius commented Feb 9, 2018

@carnage can you still add a DQL SQL generation test? There's an already existing test class for that.

@Ocramius
Copy link
Member

Ocramius commented Feb 9, 2018

@carnage
Copy link
Contributor Author

carnage commented Feb 9, 2018

new test passes, fix breaks other tests. Will have to look into it further.

Is there a BNF grammar defined for DQL?

@carnage
Copy link
Contributor Author

carnage commented Feb 15, 2018

Dug into this some more.

Seems the syntax "DELETE FROM \namespace\Entity" isn't legal according to the grammar. However, it was accepted as a valid statement in >2.6 and performed as expected.

There are two possible options.
a) Close this as will not fix as it's against the defined grammar
b) Update the grammar and code to accept this statement and preserve BC

@lcobucci
Copy link
Member

lcobucci commented Feb 18, 2018

@carnage thanks for your help on digging up things 👍

@Ocramius @guilhermeblanco it looks like this commit is the cause of this issue, which was actually a fix to ensure that the grammar is properly followed. That seems to be the most relevant change from v2.5.x to v2.6.x, since it affects how we parse the abstract schema name...

I've managed to make the tests pass with a nasty gambiarra™ - which essentially consist in creating an alias when it doesn't exist.

Although it's quite dirty, I'd suggest that we should remove the BC-break in v2.6.x, add a deprecation notice there on v2.7.x, and don't support this anymore on v3.0. I'll open a PR for that.


For those wondering what's a gambiarra: it's a Brazilian word for a very creative and terrible solution.

If that description is not enough, here's an image.

@Ocramius Ocramius added this to the 2.6.1 milestone Feb 19, 2018
Ocramius added a commit that referenced this pull request Feb 19, 2018
@Ocramius Ocramius closed this in 92445d0 Feb 19, 2018
maglnet pushed a commit to maglnet/doctrine2 that referenced this pull request Oct 10, 2018
@carnage carnage deleted the hotfix-parser branch August 28, 2021 14:57
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.

3 participants