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] Deprecate extending some methods#19734
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
| // Deprecations | ||
| $class =get_class($this); | ||
| if (__CLASS__ ===$class ||0 ===strpos($class,'Symfony\\')) { |
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.
should we hardcode a list here to improve performance?
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 me. what about this µoptim:?if (__CLASS__ === $class || 'S' !== $class[0] || 0 === strpos($class, 'Symfony\\Component\\')) {
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.
Any vendor not beginning byS would pass the check. I'll try hard-coding a list.
| return; | ||
| } | ||
| foreach (array('setDate','setExpires','setLastModified','getDate','getExpires','getLastModified')as$method) { |
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.
array should be moved to aprivate static
| privatestatic$deprecatedMethods =array('setDate','setExpires','setLastModified','getDate','getExpires','getLastModified'); | ||
| privatestatic$deprecationsTriggered =array( | ||
| __CLASS__ =>true, | ||
| ApacheRequest::class =>true, |
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.
this is Request, not Response :)
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.
Indeed, I really need an autocompleter... :P
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.
Well in fact that's request, it just shouldn't be here... ^^
stof commentedAug 25, 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.
Regarding mocks, could we skip the warning for common mocking libraries ? At least PHPunit_MockObject and Prophecy are easy to detect, as their doubles are implementing an interface ( This would avoid causing issues for people mocking the Response class for other methods (Prophecy does not allow doing partial mocking for instance). @symfony/deciders what do you think ? |
0ce0f25 to270a0cfCompareGuilhemN commentedAug 25, 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.
@stof fixed using |
| self::$deprecationsTriggered[$class] =true; | ||
| foreach (self::$deprecatedMethodsas$method) { | ||
| $m =new \ReflectionMethod($class,$method); |
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.
$m should be $r "as usual"
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.
isn't$r forReflectionClass?
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.
I'd be $c then :) $r is for Reflector, the base interface
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.
Indeed makes sense now ^^
…etick)This PR was merged into the 2.7 branch.Discussion----------[FrameworkBundle][Security] Remove useless mocks| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |Removes mocks causing issues in#19734.Commits-------fcd3345 [FrameworkBundle][Security] Remove useless mocks
…etick)This PR was merged into the 2.7 branch.Discussion----------[FrameworkBundle][Security] Remove useless mocks| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |Removes mocks causing issues insymfony/symfony#19734.Commits-------fcd3345 [FrameworkBundle][Security] Remove useless mocks
…etick)This PR was merged into the 2.7 branch.Discussion----------[FrameworkBundle][Security] Remove useless mocks| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |Removes mocks causing issues insymfony/symfony#19734.Commits-------fcd3345 [FrameworkBundle][Security] Remove useless mocks
…etick)This PR was merged into the 2.7 branch.Discussion----------[FrameworkBundle][Security] Remove useless mocks| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |Removes mocks causing issues insymfony/symfony#19734.Commits-------fcd3345 [FrameworkBundle][Security] Remove useless mocks
nicolas-grekas commentedAug 26, 2016
rebase unlocked |
GuilhemN commentedAug 27, 2016
@nicolas-grekas i already removed these commits. |
javiereguiluz commentedAug 29, 2016
@Ener-Getick could we estimate the performance hit related to the code added in the |
GuilhemN commentedAug 29, 2016
@javiereguiluz that's hard to benchmark but when I run |
nicolas-grekas commentedAug 31, 2016
👍 nice trick for eventually fixing these type hints in 4.0. |
GuilhemN commentedSep 1, 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.
At the same time, would you like to make I think we can make final those methods:
Or is that too much? I think we can deprecate extending most methods and see if people complain about some deprecations. It won't break any applications and would make bc easier if this methods actually aren't extended (which should be the case as their content is very generic). |
fabpot commentedSep 1, 2016
The Response object is a data object that should be very "stable" (as HTTP is stable). So, I would say that extending most of the methods do not make sense indeed... if someone needs to do that, it's probably a bug that we should fix instead. |
GuilhemN commentedSep 1, 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.
Ok, done.
Edit: we can't deprecate extending |
jakzal commentedSep 2, 2016
@Ener-Getick |
GuilhemN commentedSep 2, 2016
Sure i don't think i'll have the time this week-end but maybe next week. |
| 511 =>'Network Authentication Required',// RFC6585 | ||
| ); | ||
| privatestatic$deprecatedMethods =array('setDate','getDate','setExpires','getExpires','setLastModified','getLastModified','setProtocolVersion','getProtocolVersion','setStatusCode','getStatusCode','setCharset','getCharset','setPrivate','setPublic','getAge','getMaxAge','setMaxAge','setSharedMaxAge','getTtl','setTtl','setClientTtl','getEtag','setEtag','hasVary','getVary','setVary','isInvalid','isSuccessful','isRedirection','isClientError','isOk','isForbidden','isNotFound','isRedirect','isEmpty'); |
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.
could you put this array on several lines to ease reading please?
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.
👍 done
nicolas-grekas commentedSep 14, 2016
I'm 👍 with the updated list |
fabpot commentedSep 14, 2016
Thank you @Ener-Getick. |
…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
fabpot commentedSep 14, 2016
@Ener-Getick What about doing the same with the Request class? |
GuilhemN commentedSep 15, 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 have to take a look at it to see if that's worth it, i'll probably do that in a few days. |
GuilhemN commentedSep 15, 2016
As a side node I searched methods extended in drupal and laravel and I only found |
…DebugClassLoader - prepare making some classes final (GuilhemN)This PR was merged into the 3.3-dev branch.Discussion----------[Debug] Trigger deprecation on `@final` annotation in DebugClassLoader - prepare making some classes final| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets | follows#19734| License | MIT| Doc PR | |BC promises become quickly huge but making classes `final` can limit these promises.At the same time, many classes of the symfony codebase are not meant to be extended and could be `final`; that's the goal of this PR: prepare making them final in 4.0 by triggering deprecations in their constructor:```phppublic function __construct(){ if (__CLASS__ !== get_class($this)) { @trigger_error(sprintf('Extending %s is deprecated since 3.3 and won\'t be supported in 4.0 as it will be final.', __CLASS__), E_USER_DEPRECATED); }}```I updated two classes for now but we can do much more if you like it.Commits-------c2ff111 [Debug] Trigger deprecation on `@final` annotation in DebugClassLoader
…grekas)This PR was merged into the 3.3-dev branch.Discussion----------[HttpFoundation] Mark more methods as `@final`| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | no| Fixed tickets | -| License | MIT| Doc PR | -Follow up of#19734 (ping@GuilhemN)Commits-------84a664f [HttpFoundation] Mark more methods as `@final`
Uh oh!
There was an error while loading.Please reload this page.
It's really hard to change methods signature because of bc. I'm proposing to deprecate extending some getters/setters of
Responsebecause 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
Responseentirely but does it matter as people should only mock needed methods?