[HttpFoundation] Deprecate compatibility with PHP <5.4 sessions #24239

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

     *
     * @throws \InvalidArgumentException
     */
    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

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

 /**
- * SessionHandler proxy.
- *
 * @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

@@ -205,6 +205,25 @@ FrameworkBundle
 HttpKernel
 ----------

+ * the `Symfony\Component\HttpFoundation\Session\Storage\Handler\NativeSessionHandler` 


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

+ * `NativeSessionStorage::setSaveHandler()` now takes an instance of `\SessionHandlerInterface` as argument.
+   Not passing it is deprecated and will throw a `TypeError` in 4.0.
+
+HttpKernel


Duplicate header (wrong rebase?)

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

UPGRADE-3.4.md

@@ -205,6 +205,25 @@ FrameworkBundle
 HttpKernel


This header should be HttpFoundation

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

Comments addressed :)

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

UPGRADE-3.4.md

@@ -202,6 +202,30 @@ FrameworkBundle
   `TranslationDebugCommand`, `TranslationUpdateCommand`, `XliffLintCommand`
    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

@@ -198,17 +198,17 @@ public function testSetSaveHandler()
        $this->iniSet('session.save_handler', 'files');
        $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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpFoundation] Deprecate compatibility with PHP <5.4 sessions #24239

[HttpFoundation] Deprecate compatibility with PHP <5.4 sessions #24239

afurculita commented Sep 17, 2017 •

edited

Loading

nicolas-grekas Sep 17, 2017 •

edited

Loading

afurculita Sep 17, 2017

Taluu Sep 17, 2017

afurculita Sep 17, 2017

afurculita Sep 17, 2017 •

edited

Loading

nicolas-grekas Sep 19, 2017

Tobion Sep 22, 2017

afurculita Sep 23, 2017

sstok left a comment

sstok Sep 18, 2017

sstok Sep 18, 2017

stof Sep 18, 2017

afurculita Sep 18, 2017

afurculita commented Sep 18, 2017

nicolas-grekas Sep 19, 2017

xabbuh commented Sep 20, 2017

afurculita commented Sep 24, 2017

Tobion Sep 25, 2017

Tobion Sep 25, 2017

Tobion Sep 25, 2017

nicolas-grekas commented Sep 26, 2017

Tobion commented Sep 26, 2017

Tobion commented Sep 26, 2017

afurculita commented Sep 26, 2017

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

[HttpFoundation] Deprecate compatibility with PHP <5.4 sessions #24239

[HttpFoundation] Deprecate compatibility with PHP <5.4 sessions #24239

Conversation

afurculita commented Sep 17, 2017 • edited Loading

nicolas-grekas Sep 17, 2017 • 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

afurculita Sep 17, 2017 • 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

sstok left a 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

afurculita commented Sep 18, 2017

Choose a reason for hiding this comment

xabbuh commented Sep 20, 2017

afurculita commented Sep 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Sep 26, 2017

Tobion commented Sep 26, 2017

Tobion commented Sep 26, 2017

afurculita commented Sep 26, 2017

afurculita commented Sep 17, 2017 •

edited

Loading

nicolas-grekas Sep 17, 2017 •

edited

Loading

afurculita Sep 17, 2017 •

edited

Loading