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] Add support for DateTimeImmutable in setters#19727
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
Backwards compatibility is assured by typehinting for DateTimeInterface, and converting to \DateTime for onward use
linaori commentedAug 24, 2016
The problem is that when someone extends the response and the methods you change the signature of, it will cause compatibility issues afaik. |
dvdoug commentedAug 24, 2016
Urgh, I suppose that is indeed true although I can't imagine anyone actually extending those methods. If I targeted this to master would you take it? |
linaori commentedAug 24, 2016
The master would still mean it's a BC break and that cannot be accepted :( |
ro0NL commentedAug 24, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Same as#19578 These changes need a |
wouterj commentedAug 24, 2016
No, there never can be a BC break in Symfony. |
ro0NL commentedAug 24, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
4.0 will have BC breaks..? Anyway, currently Having a branch targeting 4.0 is considerable as you can actually make these changes somewhere and you prepare the release beforehand. Pro: actually have your upcoming version in VCS |
linaori commentedAug 24, 2016
Method signatures other than constructors can't really be provided via a BC layer |
ro0NL commentedAug 24, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I would consider this BC break in 4.0 :) but it can't be done. Lets move forward :) edit: we could consider documenting the break now, and not forget to make the change in 4.0 (like not forgetting to remove the deprecations, etc.) |
| * @return Response | ||
| */ | ||
| publicfunctionsetDate(\DateTime$date) | ||
| publicfunctionsetDate(\DateTimeInterface$date) |
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.
What about renaming it tosetDateHeader for bc ?
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.
technically, it would work yes
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.
Another solution could be to deprecate extending this functions and make them final in 4.0 (i prefer this solution).
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.
@Ener-Getick let's give it a try, would you mind opening a PR doing this?
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.
sure, see#19734
…r-Getick)This PR was merged into the 3.2-dev branch.Discussion----------[HttpFoundation] Deprecate extending some methods| Q | A| ------------- | ---| Branch? | "master"| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets |#19727| License | MIT| Doc PR |It's really hard to change methods signature because of bc. I'm proposing to deprecate extending some getters/setters of `Response` because of this (and because extending them is not really useful).If you like this approach it could be used in other places to simplify bc in 4.0.Edit: This causes issues (warnings always triggered) when mocking `Response` entirely but does it matter as people should only mock needed methods?Commits-------c0a26bc [HttpFoundation] Deprecate extending some methods
…r-Getick)This PR was merged into the 3.2-dev branch.Discussion----------[HttpFoundation] Deprecate extending some methods| Q | A| ------------- | ---| Branch? | "master"| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets |symfony/symfony#19727| License | MIT| Doc PR |It's really hard to change methods signature because of bc. I'm proposing to deprecate extending some getters/setters of `Response` because of this (and because extending them is not really useful).If you like this approach it could be used in other places to simplify bc in 4.0.Edit: This causes issues (warnings always triggered) when mocking `Response` entirely but does it matter as people should only mock needed methods?Commits-------c0a26bc [HttpFoundation] Deprecate extending some methods
Backwards compatibility is assured by typehinting for DateTimeInterface, and converting to \DateTime for onward use
This is my first PR for Symfony, so please let me know if I've done something not quite right.