[HttpFoundation] Allow DateTimeImmutable in Response setters #24677
Conversation
fea61e9 to
cc7ccee
Compare
| $date = \DateTimeImmutable::createFromMutable($date); | ||
| } | ||
|
|
||
| $date = $date->setTimezone(new \DateTimeZone('UTC')); |
There was a problem hiding this comment.
Correct me if I'm wrong, but DateTimeInterface doesn't have a setTimezone method from what I can tell.
There was a problem hiding this comment.
It doesn't. That's why the block above that line makes sure we're dealing with a DateTimeImmutable here.
There was a problem hiding this comment.
To explain this a bit more: The sole purpose of DateTimeInterface is to allow type-hinting for objects that are either DateTime or DateTimeImmutable. Those are the only two implementations of that interface and the php runtime doesn't allow userland code to directly implement the interface.
So at the beginning of the method, $date has to be either, null, DateTime or DateTimeImmutable. For null, we have an early exit. A DateTime instance is converted to DateTimeImmutable.
So right here, $date has to be an instance of DateTimeImmutable which contains the method setTimezone().
There was a problem hiding this comment.
the php runtime doesn't allow userland code to directly implement the interface.
Does this include extensions? Regardless of php allowing it or not, it looks weird like this. Should probably throw an exception or ignore the setter when it's not supported.
There was a problem hiding this comment.
We don't need to handle something that doesn't happen. that's just adding visual noise. to me it's good as it is
There was a problem hiding this comment.
Does this include extensions?
An extension could probably do that, but it would be a very odd thing to do.
I could not even write a test for the case that we have a DateTimeInterface instance that is neither a DateTime nor a DateTimeImmutable instance. I don't think, we need to handle it.
|
Thank you @derrabus. |
…etters (derrabus) This PR was merged into the 4.0-dev branch. Discussion ---------- [HttpFoundation] Allow DateTimeImmutable in Response setters | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | none | License | MIT | Doc PR | TODO ## Proposal This PR adds the ability to use `DateTimeImmutable` objects instead of `DateTime` in the following setters of HttpFoundation's `Response` class. * `setDate()` * `setExpires()` * `setLastModified()` The corresponding getters are not touched, meaning they will still return good old `DateTime` instances. ## BC considerations * Calling code using `DateTime` objects will still work as before. * Classes derived from `Response` will break if they override one of the methods above. Since all of them are considered final in Symfony 4, none of them should be overridden, though. Commits ------- cc7ccee Allow DateTimeImmutable in Response setters.
Proposal
This PR adds the ability to use
DateTimeImmutableobjects instead ofDateTimein the following setters of HttpFoundation'sResponseclass.setDate()setExpires()setLastModified()The corresponding getters are not touched, meaning they will still return good old
DateTimeinstances.BC considerations
DateTimeobjects will still work as before.Responsewill break if they override one of the methods above. Since all of them are considered final in Symfony 4, none of them should be overridden, though.