Deprecate type inference in Query::setParameter
#8540
Conversation
| @@ -44,6 +44,9 @@ class ParameterTypeInferer | |||
| * - Type (\Doctrine\DBAL\Types\Type::*) | |||
| * - Connection (\Doctrine\DBAL\Connection::PARAM_*) | |||
| * | |||
| * @deprecated Infer Type will be removed in the future for setParameter method, | |||
There was a problem hiding this comment.
Will this not cause IDEs and static analyzers to mark all calls to this method as deprecated?
There was a problem hiding this comment.
yes that's correct but isn't this the desired outcome of the opened issue or did I misunderstood the issue?
There was a problem hiding this comment.
I think you misunderstood. IMO what's needed here is some usage of doctrine/deprecations in the cases that should be deprecated and only those.
There was a problem hiding this comment.
My bad then. Does this mean, if I understood it correctly now, that we just need to trigger a deprecation warning for the third argument of setParameter method by using doctrine/deprecations?
There was a problem hiding this comment.
Yes, I believe that's indeed what's needed 👍
There was a problem hiding this comment.
thanks for the clarification, I'll update the PR later today
There was a problem hiding this comment.
Do I have to update the existing tests or create a new one? How is this usually handled?
There was a problem hiding this comment.
It looks like you had the former anyway, but it might be good to do the latter too, to demonstrate that it affects all classes extending AbstractQuery, and not just NativeQuery
There was a problem hiding this comment.
I found only two classes that extend the AbstractQuery and the tests for both classes are updated. Can you think about any use-case for which we might need new tests for this?
There was a problem hiding this comment.
Since AbstractQuery is neither final nor internal, anybody can extend it outside doctrine/orm if they want to, and they should get the deprecation too.
…tion # Conflicts: # tests/Doctrine/Tests/ORM/Functional/NativeQueryTest.php
| @@ -381,6 +381,14 @@ public function setParameters($parameters) | |||
| */ | |||
| public function setParameter($key, $value, $type = null) | |||
| { | |||
| if ($type === null) { | |||
There was a problem hiding this comment.
We should assume every object passed must be an entity or specify a $type as third argument. Only primitive types should work with type inference.
I think the deprecation should fire only in some cases, but I'm not sure how to do that detection: @beberlei please help 🙏
There was a problem hiding this comment.
Based on the example of Datetime I guess we should also check if the value is an object and type is null, than we trigger the deprecation, but let's wait and see if we can get some more input
|
sorry didn't see tis earlier, i looked over the code for a few times as well, its sadly quite complicated and i haven't found the perfect location either. I believe |
@beberlei just for my understanding since from the issue description it says, if(is_object($value) && $type === null) {
//trigger deprecation notice
}This way we let the user know that the third argument has to be provided for non-primitive values. What are the implications that prevent us from doing so? In the |
|
What I want to prevent here is #8113 for example, in |
Deprecated type inference for
Query::setParametermentioned in issue #8379