Orm: Failed parse the condition in ORDER BY

[peter-gribanov]

Bug Report

I want calculate the number of occurrences of a char in string. But DQL is failed parse to SQL.

| Q | A
|------------ | ------
| BC Break | no
| Version | 2.6.3

Summary

$ composer show --latest 'doctrine/*'
doctrine/annotations                v1.6.0  v1.6.0  Docblock Annotations Parser
doctrine/cache                      v1.8.0  v1.8.0  Caching library offering an object-oriented API for many cache backends
doctrine/collections                v1.5.0  v1.5.0  Collections Abstraction library
doctrine/common                     v2.10.0 v2.10.0 PHP Doctrine Common project is a library that provides additional functionality that other Doctrine p...
doctrine/dbal                       v2.9.0  v2.9.2  Powerful PHP database abstraction layer (DBAL) with many features for database schema introspection a...
doctrine/doctrine-bundle            1.10.0  1.10.2  Symfony DoctrineBundle
doctrine/doctrine-cache-bundle      1.3.5   1.3.5   Symfony Bundle for Doctrine Cache
doctrine/doctrine-migrations-bundle v1.3.2  v2.0.0  Symfony DoctrineMigrationsBundle
doctrine/event-manager              v1.0.0  v1.0.0  Doctrine Event Manager component
doctrine/inflector                  v1.3.0  v1.3.0  Common String Manipulations with regard to casing and singular/plural rules.
doctrine/instantiator               1.1.0   1.1.0   A small, lightweight utility to instantiate objects in PHP without invoking their constructors
doctrine/lexer                      v1.0.1  v1.0.1  Base library for a lexer that can be used in Top-Down, Recursive Descent Parsers.
doctrine/migrations                 v1.8.1  v2.0.0  Database Schema migrations using Doctrine DBAL
doctrine/orm                        v2.6.3  v2.6.3  Object-Relational-Mapper for PHP
doctrine/persistence                v1.1.0  v1.1.0  The Doctrine Persistence project is a set of shared interfaces and functionality that the different D...
doctrine/reflection                 v1.0.0  v1.0.0  Doctrine Reflection component

How to reproduce

DQL

SELECT (LENGTH(e.path) - LENGTH(REPLACE(e.path, :path_separator, :empty_string))) s FROM Category e

Result SQL

SELECT (LENGTH(e.path) - LENGTH(REPLACE(e.path, ".", ""))) AS s FROM shop__category e

DQL

SELECT LENGTH(e.path) - LENGTH(REPLACE(e.path, :path_separator, :empty_string)) FROM Category e

Result SQL

SELECT LENGTH(e.path) - LENGTH(REPLACE(e.path, ".", "")) FROM shop__category e

DQL

SELECT e FROM Category e ORDER BY LENGTH(e.path) - LENGTH(REPLACE(e.path, :path_separator, :empty_string)) ASC

Current behavior

[Syntax Error] line 0, col 82: Error: Expected end of string, got '-'

Expected SQL

SELECT e.* FROM shop__category e ORDER BY LENGTH(e.path) - LENGTH(REPLACE(e.path, ".", "")) ASC

DQL

SELECT e FROM Category e ORDER BY (LENGTH(e.path) - LENGTH(REPLACE(e.path, :path_separator, :empty_string))) ASC

Current behavior

[Syntax Error] line 0, col 99: Error: Expected Doctrine\ORM\Query\Lexer::T_IDENTIFIER, got '('

Expected SQL

SELECT e.* FROM shop__category e ORDER BY (LENGTH(e.path) - LENGTH(REPLACE(e.path, ".", ""))) ASC

[peter-gribanov]

I can solve the problem using a hidden field, but this is not a solution.

SELECT e, LENGTH(e.path) - LENGTH(REPLACE(e.path, :path_separator, :empty_string)) HIDDEN path_level FROM Category e ORDER BY path_level ASC

[Ocramius]

Duplicate of #7628

[peter-gribanov]

This is not a question. This is a bug report.
As i said before, this is not a solution. This is a crutch.
@Ocramius please explain to me what prevents the use of expressions in sorting?

[secit-pl]

I'm also interested the explanation of the question asked by @peter-gribanov

[peter-gribanov]

@Ocramius will there be any answer to the question related to this bug?

[Ocramius]

I think I referenced the wrong issue while closing, hence the confusion, sorry.

Lemme re-open here.

[Ocramius]

Ah, no, the closing is correct: the other issue explains that expressions in ORDER BY are indeed to be used by putting them in the SELECT clause and then re-using them (in SQL only) via the HIDDEN clause and an alias.

[peter-gribanov]

@Ocramius thank. I know this and wrote about it in the description of the problem. Me and @secit-pl are interested in why such a non-standard implementation is chosen?
The standard SQL allows the use of expressions in sorting, but the Doctrine does not allow it. Why?

[Ocramius]

Hmm, indeed, looked at support in downstream DBs, and they seem to be all supporting expressions in ORDER BY (unless I'm mistaken - maybe @morozov knows more here).

The DQL definitions affecting this (as of this writing) are:

OrderByClause       ::= "ORDER" "BY" OrderByItem {"," OrderByItem}*
OrderByItem ::= (SimpleArithmeticExpression | SingleValuedPathExpression | ScalarExpression | ResultVariable | FunctionDeclaration) ["ASC" | "DESC"]
SimpleArithmeticExpression ::= ArithmeticTerm {("+" | "-") ArithmeticTerm}*
ArithmeticTerm             ::= ArithmeticFactor {("*" | "/") ArithmeticFactor}*
ArithmeticFactor           ::= [("+" | "-")] ArithmeticPrimary
ArithmeticPrimary          ::= SingleValuedPathExpression | Literal | "(" SimpleArithmeticExpression ")"
                               | FunctionsReturningNumerics | AggregateExpression | FunctionsReturningStrings
                               | FunctionsReturningDatetime | IdentificationVariable | ResultVariable
                               | InputParameter | CaseExpression

Now, in theory this would parse this pretty much OK:

ORDER BY LENGTH(e.path) - LENGTH(REPLACE(e.path, :path_separator, :empty_string)) ASC

Since that is a:

OrderByClause(
  SimpleArithmeticExpression(
    ArithmeticTerm(ArithmeticPrimary(FunctionsReturningNumerics("LENGTH", ...)),
    "-",
    ArithmeticTerm(ArithmeticPrimary(FunctionsReturningNumerics("LENGTH", ...)))
  )
),
"ASC"

I guess I turned a long standing parser bug into an intentional limitation, sorry!

[morozov]

As long as the two values can be compared and are deterministic, there shouldn’t be any problems.

[lcobucci]

Using a HIDDEN field is mandatory according to our design. It's fine to discuss modifying this but it falls into Improvement and not Bug and should be implemented for v2.7 or v3.0 only

[Ocramius]

Closing here: I think @lcobucci's approach is simple and already solves the issue without introducing further questions about supported syntax.

[ipernet]

Ah, no, the closing is correct: the other issue explains that expressions in ORDER BY are indeed to be used by putting them in the SELECT clause and then re-using them (in SQL only) via the HIDDEN clause and an alias.

If I'm not mistaken, this cannot be a workaround when using window functions with Postgres or MySQL8.

This works with such as parser:

SELECT ROW_NUMBER() OVER(ORDER BY alias.field ASC) FROM Entity alias

But this does not due to the reported issue:

SELECT ROW_NUMBER() OVER(ORDER BY SOME_COMPLEX_EXPRESSION() ASC) FROM Entity alias

And the suggested workaround is not applicable then:

SELECT 
SOME_COMPLEX_EXPRESSION() as HIDDEN foo,
ROW_NUMBER() OVER(ORDER BY foo ASC)
FROM Entity alias

[ipernet]

The parser bug may be that in the case of OrderByItem(), a function will be detected before an expression, while it should be the opposite: expression can contain functions and thus they should be match first.

ANd due to that, if the first term after the GROUP BY is a function, the expression is not parsed and this fails:

ORDER BY LENGTH(e.path) - LENGTH(REPLACE(e.path, :path_separator, :empty_string)) ASC

But if it is something else, the expression is parsed successfully:

ORDER BY e.path - LENGTH(REPLACE(e.path, :path_separator, :empty_string)) ASC

A fix could be as simple as changing the "priority" of the switch statement used when iterating over tokens for OrderByItem():

diff --git a/lib/Doctrine/ORM/Query/Parser.php b/lib/Doctrine/ORM/Query/Parser.php
index 3fe1eb04f4..340dadbb55 100644
--- a/lib/Doctrine/ORM/Query/Parser.php
+++ b/lib/Doctrine/ORM/Query/Parser.php
@@ -1510,10 +1510,6 @@ public function OrderByItem()
         $glimpse = $this->lexer->glimpse();

         switch (true) {
-            case ($this->isFunction()):
-                $expr = $this->FunctionDeclaration();
-                break;
-
             case ($this->isMathOperator($peek)):
                 $expr = $this->SimpleArithmeticExpression();
                 break;
@@ -1526,6 +1522,10 @@ public function OrderByItem()
                 $expr = $this->ScalarExpression();
                 break;

+            case ($this->isFunction()):
+                $expr = $this->FunctionDeclaration();
+                break;
+
             default:
                 $expr = $this->ResultVariable();
                 break;

This needs more testing of course but this fix works well for the current issue and when using window functions.