Allow DQL functions to specify return type #7941
Conversation
There was a problem hiding this comment.
Copying my review here
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.
If more dpeople could share their opinion about what I writtent, that would be nice too
|
@ostrolucky If you want I could postpone the change on ABS() so the new feature can be merged without BC (knowing we would have to change it in the future). |
|
Before doing that, let's see what other doctrine maintainers think. WDYT @doctrine/doctrinecore ? |
|
master is the next major release. Isn't this a fitting time to introduce a BC break? |
There was a problem hiding this comment.
This changeset is quite nice indeed. The title is a bit misleading since the previous behaviour was working as intended, I'd consider this as an improvement of the API.
The usage of floats for SUM(), AVG(), and ABS() might indeed cause some trouble with precision (which is why we don't do casts for decimal columns). I'd say we should leave them as string for now and have this targeting v2.8.0.
lcobucci
changed the title
| $query = $this->_em->createQuery( | ||
| 'SELECT | ||
| COUNT(product.id) as count, | ||
| SUM(product.price) as sales, | ||
| AVG(product.price) as average | ||
| FROM ' . GH7941Product::class . ' product' | ||
| ); | ||
|
|
||
| $result = $query->getSingleResult(); | ||
|
|
||
| self::assertSame(6, $result['count']); | ||
| self::assertSame('325', $result['sales']); | ||
| self::assertRegExp('/^54\.16+7$/', $result['average']); |
There was a problem hiding this comment.
@Grafikart this is what I meant with a more functional approach 😄
The main advantage is that we only rely on the public API here, which makes it easier to detect BC-breaks.
By using self::assertSame() we check that both value and type match expected value and self::assertRegExp() only accepts strings.
There was a problem hiding this comment.
This is good to go IMHO, it would be nice to have another pair of eyes here.
It might be good to document a minor behaviour change regarding the return type of COUNT() and LENGTH() - there's no need for casting it to integer anymore. What do you think @doctrine/team-orm-documentation?
|
@Grafikart thanks! 🚢 |
|
In some cases functions like |
|
@Grafikart When I use But if I use Shouldn't the result be consistent ? |
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.
This is the follow up of the #7641 PR (I won't delete it this time sorry for the inconvenience)