Ticket #1772 (closed defect: fixed)
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).