fix: COUNT() being casted as string #7641
Conversation
|
A much better solution (but I need to know if it would be accepted before implementing it) would be to add an interface This function would return "string" on the Then checking for this interface would be sufficient to resolve the type and every function could inform the walker of the type returned. |
|
I can try to write test but the SqlWalkerTest seems a bit empty ^^ |
|
I suggest looking at integration tests that verify the generated |
|
Good idea, can you point me to the file where I should add this test ? |
|
Possibly need a new file, similar to https://github.com/doctrine/orm/blob/master/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php, but focused on resultset mapping |
|
I added the tests and altered more functions |
|
Everything should be fixed now |
|
|
||
| /** | ||
| * @inheritDoc | ||
| */ |
There was a problem hiding this comment.
There is no doc to inherit yet, is this in case some is added in the future?
| @@ -1388,7 +1388,12 @@ public function walkSelectExpression($selectExpression) | |||
| if (! $hidden) { | |||
| // Conceptually we could resolve field type here by traverse through AST to retrieve field type, | |||
| // but this is not a feasible solution; assume 'string'. | |||
| $this->rsm->addScalarResult($columnAlias, $resultAlias, Type::getType('string')); | |||
| if ($expr instanceof Query\AST\TypedExpression) { | |||
There was a problem hiding this comment.
Maybe the comment above is no longer 100% accurate? Should probably be "assume 'string' when the expression is not typed"?
|
|
||
| use Doctrine\DBAL\Types\Type; | ||
|
|
||
| interface TypedExpression |
There was a problem hiding this comment.
Is this interface for internal use only? If yes, please mark it as such with @internal + a comment. If not, please document it.
There was a problem hiding this comment.
I don't think so it can be used by custom function created by the user (for instance COS() could implement this interface)
|
@Grafikart why did you delete your repository? I hope I didn't discourage you with my nitpicks? |
There was a problem hiding this comment.
It's a nice, simple patch solving the problem, however I'm not sure we should bite the bullet with floats. COUNT() and LENGTH() as integer types are fine, but casting to float in PHP will mean losing accuracy. It's also quite strange to make eg. ABS(1) return 1.0. Also, this a BC break and as such should be documented.
|
|
||
| public function getReturnType() : Type | ||
| { | ||
| return Type::getType(Type::STRING); |
There was a problem hiding this comment.
Why defining this when it's default fallback anyway?
There was a problem hiding this comment.
It's a nice, simple patch solving the problem, however I'm not sure we should bite the bullet with floats. COUNT() and LENGTH() as integer types are fine, but casting to float in PHP will mean losing accuracy. It's also quite strange to make eg. ABS(1) return 1.0. Also, this a BC break and as such should be documented.
|
@greg0ire I wanted to clean my repositories and I ended up deleting all my forks with an automated tool :D. I have no problem with the nitpicking since it makes the code better for everyone and helps me learning how to be more rigorous. To be honest I forgot about this PR but I could send a new PR with the same fixes if you still want this functionnality. |
|
Please do so. I think there is an interest from core team to have this functionality in some way |
|
I did a new PR #7941. I'm closing this one since I deleted the fork by mistake 👎 |
Some functions have a predefined type and could be resolved correctly by the SQLWalker. I don't want to change things too much but it could be possible to do the same thing for other functions like SUM for instance that would be resolved to float.
A much better solution would be to retrieve the aggregateExpression, then the pathExpression to resolve the type from the field but it's not feasable without an accessor from FunctionNode class.