[HttpFoundation] Deprecate compatibility with PHP <5.4 sessions by afurculita · Pull Request #24239 · symfony/symfony

afurculita · 2017-09-17T13:33:12Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

This PR removes functionality added in Symfony 2.1 as a compatibility layer with sessions from PHP <5.4.

Fix tests

nicolas-grekas · 2017-09-17T13:48:16Z

src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php

    public function setSaveHandler($saveHandler = null)
    {
        if (!$saveHandler instanceof AbstractProxy &&
-            !$saveHandler instanceof NativeSessionHandler &&


should be reverted as that's a BC break, isn't it?

Nope, since Symfony 3.0, NativeSessionHandler always extends \SessionHandler. Thus now the rule !$saveHandler instanceof NativeSessionHandler is covered by !$saveHandler instanceof \SessionHandlerInterface

Taluu · 2017-09-17T15:27:28Z

src/Symfony/Component/HttpFoundation/Session/Storage/Proxy/AbstractProxy.php

@@ -12,7 +12,7 @@
 namespace Symfony\Component\HttpFoundation\Session\Storage\Proxy;



shouldn't you add a deprecated notice here ?

I had one, but removed it because AbstractProxy is extended by SessionHandlerProxy and this will make the tests that use SessionHandlerProxy to not pass. I can't mark these tests as legacy. What's a good solution to have a deprecation notice here and tests pass?

afurculita · 2017-09-17T16:03:43Z

src/Symfony/Component/HttpFoundation/Session/Storage/Proxy/SessionHandlerProxy.php

- *
 * @author Drak <drak@zikula.org>
 */
 class SessionHandlerProxy extends AbstractProxy implements \SessionHandlerInterface


I would also move this class from the Proxy folder to Handler. What do you think, @nicolas-grekas ?

That would need deprecating this class in favor of the new one, and allow triggering a deprecation in AbstractProxy also. LGTM.

I think the SessionHandlerProxy can be deprecated as well. When we only support \SessionHandlerInterface anyway, the proxy does not provide any value anymore AFAIK. I was meant as a way to make \SessionHandlerInterface and others behave the same.

I was thinking the same. The extra methods that SessionHandlerProxy is having can easily be moved to NativeSessionStorage as they are relevant only for a native session storage.

sstok

Some minor comments on the changelog, but it seems the HttpFoundation also has this rather strange commentary usage.

sstok · 2017-09-18T09:26:04Z

UPGRADE-3.4.md

   `TranslationDebugCommand`, `TranslationUpdateCommand`, `XliffLintCommand`
    and `YamlLintCommand` classes have been marked as final

+HttpKernel


Missing capital in the sentence (see other lines in this file).

And each line is separated by a single white line.

sstok · 2017-09-18T09:27:41Z

UPGRADE-3.4.md

+ * the `Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy::isWrapper()`
+   method has been deprecated and will be removed in 4.0. You can check explicitly if the proxy wraps 
+   a `\SessionHandler` instance.
+ * `NativeSessionStorage::setSaveHandler()` now takes an instance of `\SessionHandlerInterface` as argument.


Duplicate header (wrong rebase?)

stof · 2017-09-18T09:58:56Z

UPGRADE-3.4.md

@@ -202,6 +202,25 @@ FrameworkBundle
   `TranslationDebugCommand`, `TranslationUpdateCommand`, `XliffLintCommand`


This header should be HttpFoundation

afurculita · 2017-09-18T10:33:36Z

Comments addressed :)

nicolas-grekas · 2017-09-19T09:05:29Z

UPGRADE-3.4.md

    and `YamlLintCommand` classes have been marked as final

+HttpFoundation
+----------


wrong number of dashes :)

xabbuh · 2017-09-20T10:38:36Z

Please also update the UPGRADE-4.0.md file.

afurculita · 2017-09-24T19:36:35Z

Ready for review. I've also deprecated SessionHandlerProxy

Signed-off-by: Alexandru Furculita <alex@furculita.net>

Tobion · 2017-09-25T11:41:10Z

src/Symfony/Component/HttpFoundation/Tests/Session/Storage/NativeSessionStorageTest.php

        $storage = $this->getStorage();
        $storage->setSaveHandler();
-        $this->assertInstanceOf('Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy', $storage->getSaveHandler());
+        $this->assertInstanceOf(SessionHandlerProxy::class, $storage->getSaveHandler());


we don't change that to avoid conflicts unless the line needs to be touched anyway. so this should be reverted.

Tobion · 2017-09-25T11:50:35Z

src/Symfony/Component/HttpFoundation/Session/Storage/PhpBridgeSessionStorage.php

-     *
-     * @param AbstractProxy|NativeSessionHandler|\SessionHandlerInterface|null $handler
-     * @param MetadataBag                                                      $metaBag MetadataBag
+     * @param AbstractProxy|\SessionHandlerInterface|null $handler


Argument types that are deprecated should be removed from the doc

Tobion · 2017-09-25T11:51:22Z

src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php

-     * @param AbstractProxy|NativeSessionHandler|\SessionHandlerInterface|null $handler
-     * @param MetadataBag                                                      $metaBag MetadataBag
+     * @param array                                       $options Session configuration options
+     * @param AbstractProxy|\SessionHandlerInterface|null $handler


same, AbstractProxy should not be documented anymore

nicolas-grekas · 2017-09-26T10:24:06Z

@Tobion OK for you? Can I let you merge if yes?

Tobion · 2017-09-26T12:24:41Z

Thank you @afurculita.

… sessions (afurculita) This PR was squashed before being merged into the 3.4 branch (closes #24239). Discussion ---------- [HttpFoundation] Deprecate compatibility with PHP <5.4 sessions | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This PR removes functionality added in Symfony 2.1 as a compatibility layer with sessions from PHP <5.4. - [x] Fix tests Commits ------- 3deb394 [HttpFoundation] Deprecate compatibility with PHP <5.4 sessions

Tobion · 2017-09-26T12:33:04Z

@afurculita could you work on a PR against master to remove the deprecated stuff? Would be much appreciated.

afurculita · 2017-09-26T12:37:49Z

@Tobion already started it ;)

…5.4 sessions (afurculita) This PR was merged into the 4.0-dev branch. Discussion ---------- [HttpFoundation] Removed compatibility layer for PHP <5.4 sessions | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This is a follow-up of #24239. This PR removes the compatibility layer added for sessions for PHP <5.4. Commits ------- 37d1a21 Removed compatibility layer for PHP <5.4 sessions

carsonbot added Status: Needs Review HttpFoundation Deprecation labels Sep 17, 2017

afurculita force-pushed the 3.4 branch from f3a5b8e to 1078645 Compare September 17, 2017 13:35

nicolas-grekas added this to the 3.4 milestone Sep 17, 2017

nicolas-grekas reviewed Sep 17, 2017

View reviewed changes

afurculita force-pushed the 3.4 branch 2 times, most recently from 640661c to 71a92a2 Compare September 17, 2017 15:24

Taluu reviewed Sep 17, 2017

View reviewed changes

afurculita commented Sep 17, 2017

View reviewed changes

sstok reviewed Sep 18, 2017

View reviewed changes

stof requested changes Sep 18, 2017

View reviewed changes

afurculita force-pushed the 3.4 branch 2 times, most recently from 845d5a6 to f275707 Compare September 18, 2017 10:31

nicolas-grekas approved these changes Sep 19, 2017

View reviewed changes

afurculita force-pushed the 3.4 branch 2 times, most recently from 415ddf0 to 46c0740 Compare September 24, 2017 19:33

Alexandru Furculita and others added 2 commits September 24, 2017 22:38

[HttpFoundation] Deprecate <5.4 session functionality

73a1235

Signed-off-by: Alexandru Furculita <alex@furculita.net>

Deprecate SessonHanderProxy

831c006

Signed-off-by: Alexandru Furculita <alex@furculita.net>

afurculita force-pushed the 3.4 branch from 46c0740 to 831c006 Compare September 24, 2017 19:49

Tobion reviewed Sep 25, 2017

View reviewed changes

Remove deprecated classes from phpdoc; revert test changes

fdfe60b

Tobion closed this Sep 26, 2017

afurculita deleted the 3.4 branch September 26, 2017 12:26

afurculita restored the 3.4 branch September 26, 2017 12:26

afurculita mentioned this pull request Sep 26, 2017

[HttpFoundation] Removed compatibility layer for PHP <5.4 sessions #24338

Merged

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

MatTheCat mentioned this pull request Jun 29, 2023

[HttpFoundation] Make SessionHandlerProxy implement SessionIdInterface #50626

Closed

		@@ -12,7 +12,7 @@
		namespace Symfony\Component\HttpFoundation\Session\Storage\Proxy;

		@@ -202,6 +202,25 @@ FrameworkBundle
		`TranslationDebugCommand`, `TranslationUpdateCommand`, `XliffLintCommand`

Uh oh!

Conversation

afurculita commented Sep 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas Sep 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afurculita Sep 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sstok left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afurculita commented Sep 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Sep 20, 2017

Uh oh!

afurculita commented Sep 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Sep 26, 2017

Uh oh!

Tobion commented Sep 26, 2017

Uh oh!

Tobion commented Sep 26, 2017

Uh oh!

afurculita commented Sep 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

afurculita commented Sep 17, 2017 •

edited

Loading

nicolas-grekas Sep 17, 2017 •

edited

Loading

afurculita Sep 17, 2017 •

edited

Loading