Allow DateTimeImmutable as parameter value #1333
Conversation
|
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DDC-3616 We use Jira to track the state of pull requests and the versions they got |
| @@ -53,7 +53,7 @@ public static function inferType($value) | |||
| return Type::BOOLEAN; | |||
| } | |||
|
|
|||
| if ($value instanceof \DateTime) { | |||
| if ($value instanceof \DateTime || $value instanceof \DateTimeImmutable) { | |||
There was a problem hiding this comment.
What about only one instanceof using \DateTimeInterface?
There was a problem hiding this comment.
DateTimeInterface isn't defined for 5.4.x
There was a problem hiding this comment.
@stekycz That's available only since PHP 5.5
There was a problem hiding this comment.
I know but \DateTimeImmutable too. So when I do not pass instance of \DateTime in PHP 5.4 or older then it will fail on the second part of this condition, right?
There was a problem hiding this comment.
Undefined class in instanceof check is not an error, is it?
Well you can still make it like this, if you wanna be pedantic:
if ($value instanceof \DateTime || (PHP_VERSION_ID >= 50500 && $value instanceof \DateTimeInterface)) {There was a problem hiding this comment.
Yes, undefined class in instanceof is not an error, it returns false. I think complicating the condition is unnecessary, since it's working in proposed form :-) http://3v4l.org/6KEnb
There was a problem hiding this comment.
@janlanger I would still be in favor of checking the interface rather than \DateTimeImmutable to support PHP 5.5+
There was a problem hiding this comment.
You mean like if ($value instanceof \DateTime || $value instanceof \DateTimeInterface) { ?
Hm, that would be probably better...
|
Note: test required. |
|
@Ocramius Is the test enough, or do you want something more? |
|
@janlanger manually merged into |
…e parameters (#8328) The support for passing \DateTimeImmutable instance as a query parameter has been added to ORM in #1333 (the year 2015), a long time before immutable date types (datetime_immutable etc) were introduced to DBAL in doctrine/dbal#2450 (2017). Back then, it made sense to treat \DateTimeImmutable (or any \DateTimeInterface) in the same way as \DateTime and infer parameter type as datetime. However, when immutable date types were later added to DBAL, it wasn't reflected anyhow in type inference in ORM and \DateTimeImmmutable instances are still inferred as datetime DBAL type. This PR fixes this IMO incorrect behaviour of ParameterTypeInferer::inferType(): for a \DateTimeImmmutable parameter, it now returns datetime_immutable DBAL type; for \DateTime or any other types implementing \DateTimeInterface, it returns datetime DBAL type as it did before. This behaviour is in line with DateTimeImmutableType handling only \DateTimeImmutable and DateTimeType handling any \DateTimeInterface. Why? In most cases, it doesn't matter and datetime works for \DateTimeImmutable parameters just fine. But it does matter if using custom implementation of datetime_immutable type like UTCDateTimeImmutableType from simpod/doctrine-utcdatetime. Then the broken type inference is revealed. This is partially related to #6443, however, this PR isn't about custom DBAL types but about correct type inference for build-in types.
This pull request allows passing instance of DateTimeImmutable as value to
setParameterand its correct formating in query.