Ticket #1772 (closed defect: fixed)

Opened 7 months ago

Last modified 6 months ago

Table aliases in RHS of HAVING condition are not correctly resolved [patch provided]

Reported by: argasek Owned by: romanb
Priority: major Milestone: 1.1.0-BETA2
Component: Query/Hydration Version: 1.1.0-BETA1
Severity: Keywords: having, parser
Cc: Has Test: no
Status: Pending Core Response Has Patch: yes

Description

There are two errors in the method load() of Doctrine_Query_Having.

  • There's parsing done for RHS of condition containing functions, but the value is never used:
        $part .= ' ' . $operator . ' ' . $value;
        // check the RHS for aggregate functions
        if (strpos($value, '(') !== false) {
          $value = $this->parseAggregateFunction($value);
        }

The correct order should be:

        // check the RHS for aggregate functions
        if (strpos($value, '(') !== false) {
          $value = $this->parseAggregateFunction($value);
        }
        $part .= ' ' . $operator . ' ' . $value;
  • The second problem is: if RHS is not an aggregate function, but some (eg.) table field with an alias, the alias is not parsed correctly (however, it's done correctly inside "else" condition of parseAggregateFunction().

Attached patch solves both problems.

Now, some comment of mine: I'm not an object oriented programming guru, but calling load() method in body of method belonging to abstract class (Doctrine_Query_Condition in this case) is a violation of OOP in my opinion - how could parent class know, if a child class implements this method? The only correct solution would be by providing the interface to Doctrine_Query_Condition class, which declares load() method, thus enforcing the child classes to implement it. Maybe it could be also solved some way using Delegation pattern; anyway current implementation in Doctrine seems to be plain wrong (or, at least, less error-prone).

Attachments

having_rhs_aliases.patch (2.6 KB) - added by argasek 7 months ago.
Patch for correct resolution of aliases in RHS part of HAVING conditions

Change History

Changed 7 months ago by argasek

Patch for correct resolution of aliases in RHS part of HAVING conditions

Changed 6 months ago by guilhermeblanco

  • status changed from new to closed
  • resolution set to fixed

(In [5326]) fixes #1772. Thanks for the patch! I had to modify it a little, because it was breaking 2 test cases in our test suite.

Note: See TracTickets for help on using tickets.