Fix BC-break on delete without alias DQL #7077
Conversation
lib/Doctrine/ORM/Query/Parser.php
Outdated
| } | ||
|
|
||
| $aliasIdentificationVariable = $this->AliasIdentificationVariable(); | ||
| try { |
There was a problem hiding this comment.
Maybe we should a have a comment here to explain the 💩 and specially mention that we should raise a deprecation notice in v2.7.0
| if ($token < Lexer::T_IDENTIFIER) { | ||
| $this->syntaxError($this->lexer->getLiteral($token)); | ||
| } | ||
| if ($lookaheadType === $token) { |
There was a problem hiding this comment.
I can drop this commit if you prefer, it was just to make things a bit more sane.
There was a problem hiding this comment.
It works indeed, it's just not so good as it could be... I can send another PR to make you happier 😂
|
@Ocramius @guilhermeblanco I was checking JPA 2.2 specs and maybe we should rather update the grammar for bulk deletes:
Source: http://download.oracle.com/otndocs/jcp/persistence-2_2-mrel-spec/index.html (4.10 Bulk Update and Delete Operations) |
lib/Doctrine/ORM/Query/Parser.php
Outdated
| } | ||
|
|
||
| $aliasIdentificationVariable = $this->AliasIdentificationVariable(); | ||
| try { |
There was a problem hiding this comment.
Could you use $this->lexer->isNextToken for this instead of trying to handle the exception?
There was a problem hiding this comment.
@carnage thanks... it was a way too gambiarra before HAHAHA
The `v2.5.x` series of the ORM allowed to have DELETE DQLs without using an alias, even though it didn't follow the grammar rules of the parser. We fixed that issue on `v2.6.0` however that was a BC-breaking change and lots of people were relying on this faulty behaviour. This workaround fixes the BC-break, without even trying to be elegant. In `v2.7.0.` we should raise a deprecation notice to notify people that we'll drop that "feature" in `v3.0`.
096ce9f to
fc943b7
Compare
| if ($token < Lexer::T_IDENTIFIER) { | ||
| $this->syntaxError($this->lexer->getLiteral($token)); | ||
| } | ||
| if ($lookaheadType === $token) { |
| $aliasIdentificationVariable = $this->AliasIdentificationVariable(); | ||
| $aliasIdentificationVariable = $this->lexer->isNextToken(Lexer::T_IDENTIFIER) | ||
| ? $this->AliasIdentificationVariable() | ||
| : 'alias_should_have_been_set'; |
There was a problem hiding this comment.
We should likely raise a deprecation notice here, to clarify that this isn't supported DQL (according to BNF)
There was a problem hiding this comment.
Didn't we say that deprecation notices were going to be added in v2.7 only?
There was a problem hiding this comment.
Works for me as long as you please create an issue tracking this specific deprecation 👍
There was a problem hiding this comment.
Sure, I was going to do that anyway 😄
| } | ||
| } | ||
|
|
||
| public function testSupportsDeleteWithoutWhereAndAlias() : void |
|
@Ocramius and what's your opinion regarding changing the grammar? |
Only after we move to https://github.com/hoaproject/Compiler Documenting grammar BC breaks becomes trivial after that. No changes before that, IMO. |
|
This has been forward-ported to |
…lt, so the test needs adaptation
As mentioned in #7053, this fixes the issue without breaking other tests.