Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpFoundation] Allow DateTimeImmutable in Response setters#24677
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
(even for 4.0)
fabpot left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok for 4.0 as well
| $date = \DateTimeImmutable::createFromMutable($date); | ||
| } | ||
| $date =$date->setTimezone(new \DateTimeZone('UTC')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Correct me if I'm wrong, butDateTimeInterface doesn't have asetTimezone method from what I can tell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It doesn't. That's why the block above that line makes sure we're dealing with aDateTimeImmutable here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
To explain this a bit more: The sole purpose ofDateTimeInterface is to allow type-hinting for objects that are eitherDateTime orDateTimeImmutable. 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 orDateTimeImmutable. Fornull, we have an early exit. ADateTime instance is converted toDateTimeImmutable.
So right here,$date has to be an instance ofDateTimeImmutable which contains the methodsetTimezone().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 aDateTimeInterface instance that is neither aDateTime nor aDateTimeImmutable instance. I don't think, we need to handle it.
Tobion commentedOct 26, 2017
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## ProposalThis 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.