Deprecat service "session"

jderusse · jderusse · commit 3b07b6297126 · 2021-01-28T16:56:06.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,7 @@ CHANGELOG
 5.3
 ---
 
+ * Deprecate the `session` service and the `SessionInterface` alias, use the `Request::getSession()` or the new `RequestStack::getSession()` methods instead
  * Added `AbstractController::renderForm()` to render a form and set the appropriate HTTP status code
  * Added support for configuring PHP error level to log levels
  * Added the `dispatcher` option to `debug:event-dispatcher`
diff --git a/Controller/AbstractController.php b/Controller/AbstractController.php
@@ -22,6 +22,7 @@
 use Symfony\Component\Form\FormFactoryInterface;
 use Symfony\Component\Form\FormInterface;
 use Symfony\Component\HttpFoundation\BinaryFileResponse;
+use Symfony\Component\HttpFoundation\Exception\SessionNotFoundException;
 use Symfony\Component\HttpFoundation\JsonResponse;
 use Symfony\Component\HttpFoundation\RedirectResponse;
 use Symfony\Component\HttpFoundation\Request;
@@ -200,11 +201,11 @@ protected function file($file, string $fileName = null, string $disposition = Re
      */
     protected function addFlash(string $type, $message): void
     {
-        if (!$this->container->has('session')) {
-            throw new \LogicException('You can not use the addFlash method if sessions are disabled. Enable them in "config/packages/framework.yaml".');
+        try {
+            $this->container->get('request_stack')->getSession()->getFlashBag()->add($type, $message);
+        } catch (SessionNotFoundException $e) {
+            throw new \LogicException('You can not use the addFlash method if sessions are disabled. Enable them in "config/packages/framework.yaml".', 0, $e);
         }
-
-        $this->container->get('session')->getFlashBag()->add($type, $message);
     }
 
     /**
diff --git a/DependencyInjection/Compiler/SessionPass.php b/DependencyInjection/Compiler/SessionPass.php
@@ -22,21 +22,41 @@ class SessionPass implements CompilerPassInterface
 {
     public function process(ContainerBuilder $container)
     {
-        if (!$container->hasDefinition('session')) {
+        if (!$container->has('session.storage')) {
             return;
         }
 
+        // BC layer: Make "session" an alias of ".session.do-not-use" when not overriden by the user
+        if (!$container->has('session')) {
+            $alias = $container->setAlias('session', '.session.do-not-use');
+            $alias->setDeprecated('symfony/framework-bundle', '5.3', 'The "%alias_id%" service is deprecated, use "$requestStack->getSession()" instead.');
+
+            return;
+        }
+
+        if ($container->hasDefinition('session')) {
+            $definition = $container->getDefinition('session');
+            $definition->setDeprecated('symfony/framework-bundle', '5.3', 'The "%service_id%" service is deprecated, use "$requestStack->getSession()" instead.');
+        } else {
+            $alias = $container->getAlias('session');
+            $alias->setDeprecated('symfony/framework-bundle', '5.3', 'The "%alias_id%" alias is deprecated, use "$requestStack->getSession()" instead.');
+            $definition = $container->findDefinition('session');
+        }
+
+        // Convert internal service `.session.do-not-use` into alias of `session`.
+        $container->setAlias('.session.do-not-use', 'session');
+
         $bags = [
             'session.flash_bag' => $container->hasDefinition('session.flash_bag') ? $container->getDefinition('session.flash_bag') : null,
             'session.attribute_bag' => $container->hasDefinition('session.attribute_bag') ? $container->getDefinition('session.attribute_bag') : null,
         ];
 
-        foreach ($container->getDefinition('session')->getArguments() as $v) {
+        foreach ($definition->getArguments() as $v) {
             if (!$v instanceof Reference || !isset($bags[$bag = (string) $v]) || !\is_array($factory = $bags[$bag]->getFactory())) {
                 continue;
             }
 
-            if ([0, 1] !== array_keys($factory) || !$factory[0] instanceof Reference || 'session' !== (string) $factory[0]) {
+            if ([0, 1] !== array_keys($factory) || !$factory[0] instanceof Reference || !\in_array((string) $factory[0], ['session', '.session.do-not-use'], true)) {
                 continue;
             }
 
diff --git a/KernelBrowser.php b/KernelBrowser.php
@@ -18,6 +18,7 @@
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\HttpFoundation\Session\Session;
 use Symfony\Component\HttpKernel\HttpKernelBrowser;
 use Symfony\Component\HttpKernel\KernelInterface;
 use Symfony\Component\HttpKernel\Profiler\Profile as HttpProfile;
@@ -122,7 +123,7 @@ public function loginUser($user, string $firewallContext = 'main'): self
 
         $token = new TestBrowserToken($user->getRoles(), $user);
         $token->setAuthenticated(true);
-        $session = $this->getContainer()->get('session');
+        $session = new Session($this->getContainer()->get('test.service_container')->get('session.storage'));
         $session->set('_security_'.$firewallContext, serialize($token));
         $session->save();
 
diff --git a/Resources/config/security_csrf.php b/Resources/config/security_csrf.php
@@ -27,7 +27,7 @@
         ->alias(TokenGeneratorInterface::class, 'security.csrf.token_generator')
 
         ->set('security.csrf.token_storage', SessionTokenStorage::class)
-            ->args([service('session')])
+            ->args([service('request_stack')])
 
         ->alias(TokenStorageInterface::class, 'security.csrf.token_storage')
 
diff --git a/Resources/config/session.php b/Resources/config/session.php
@@ -11,6 +11,7 @@
 
 namespace Symfony\Component\DependencyInjection\Loader\Configurator;
 
+use Symfony\Bundle\FrameworkBundle\Session\DeprecatedSessionFactory;
 use Symfony\Component\HttpFoundation\Session\Attribute\AttributeBag;
 use Symfony\Component\HttpFoundation\Session\Flash\FlashBag;
 use Symfony\Component\HttpFoundation\Session\Flash\FlashBagInterface;
@@ -33,15 +34,17 @@
     $container->parameters()->set('session.metadata.storage_key', '_sf2_meta');
 
     $container->services()
-        ->set('session', Session::class)
-            ->public()
+        ->set('.session.do-not-use', Session::class) // to be removed in 6.0
             ->args([
                 service('session.storage'),
                 null, // AttributeBagInterface
                 null, // FlashBagInterface
                 [service('session_listener'), 'onSessionUsage'],
             ])
-        ->alias(SessionInterface::class, 'session')
+        ->set('.session.deprecated', SessionInterface::class) // to be removed in 6.0
+            ->factory([inline_service(DeprecatedSessionFactory::class)->args([service('request_stack')]), 'getSession'])
+        ->alias(SessionInterface::class, '.session.do-not-use')
+            ->deprecate('symfony/framework-bundle', '5.3', 'The "%alias_id%" alias is deprecated, use "$requestStack->getSession()" instead.')
         ->alias(SessionStorageInterface::class, 'session.storage')
         ->alias(\SessionHandlerInterface::class, 'session.handler')
 
@@ -65,12 +68,12 @@
             ])
 
         ->set('session.flash_bag', FlashBag::class)
-            ->factory([service('session'), 'getFlashBag'])
+            ->factory([service('.session.do-not-use'), 'getFlashBag'])
             ->deprecate('symfony/framework-bundle', '5.1', 'The "%service_id%" service is deprecated, use "$session->getFlashBag()" instead.')
         ->alias(FlashBagInterface::class, 'session.flash_bag')
 
         ->set('session.attribute_bag', AttributeBag::class)
-            ->factory([service('session'), 'getBag'])
+            ->factory([service('.session.do-not-use'), 'getBag'])
             ->args(['attributes'])
             ->deprecate('symfony/framework-bundle', '5.1', 'The "%service_id%" service is deprecated, use "$session->getAttributeBag()" instead.')
 
@@ -94,8 +97,8 @@
         ->set('session_listener', SessionListener::class)
             ->args([
                 service_locator([
-                    'session' => service('session')->ignoreOnInvalid(),
-                    'initialized_session' => service('session')->ignoreOnUninitialized(),
+                    'session' => service('.session.do-not-use')->ignoreOnInvalid(),
+                    'initialized_session' => service('.session.do-not-use')->ignoreOnUninitialized(),
                     'logger' => service('logger')->ignoreOnInvalid(),
                     'session_collector' => service('data_collector.request.session_collector')->ignoreOnInvalid(),
                 ]),
diff --git a/Resources/config/test.php b/Resources/config/test.php
@@ -38,7 +38,7 @@
         ->set('test.session.listener', TestSessionListener::class)
             ->args([
                 service_locator([
-                    'session' => service('session')->ignoreOnInvalid(),
+                    'session' => service('.session.do-not-use')->ignoreOnInvalid(),
                 ]),
             ])
             ->tag('kernel.event_subscriber')
diff --git a/Session/DeprecatedSessionFactory.php b/Session/DeprecatedSessionFactory.php
@@ -0,0 +1,46 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\FrameworkBundle\Session;
+
+use Symfony\Component\HttpFoundation\Exception\SessionNotFoundException;
+use Symfony\Component\HttpFoundation\RequestStack;
+use Symfony\Component\HttpFoundation\Session\SessionInterface;
+
+/**
+ * Provides session and trigger deprecation.
+ *
+ * Used by service that should trigger deprecation when accessed by the user.
+ *
+ * @author Jérémy Derussé <jeremy@derusse.com>
+ *
+ * @internal to be removed in 6.0
+ */
+class DeprecatedSessionFactory
+{
+    private $requestStack;
+
+    public function __construct(RequestStack $requestStack)
+    {
+        $this->requestStack = $requestStack;
+    }
+
+    public function getSession(): ?SessionInterface
+    {
+        trigger_deprecation('symfony/framework-bundle', '5.3', 'The "session" service is deprecated, use "$requestStack->getSession()" instead.');
+
+        try {
+            return $this->requestStack->getSession();
+        } catch (SessionNotFoundException $e) {
+            return null;
+        }
+    }
+}
diff --git a/Tests/Controller/AbstractControllerTest.php b/Tests/Controller/AbstractControllerTest.php
@@ -497,8 +497,14 @@ public function testAddFlash()
         $session = $this->createMock(Session::class);
         $session->expects($this->once())->method('getFlashBag')->willReturn($flashBag);
 
+        $request = new Request();
+        $request->setSession($session);
+        $requestStack = new RequestStack();
+        $requestStack->push($request);
+
         $container = new Container();
         $container->set('session', $session);
+        $container->set('request_stack', $requestStack);
 
         $controller = $this->createController();
         $controller->setContainer($container);
diff --git a/Tests/DependencyInjection/Compiler/SessionPassTest.php b/Tests/DependencyInjection/Compiler/SessionPassTest.php
@@ -19,26 +19,77 @@
 class SessionPassTest extends TestCase
 {
     public function testProcess()
+    {
+        $container = new ContainerBuilder();
+        $container
+            ->register('session.storage'); // marker service
+        $container
+            ->register('.session.do-not-use');
+
+        (new SessionPass())->process($container);
+
+        $this->assertTrue($container->hasAlias('session'));
+        $this->assertSame($container->findDefinition('session'), $container->getDefinition('.session.do-not-use'));
+        $this->assertTrue($container->getAlias('session')->isDeprecated());
+    }
+
+    public function testProcessUserDefinedSession()
     {
         $arguments = [
             new Reference('session.flash_bag'),
             new Reference('session.attribute_bag'),
         ];
         $container = new ContainerBuilder();
+        $container
+            ->register('session.storage'); // marker service
         $container
             ->register('session')
             ->setArguments($arguments);
         $container
             ->register('session.flash_bag')
-            ->setFactory([new Reference('session'), 'getFlashBag']);
+            ->setFactory([new Reference('.session.do-not-use'), 'getFlashBag']);
         $container
             ->register('session.attribute_bag')
-            ->setFactory([new Reference('session'), 'getAttributeBag']);
+            ->setFactory([new Reference('.session.do-not-use'), 'getAttributeBag']);
 
         (new SessionPass())->process($container);
 
         $this->assertSame($arguments, $container->getDefinition('session')->getArguments());
         $this->assertNull($container->getDefinition('session.flash_bag')->getFactory());
         $this->assertNull($container->getDefinition('session.attribute_bag')->getFactory());
+        $this->assertTrue($container->hasAlias('.session.do-not-use'));
+        $this->assertSame($container->getDefinition('session'), $container->findDefinition('.session.do-not-use'));
+        $this->assertTrue($container->getDefinition('session')->isDeprecated());
+    }
+
+    public function testProcessUserDefinedAlias()
+    {
+        $arguments = [
+            new Reference('session.flash_bag'),
+            new Reference('session.attribute_bag'),
+        ];
+        $container = new ContainerBuilder();
+        $container
+            ->register('session.storage'); // marker service
+        $container
+            ->register('trueSession')
+            ->setArguments($arguments);
+        $container
+            ->setAlias('session', 'trueSession');
+        $container
+            ->register('session.flash_bag')
+            ->setFactory([new Reference('.session.do-not-use'), 'getFlashBag']);
+        $container
+            ->register('session.attribute_bag')
+            ->setFactory([new Reference('.session.do-not-use'), 'getAttributeBag']);
+
+        (new SessionPass())->process($container);
+
+        $this->assertSame($arguments, $container->findDefinition('session')->getArguments());
+        $this->assertNull($container->getDefinition('session.flash_bag')->getFactory());
+        $this->assertNull($container->getDefinition('session.attribute_bag')->getFactory());
+        $this->assertTrue($container->hasAlias('.session.do-not-use'));
+        $this->assertSame($container->findDefinition('session'), $container->findDefinition('.session.do-not-use'));
+        $this->assertTrue($container->getAlias('session')->isDeprecated());
     }
 }
diff --git a/Tests/DependencyInjection/FrameworkExtensionTest.php b/Tests/DependencyInjection/FrameworkExtensionTest.php
@@ -46,6 +46,7 @@
 use Symfony\Component\HttpClient\MockHttpClient;
 use Symfony\Component\HttpClient\RetryableHttpClient;
 use Symfony\Component\HttpClient\ScopingHttpClient;
+use Symfony\Component\HttpFoundation\Session\SessionInterface;
 use Symfony\Component\HttpKernel\DependencyInjection\LoggerPass;
 use Symfony\Component\Messenger\Transport\TransportFactory;
 use Symfony\Component\PropertyAccess\PropertyAccessor;
@@ -541,7 +542,7 @@ public function testSession()
     {
         $container = $this->createContainerFromFile('full');
 
-        $this->assertTrue($container->hasDefinition('session'), '->registerSessionConfiguration() loads session.xml');
+        $this->assertTrue($container->hasAlias(SessionInterface::class), '->registerSessionConfiguration() loads session.xml');
         $this->assertEquals('fr', $container->getParameter('kernel.default_locale'));
         $this->assertEquals('session.storage.native', (string) $container->getAlias('session.storage'));
         $this->assertEquals('session.handler.native_file', (string) $container->getAlias('session.handler'));
@@ -567,7 +568,7 @@ public function testNullSessionHandler()
     {
         $container = $this->createContainerFromFile('session');
 
-        $this->assertTrue($container->hasDefinition('session'), '->registerSessionConfiguration() loads session.xml');
+        $this->assertTrue($container->hasAlias(SessionInterface::class), '->registerSessionConfiguration() loads session.xml');
         $this->assertNull($container->getDefinition('session.storage.native')->getArgument(1));
         $this->assertNull($container->getDefinition('session.storage.php_bridge')->getArgument(0));
         $this->assertSame('session.handler.native_file', (string) $container->getAlias('session.handler'));
diff --git a/Tests/Functional/Bundle/TestBundle/Controller/DeprecatedSessionController.php b/Tests/Functional/Bundle/TestBundle/Controller/DeprecatedSessionController.php
@@ -0,0 +1,16 @@
+<?php
+
+namespace Symfony\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\Controller;
+
+use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
+use Symfony\Component\HttpFoundation\Response;
+
+class DeprecatedSessionController extends AbstractController
+{
+    public function triggerAction()
+    {
+        $this->get('session');
+
+        return new Response('done');
+    }
+}
diff --git a/Tests/Functional/Bundle/TestBundle/Resources/config/routing.yml b/Tests/Functional/Bundle/TestBundle/Resources/config/routing.yml
@@ -22,6 +22,10 @@ injected_flashbag_session_setflash:
     path:     injected_flashbag/session_setflash/{message}
     defaults: { _controller: Symfony\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\Controller\InjectedFlashbagSessionController::setFlashAction}
 
+deprecated_session_setflash:
+    path:     /deprecated_session/trigger
+    defaults: { _controller: Symfony\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\Controller\DeprecatedSessionController::triggerAction}
+
 session_showflash:
     path:     /session_showflash
     defaults: { _controller: Symfony\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\Controller\SessionController::showFlashAction }
diff --git a/Tests/Functional/SessionTest.php b/Tests/Functional/SessionTest.php
@@ -99,6 +99,26 @@ public function testFlashOnInjectedFlashbag($config, $insulate)
         $this->assertStringContainsString('No flash was set.', $crawler->text());
     }
 
+    /**
+     * @group legacy
+     * @dataProvider getConfigs
+     */
+    public function testSessionServiceTriggerDeprecation($config, $insulate)
+    {
+        $this->expectDeprecation('Since symfony/framework-bundle 5.3: The "session" service is deprecated, use "$requestStack->getSession()" instead.');
+
+        $client = $this->createClient(['test_case' => 'Session', 'root_config' => $config]);
+        if ($insulate) {
+            $client->insulate();
+        }
+
+        // trigger deprecation
+        $crawler = $client->request('GET', '/deprecated_session/trigger');
+
+        // check response
+        $this->assertStringContainsString('done', $crawler->text());
+    }
+
     /**
      * See if two separate insulated clients can run without
      * polluting each other's session data.
diff --git a/Tests/Functional/app/Session/config.yml b/Tests/Functional/app/Session/config.yml
@@ -9,3 +9,7 @@ services:
     Symfony\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\Controller\InjectedFlashbagSessionController:
         autowire: true
         tags: ['controller.service_arguments']
+
+    Symfony\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\Controller\DeprecatedSessionController:
+        autowire: true
+        autoconfigure: true
diff --git a/composer.json b/composer.json
