[HttpFoundation] Deprecate extending some methods #19734

GuilhemN · 2016-08-25T12:23:23Z

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?

GuilhemN · 2016-08-25T12:24:29Z

src/Symfony/Component/HttpFoundation/Response.php

+
+        // Deprecations
+        $class = get_class($this);
+        if (__CLASS__ === $class || 0 === strpos($class, 'Symfony\\')) {


should we hardcode a list here to improve performance?

ok for me. what about this µoptim:?
if (__CLASS__ === $class || 'S' !== $class[0] || 0 === strpos($class, 'Symfony\\Component\\')) {

Any vendor not beginning by S would pass the check. I'll try hard-coding a list.

nicolas-grekas · 2016-08-25T13:32:29Z

src/Symfony/Component/HttpFoundation/Response.php

+            return;
+        }
+
+        foreach (array('setDate', 'setExpires', 'setLastModified', 'getDate', 'getExpires', 'getLastModified') as $method) {


array should be moved to a private static

nicolas-grekas · 2016-08-25T14:03:36Z

src/Symfony/Component/HttpFoundation/Response.php

+    private static $deprecatedMethods = array('setDate', 'setExpires', 'setLastModified', 'getDate', 'getExpires', 'getLastModified');
+    private static $deprecationsTriggered = array(
+        __CLASS__ => true,
+        ApacheRequest::class => true,


this is Request, not Response :)

Indeed, I really need an autocompleter... :P

Well in fact that's request, it just shouldn't be here... ^^

stof · 2016-08-25T16:21:29Z

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 (\PHPUnit_Framework_MockObject_MockObject for PHPUnit and \Prophecy\Doubler\DoubleInterface for Prophecy). I don't know Mockery, but it may be the same.

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 ?

GuilhemN · 2016-08-25T16:55:21Z

@stof fixed using get_parent_class when necessary. For Mockery it looks like its mocks aren't extending the mocked class so it won't change anything.

nicolas-grekas · 2016-08-25T17:02:52Z

src/Symfony/Component/HttpFoundation/Response.php

+
+        self::$deprecationsTriggered[$class] = true;
+        foreach (self::$deprecatedMethods as $method) {
+            $m = new \ReflectionMethod($class, $method);


$m should be $r "as usual"

isn't $r for ReflectionClass?

I'd be $c then :) $r is for Reflector, the base interface

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 in symfony/symfony#19734. Commits ------- fcd3345 [FrameworkBundle][Security] Remove useless mocks

nicolas-grekas · 2016-08-26T12:09:56Z

rebase unlocked

GuilhemN · 2016-08-27T23:44:47Z

@nicolas-grekas i already removed these commits.

javiereguiluz · 2016-08-29T07:45:37Z

@Ener-Getick could we estimate the performance hit related to the code added in the Response class (if any)? Thanks!

GuilhemN · 2016-08-29T09:45:38Z

@javiereguiluz that's hard to benchmark but when I run ResponseTest in different conditions, I have no significant results (maybe a difference of 1 or 2ms and it originally took ~50ms).

nicolas-grekas · 2016-08-31T07:47:22Z

👍 nice trick for eventually fixing these type hints in 4.0.
performance shouldn't be an issue: the new code is quick, and Response aren't instantiated that often.

GuilhemN · 2016-09-01T10:19:37Z

At the same time, would you like to make final other methods of Response?

I think we can make final those methods:

setContent/getContent
setProtocolVersion/getProtocolVersion
setStatusCode/getStatusCode
setCharset/getCharset
setPrivate/setPublic
getAge
getMaxAge/setMaxAge
setSharedMaxAge
getTtl/setTtl
setClientTtl
getEtag/setEtag
hasVary/getVary/setVary
isInvalid/isSuccessful/isRedirection/isClientError/isServerError
isOk/isForbidden/isNotFound/isRedirect/isEmpty

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 · 2016-09-01T17:32:56Z

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 · 2016-09-01T18:05:07Z

Ok, done.
The following methods will still be extendable (if you think we should also deprecate extensing some of them):

create
__clone
prepare
send/sendHeaders/sendContent
isCacheable/isFresh
isValidateable/mustRevalidate
expire
setCache
setNotModified/isNotModified
closeOutputBuffers
ensureIEOverSSLCompatibility (maybe this one should be made @internal)

Edit: we can't deprecate extending setContent and getContent as they are extended in symfony's specialized responses.

jakzal · 2016-09-02T09:46:42Z

@Ener-Getick setContent() is also overridden by laravel. Before proceeding with this PR, we should check if there are any popular libraries extending methods you propose to make final.

GuilhemN · 2016-09-02T17:08:07Z

Sure i don't think i'll have the time this week-end but maybe next week.

nicolas-grekas · 2016-09-14T08:37:17Z

src/Symfony/Component/HttpFoundation/Response.php

@@ -186,6 +186,15 @@ class Response
        511 => 'Network Authentication Required',                             // RFC6585
    );

+    private static $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');


could you put this array on several lines to ease reading please?

nicolas-grekas · 2016-09-14T08:38:26Z

I'm 👍 with the updated list

fabpot · 2016-09-14T20:47:46Z

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 · 2016-09-14T20:48:03Z

@Ener-Getick What about doing the same with the Request class?

GuilhemN · 2016-09-15T05:29:58Z

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 · 2016-09-15T15:44:59Z

As a side node I searched methods extended in drupal and laravel and I only found setContent and sendContent, so no problem with this PR.

…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: ```php public 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

@GuilhemN

…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`

carsonbot added Status: Needs Review HttpFoundation Deprecation labels Aug 25, 2016

GuilhemN mentioned this pull request Aug 25, 2016

[HttpFoundation] Add support for DateTimeImmutable in setters #19727

Closed

GuilhemN reviewed Aug 25, 2016
View reviewed changes

GuilhemN mentioned this pull request Aug 25, 2016

[FrameworkBundle][Security] Remove useless mocks #19736

Merged

nicolas-grekas reviewed Aug 25, 2016
View reviewed changes

GuilhemN force-pushed the RESPONSE branch from f76ff6e to dd815e5 Compare August 25, 2016 14:02

nicolas-grekas reviewed Aug 25, 2016
View reviewed changes

GuilhemN force-pushed the RESPONSE branch from dd815e5 to 85dd8ff Compare August 25, 2016 14:05

GuilhemN force-pushed the RESPONSE branch 2 times, most recently from 0ce0f25 to 270a0cf Compare August 25, 2016 16:53

nicolas-grekas reviewed Aug 25, 2016
View reviewed changes

nicolas-grekas added Status: Reviewed and removed Status: Needs Review labels Aug 31, 2016

GuilhemN force-pushed the RESPONSE branch from 5831ec8 to a52dbcb Compare September 1, 2016 18:04

GuilhemN force-pushed the RESPONSE branch from a52dbcb to 1206fa9 Compare September 1, 2016 18:12

nicolas-grekas reviewed Sep 14, 2016
View reviewed changes

GuilhemN force-pushed the RESPONSE branch from 1206fa9 to 62ab19a Compare September 14, 2016 10:39

[HttpFoundation] Deprecate extending some methods

c0a26bc

GuilhemN force-pushed the RESPONSE branch from 62ab19a to c0a26bc Compare September 14, 2016 10:52

fabpot merged commit c0a26bc into symfony:master Sep 14, 2016

GuilhemN deleted the RESPONSE branch September 15, 2016 05:28

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

GuilhemN mentioned this pull request Nov 11, 2016

[Debug] Trigger deprecation on @final annotation in DebugClassLoader - prepare making some classes final #20493

Merged

nicolas-grekas mentioned this pull request Jan 30, 2017

[HttpFoundation] Mark more methods as @final #21461

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpFoundation] Deprecate extending some methods #19734

[HttpFoundation] Deprecate extending some methods #19734

GuilhemN commented Aug 25, 2016 •

edited

Loading

GuilhemN Aug 25, 2016 •

edited

Loading

nicolas-grekas Aug 25, 2016

GuilhemN Aug 25, 2016

nicolas-grekas Aug 25, 2016

nicolas-grekas Aug 25, 2016

GuilhemN Aug 25, 2016

GuilhemN Aug 25, 2016

stof commented Aug 25, 2016 •

edited

Loading

GuilhemN commented Aug 25, 2016 •

edited

Loading

nicolas-grekas Aug 25, 2016

GuilhemN Aug 25, 2016 •

edited

Loading

nicolas-grekas Aug 25, 2016

GuilhemN Aug 25, 2016

nicolas-grekas commented Aug 26, 2016

GuilhemN commented Aug 27, 2016

javiereguiluz commented Aug 29, 2016

GuilhemN commented Aug 29, 2016

nicolas-grekas commented Aug 31, 2016

GuilhemN commented Sep 1, 2016 •

edited

Loading

fabpot commented Sep 1, 2016

GuilhemN commented Sep 1, 2016 •

edited

Loading

jakzal commented Sep 2, 2016

GuilhemN commented Sep 2, 2016

nicolas-grekas Sep 14, 2016

GuilhemN Sep 14, 2016

nicolas-grekas commented Sep 14, 2016

fabpot commented Sep 14, 2016

fabpot commented Sep 14, 2016

GuilhemN commented Sep 15, 2016 •

edited

Loading

GuilhemN commented Sep 15, 2016

[HttpFoundation] Deprecate extending some methods #19734

[HttpFoundation] Deprecate extending some methods #19734

Conversation

GuilhemN commented Aug 25, 2016 • edited Loading

GuilhemN Aug 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Aug 25, 2016 • edited Loading

GuilhemN commented Aug 25, 2016 • edited Loading

Choose a reason for hiding this comment

GuilhemN Aug 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Aug 26, 2016

GuilhemN commented Aug 27, 2016

javiereguiluz commented Aug 29, 2016

GuilhemN commented Aug 29, 2016

nicolas-grekas commented Aug 31, 2016

GuilhemN commented Sep 1, 2016 • edited Loading

fabpot commented Sep 1, 2016

GuilhemN commented Sep 1, 2016 • edited Loading

jakzal commented Sep 2, 2016

GuilhemN commented Sep 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Sep 14, 2016

fabpot commented Sep 14, 2016

fabpot commented Sep 14, 2016

GuilhemN commented Sep 15, 2016 • edited Loading

GuilhemN commented Sep 15, 2016

GuilhemN commented Aug 25, 2016 •

edited

Loading

GuilhemN Aug 25, 2016 •

edited

Loading

stof commented Aug 25, 2016 •

edited

Loading

GuilhemN commented Aug 25, 2016 •

edited

Loading

GuilhemN Aug 25, 2016 •

edited

Loading

GuilhemN commented Sep 1, 2016 •

edited

Loading

GuilhemN commented Sep 1, 2016 •

edited

Loading

GuilhemN commented Sep 15, 2016 •

edited

Loading