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

Allow MAX function as a CONCAT argument (improvement?) #7205

Closed
stphane opened this issue Apr 20, 2018 · 4 comments
Closed

Allow MAX function as a CONCAT argument (improvement?) #7205

stphane opened this issue Apr 20, 2018 · 4 comments

Comments

@stphane
Copy link

stphane commented Apr 20, 2018

I recently migrated our project from doctrine v2.5.6 to v2.6.1. and ran into an error saying:

Expected StateFieldPathExpression | string | InputParameter | FunctionsReturningStrings | AggregateExpression, got 'MAX'

… with the query being:

$QB = $this->_em->createQueryBuilder();
$QB ->addSelect("CONCAT(t2.dagId, MAX(t1.executionDate))")
    ->from(TaskInstance::class, 't1')
    ->join('t1.dag', 't2')
    ->addGroupBy('t2.dagId')
    ;


// Calling either getResults() or getSQL raises the error.
print_r($QB->getQuery()->getSQL());

After looking at Doctrine\ORM\Query\Parser::StringPrimary(), I ended up nesting MAX(…) call within a IF to fix the error:

$QB ->addSelect("CONCAT(t2.dagId, IF(1=1, MAX(t1.executionDate)))")

Is such a call rejection normal/intended or should this problem be addressed ?

@stof
Copy link
Member

stof commented Apr 27, 2018

shouldn't MAX be an AggregateExpression ?

@stphane
Copy link
Author

stphane commented Apr 27, 2018

That's what I thought since it is an aggregate function. It feels surprising that such functions are not considered to be valid tokens in this context.

@NothingWeAre
Copy link

After encountering similar problem, and looking at code i've noticed that StringPrimary no longer processes aggregate functions, this change happened somwhere betwean 2.5.14 - 2.6.1
Old function looked like:

            case Lexer::T_CASE:
            case Lexer::T_COALESCE:
            case Lexer::T_NULLIF:
                return $this->CaseExpression();

            default:
                if ($this->isAggregateFunction($lookaheadType)) {
                    return $this->AggregateExpression();
                }
        }
        $this->syntaxError(
            'StateFieldPathExpression | string | InputParameter | FunctionsReturningStrings | AggregateExpression'
        );

while new one missing part responsible for accepting aggregate expressions:

            case Lexer::T_CASE:
            case Lexer::T_COALESCE:
            case Lexer::T_NULLIF:
                return $this->CaseExpression();
        }
        $this->syntaxError(
            'StateFieldPathExpression | string | InputParameter | FunctionsReturningStrings | AggregateExpression'
        );

I did not find any mentions of this in documentation (but I could have missed it), so i think this is regression.

@Majkl578
Copy link
Contributor

Majkl578 commented Oct 8, 2018

This has been fixed in 2.6.2 via #7296.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants