Fix for DDC-3697 and DDC-3701

[mpdude]

This is about the DQL parser generating wrong SQL because conditions may end in the "ON" clause of a LEFT JOIN instead of the WHERE clause (http://www.doctrine-project.org/jira/browse/DDC-3697).

It also fixes that the parser may be too sloppy on expected tokens, for example accepting a WHERE instead of WITH (http://www.doctrine-project.org/jira/browse/DDC-3701) or identifiers with backslashes like in SELECT \foo.bar\baz FROM.... The first problem probably would have surfaced earlier with this strict check in place.

[doctrinebot]

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3715

We use Jira to track the state of pull requests and the versions they got
included in.

[mpdude]

@guilhermeblanco You've put many efforts into the parts touched by this PR – what do you think about it?

[mpdude]

Closed #1389 in favor of this PR as the problem is solved here. Added some tests for the #1389 problem.

[mpdude]

Tests fail due to a change in doctrine/cache, see #1510

[Ocramius]

@mpdude since #1510 was merged, rebasing this should bring the tests back to green

[Ocramius]

There are no constants that start with T__ or T_\\ (the last one is quite un-possible).

I suggest adding more cases for those specifically instead

[Ocramius]

Could need a separate lexer test as well

[Ocramius]

After looking more carefully at DDC-3701, I don't think that this fix is needed. In DQL, an identifier is basically anything "referenceable" (alias or entity name). An entity name cannot start with \, because DQL has no namespacing.

[mpdude]

Ouch. I was not aware of that and in fact it was possible to use a FQCN with a leading \ with no problems – probably due to the parser inaccuracy addressed here.

Just change one of the tests in SelectSqlGenerationTest on master and see yourself :-).

[mpdude]

And also have a look here where a FQCN with a leading \ is used in INSTANCE OF.

In the BCNF, both InstanceOfParameter and RangeVariableDeclaration build on AbstractSchemaName, which in turn is an identifier. identifier itself is a little... underspecified?

[Ocramius]

@mpdude if \ already exists in our docs, then I agree: it's needed for consistency.

[mpdude]

Adding a \Doctrine\Tests\ORM\Query\LexerTest::testScannerRecognizesIdentifier test that also covers leading \ in FQCNs; plus a new \Doctrine\Tests\ORM\Query\ParserTest that asserts AbstractSchemaName from BCNF will have leading \ stripped.

[mpdude]

@Ocramius Thanks! I followed all your comments.

[Ocramius]

@mpdude webfactory/doctrine2@033b4734f7a081dd743e18d24a3ed8142483eef2 broke the build though :-|

[mpdude]

Ooops, forgot to remove the test that checks for the classname validation in AbstractSchemaName.

[guilhermeblanco]

fully_qualified_name and aliased_name are not terminals. Keep the identifier (which holds both of your conceptualized values) only

[mpdude]

Might get you wrong but I think this PR just changed that...?

[mpdude]

Tests are green now.

[guilhermeblanco]

Closed in favor of #1549.

Thanks a lot for your help! It's not everyone that manages to dig so deeply and understand DQL parser and you've done an amazing job working on it. In the follow-up PR I basically re-introduced backslash to not create a BC and I fixed a couple hidden bugs I've found while traversing through you patch.

I'm very confident you now have a good understanding of how DQL parser works, so I'll explain which changes I've done, so you're aware.
Keywords had to be separate from identifiers. This was required because isNextToken() applied an exact match, while match() itself was very relaxed. That means match(FROM) was valid even though it was not FROM itself, because it could be considered as an identifier.
To address it, I updated your changed condition in match() and I separated non-terminals (0-99), identifiers (100-199) and keyword (200+). This helped me to make things more strict.