Allow internal functions to be overridden. #6500
Conversation
|
Overall, this removes the internal and non-internal functions separation, which is quite an improvement. I'm not sure if I'd like folks to override stuff like |
The only reason there's a case in SqlWalker is because of the
I'm not sure I agree, there's plenty of other places to shoot yourself in the foot already. Custom SQL Walkers allow us to rewrite the output SQL in ways that don't relate to the SQL we write. I'd rather have a clean / consistent way to override functionality. Before I came up with this solution I was trying to think of alternatives via custom SQL Walkers, which would have been far more magical. At least with this I can look for a DQL function with the key |
|
Just a note here: patch looks very much clean and IMO it should be merged, but I'd like to have @beberlei, @guilhermeblanco and @lcobucci look at it first. |
|
LGTM. |
| * | ||
| * @return ORMException | ||
| */ | ||
| public static function overwriteInternalDQLFunctionNotAllowed($functionName) |
There was a problem hiding this comment.
This method removal is effectively a BC break, and while it is an internal method, it should be documented in UPGRADE.md
| @@ -541,10 +533,6 @@ public function setCustomNumericFunctions(array $functions) | |||
| */ | |||
| public function addCustomDatetimeFunction($name, $className) | |||
| { | |||
| if (Query\Parser::isInternalFunction($name)) { | |||
There was a problem hiding this comment.
@throws in docblock needs to be removed
| @@ -483,10 +479,6 @@ public function setCustomStringFunctions(array $functions) | |||
| */ | |||
| public function addCustomNumericFunction($name, $className) | |||
| { | |||
| if (Query\Parser::isInternalFunction($name)) { | |||
There was a problem hiding this comment.
@throws in docblock needs to be removed
| @@ -425,10 +425,6 @@ public function ensureProductionSettings() | |||
| */ | |||
| public function addCustomStringFunction($name, $className) | |||
| { | |||
| if (Query\Parser::isInternalFunction($name)) { | |||
There was a problem hiding this comment.
@throws in docblock needs to be removed
| /** | ||
| * @var AggregateExpression | ||
| */ | ||
| public $aggregateExpression; |
| /** | ||
| * @inheritDoc | ||
| */ | ||
| public function parse(Parser $parser) |
| * | ||
| * @return bool | ||
| */ | ||
| static public function isInternalFunction($functionName) |
There was a problem hiding this comment.
This method removal is potentially a BC break, and it needs to be documented in UPGRADE.md even though this is internal API
lib/Doctrine/ORM/Query/SqlWalker.php
Outdated
| @@ -1579,10 +1579,14 @@ public function walkSimpleSelectExpression($simpleSelectExpression) | |||
| $sql .= $this->walkPathExpression($expr); | |||
| break; | |||
|
|
|||
| case ($expr instanceof AST\AggregateExpression): | |||
| case ($expr instanceof AST\Functions\AvgFunction): | |||
There was a problem hiding this comment.
Since custom expressions are allowed, why not simply checking against $expr instanceof FunctionNode? Any reason why that wasn't done?
| */ | ||
| private $aggregateExpression; | ||
|
|
||
| public function parse(Parser $parser) |
| $this->aggregateExpression = $parser->AggregateExpression(); | ||
| } | ||
|
|
||
| public function getSql(SqlWalker $sqlWalker) |
|
@Ocramius Think I've covered everything, although are you sure you want to have void return types? Just making sure. Tests are failing because of that reason alone. |
|
@ThePixelDeveloper @Ocramius tests are failing because these changes depends on #6507. I'll review your PR now 😉 |
| * @since 2.6 | ||
| * @author Mathew Davies <[email protected]> | ||
| */ | ||
| class AvgFunction extends FunctionNode |
There was a problem hiding this comment.
Since it's brand new class we can make it final, right?
There was a problem hiding this comment.
Yup, they should all be final if they are new.
| * @since 2.6 | ||
| * @author Mathew Davies <[email protected]> | ||
| */ | ||
| class CountFunction extends FunctionNode |
There was a problem hiding this comment.
Since it's brand new class we can make it final, right?
| * @since 2.6 | ||
| * @author Mathew Davies <[email protected]> | ||
| */ | ||
| class MaxFunction extends FunctionNode |
There was a problem hiding this comment.
Since it's brand new class we can make it final, right?
| * @since 2.6 | ||
| * @author Mathew Davies <[email protected]> | ||
| */ | ||
| class MinFunction extends FunctionNode |
There was a problem hiding this comment.
Since it's brand new class we can make it final, right?
| * @since 2.6 | ||
| * @author Mathew Davies <[email protected]> | ||
| */ | ||
| class SumFunction extends FunctionNode |
There was a problem hiding this comment.
Since it's brand new class we can make it final, right?
|
@lcobucci Agreed, updated. |
|
@Ocramius @ThePixelDeveloper I've just rebased the branch to sync with |
|
@lcobucci seen, |
Background
This came about because we're writing a driver for PrestoDB and wanted to implement windowing for the aggregate functions. This is fine for anything new, but we struggled when trying to add windowing support for count, avg, etc ...
Source: 6.15. Window Functions — Presto 0.178 Documentation
Solution
Obvious solution for us was to have the ability to override the internal aggregate functions. We moved the internal aggregate functions into the
$_NUMERIC_FUNCTIONSproperty so they can be overridden; They were handled as a special case before.The only part we weren't sure of was the
dctrnalias used: https://github.com/SamKnows/doctrine2/pull/1/files#diff-0c8825d1265a66bb8b20f3d99c276961If that doesn't matter and we can go with
sclr, then this is another path that can be removed.Thoughts?
PS. First big PR for the Doctrine internals 👍