Deprecate FunctionNode without TypedExpression

[beberlei]

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]

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 #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]

As long as nobody's actively working on this issue, we don't add it to a milestone.

[Aweptimum]

Alright, makes sense

I guess #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:

1. Implement interface on type
2. Assume the return type for simple functions and go off of beberlei's implementation when the return type is dependent on the inputs
3. Update/Add unit test to reflect new behavior based on this and what beberlei tested
4. Repeat 1-3 until all functions implement it
5. (The part I'm not so clear on) Add a mechanism to let the user know they're using deprecated behavior? Would this only applicable for custom DQL functions that don't implement TypedExpression?

[beberlei]

@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]

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

1. LENGTH: Integer
2. LOCATE: Integer
3. ABS: Numeric
4. SQRT: Always a float?
5. MOD: Numeric
6. SIZE: Integer
7. DATE_DIFF: Integer? Date_Interval? Docs seem to indicate it's just a number of days.
8. BIT_AND: Integer (?)
9. BIT_OR: Integer (?)

Datetime:

1. CURRENT_DATE: Date_Immutable (?)
2. CURRENT_TIME: Time_Immutable (?)
3. CURRENT_TIMESTAMP: Datetime_Immutable (?)
4. DATE_ADD: Datetime_Immutable (?)
5. DATE_SUB: Datetime_Immutable (?)

String Functions:

1. They all return the same type, but what is the type? Text | String | ASCII_String?

I'm probably also missing where types like GUID, JSON, BLOB/BINARY, and BOOLEAN come in

[beberlei]

@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]

@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]

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 TypeAware trait 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]

So I noticed in the 2.16 branch a note on ClassMetaDataInfo->getTypeOfField about replacing it with the PersisterHelper. 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?

orm/lib/Doctrine/ORM/Utility/PersisterHelper.php

Lines 29 to 59 in 609647a

	public static function getTypeOfField($fieldName, ClassMetadata $class, EntityManagerInterface $em)
	{
	if (isset($class->fieldMappings[$fieldName])) {
	return [$class->fieldMappings[$fieldName]['type']];
	}
	if (! isset($class->associationMappings[$fieldName])) {
	return [];
	}
	$assoc = $class->associationMappings[$fieldName];
	if (! $assoc['isOwningSide']) {
	return self::getTypeOfField($assoc['mappedBy'], $em->getClassMetadata($assoc['targetEntity']), $em);
	}
	if ($assoc['type'] & ClassMetadata::MANY_TO_MANY) {
	$joinData = $assoc['joinTable'];
	} else {
	$joinData = $assoc;
	}
	$types = [];
	$targetClass = $em->getClassMetadata($assoc['targetEntity']);
	foreach ($joinData['joinColumns'] as $joinColumn) {
	$types[] = self::getTypeOfColumn($joinColumn['referencedColumnName'], $targetClass, $em);
	}
	return $types;
	}