mirror of
https://github.com/doctrine/orm.git
synced 2026-03-24 06:52:09 +01:00
Deprecate FunctionNode without TypedExpression #6399
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @beberlei on GitHub (Feb 16, 2020).
In next major version we should make DQL TypedExpression a part of FunctionNode, which requires to deprecate it first in 2.x
This was introduced in #7941
Also related to #8025 - where identity now converts types. This is actually starts converting now, which we should consider a BC break.
What about introducing a query hint that starts converting function nodes, and when the query hint is not set, then we emit a deprecation?
@Aweptimum commented on GitHub (Sep 15, 2023):
Should this be added to the 3.0 milestone? A recent discussion inspired me to see if I could help find old issues to close. In my browsing I found https://github.com/doctrine/orm/issues/4641 + the accompanying PR + this issue.
It doesn't seem that big of a change or that much of a headache on the user-side to add this to a 2.x release before 3, but I don't know how when the team is trying to ship out 3.x.
@derrabus commented on GitHub (Sep 15, 2023):
As long as nobody's actively working on this issue, we don't add it to a milestone.
@Aweptimum commented on GitHub (Sep 15, 2023):
Alright, makes sense
I guess https://github.com/doctrine/orm/pull/8025 is waiting on this issue to be hammered out first.
I was thinking it might be good to finish what's already been started (the Count and Length DQL functions have implemented this interface). If it's a BC break then I'm assuming it gets pushed to 4.0 if no one's working on it.
If you or anyone else can give me some pointers/tips on what to do, I can try working on this. But I'm guessing the steps are:
TypedExpression?@beberlei commented on GitHub (Sep 17, 2023):
@Aweptimum yes sounds like a plan.
As for 5, indeed in ctor of type, check when its not implementing the interface and log a deprecation as other code does.
As for migration in 3.x code, the interface can go away when FunctionNode just assimilates its behavior.
@Aweptimum commented on GitHub (Oct 21, 2023):
Hey @beberlei, I started working on this some. I was wondering if the logic you added in your Identity PR could be extracted to a trait? For any of the AST functions that have a return type dependent on the input.
Mainly the aggregates like: abs, max, min, sum.
In the cases of abs and sum, they can return any kind of integer or a float.
In the cases of max/min, they can return anything.
Also, I'm not 100% certain on all of the return types in general. I'm assuming the types constants are now found in the Types class rather than the Type class where your PR references them.
Here's my best guess at the types, with a union type for reference: Numeric: Bigint | Integer | Smallint | Decimal | Float
Datetime:
String Functions:
I'm probably also missing where types like GUID, JSON, BLOB/BINARY, and BOOLEAN come in
@beberlei commented on GitHub (Oct 22, 2023):
@Aweptimum the changes in the Identity PR are so small, what do you want to use as a trait there?
As for the types, I believe you are correct with everything Date_Diff is an integer yes.
Everything should have functional tests as well, so you see when and what you get wrong.
@beberlei commented on GitHub (Oct 22, 2023):
@Aweptimum rereading the comments on #7941 i realized that we have to be careful with functions and converting to float due to the casting imprecision. If for example SUM is over a float/decimal type, then it should return as string.
@Aweptimum commented on GitHub (Oct 24, 2023):
The Identity function has logic for determining the requested column type to figure out the return type (since the identifier could be any type). It seems like it'd be useful to put that in a
TypeAwaretrait or something. I might also just not completely understand what's going on in there yet though.But in the case of MAX/MIN you can use those on most types (I don't think arrays, json, or binary data is acceptable), so it might be handy to bundle field checking into a method (is it a field or an association? if field -> field type, if association -> association identifier's type). Although it is slightly different from the identity function
@Aweptimum commented on GitHub (Oct 25, 2023):
So I noticed in the 2.16 branch a note on
ClassMetaDataInfo->getTypeOfFieldabout replacing it with thePersisterHelper. I looked to see if it had been added in the 3.0 branch and it does exactly what I wanted to do in a much more robust way - could I just use this in functions like MAX/MIN?609647a51a/lib/Doctrine/ORM/Utility/PersisterHelper.php (L29-L59)