MC-28948: Doesn't work: "Files" as fallback when Magento fails to connect to "Redis"

Author: ameysar

dev/tests/integration/testsuite/Magento/Framework/Session/SaveHandlerTest.php

@@ -9,12 +9,18 @@
 use Magento\Framework\Exception\SessionException;
 use Magento\Framework\Phrase;
 use Magento\Framework\Session\Config\ConfigInterface;
-use Magento\Framework\App\ObjectManager;
+use Magento\TestFramework\Helper\Bootstrap;
+use Magento\TestFramework\ObjectManager;
 
+/**
+ * Tests \Magento\Framework\Session\SaveHandler functionality.
+ *
+ * @SuppressWarnings(PHPMD.CouplingBetweenObjects)
+ */
 class SaveHandlerTest extends \PHPUnit\Framework\TestCase
 {
     /**
-     * @var \Magento\TestFramework\ObjectManager
+     * @var ObjectManager
      */
     private $objectManager;
 
@@ -28,115 +34,64 @@ class SaveHandlerTest extends \PHPUnit\Framework\TestCase
      */
     private $saveHandlerFactoryMock;
 
+    /**
+     * @inheritdoc
+     */
     protected function setUp()
     {
-        $this->objectManager = \Magento\TestFramework\Helper\Bootstrap::getObjectManager();
+        $this->objectManager = Bootstrap::getObjectManager();
         $this->deploymentConfigMock = $this->createMock(DeploymentConfig::class);
         $this->objectManager->addSharedInstance($this->deploymentConfigMock, DeploymentConfig::class);
         $this->saveHandlerFactoryMock = $this->createMock(SaveHandlerFactory::class);
         $this->objectManager->addSharedInstance($this->saveHandlerFactoryMock, SaveHandlerFactory::class);
     }
 
+    /**
+     * @inheritdoc
+     */
     protected function tearDown()
     {
         $this->objectManager->removeSharedInstance(DeploymentConfig::class);
         $this->objectManager->removeSharedInstance(SaveHandlerFactory::class);
     }
 
     /**
-     * Tests that the session handler is correctly set when object is created.
-     *
-     * @dataProvider saveHandlerProvider
-     * @param string $deploymentConfigHandler
+     * @return void
      */
-    public function testConstructor($deploymentConfigHandler)
+    public function testRedisSaveHandlerFallbackToDefaultOnSessionException(): void
     {
-        $expected = $this->getExpectedSaveHandler($deploymentConfigHandler, ini_get('session.save_handler'));
-
         $this->deploymentConfigMock->method('get')
-            ->willReturnCallback(function ($configPath) use ($deploymentConfigHandler) {
-                switch ($configPath) {
-                    case Config::PARAM_SESSION_SAVE_METHOD:
-                        return $deploymentConfigHandler;
-                    case Config::PARAM_SESSION_CACHE_LIMITER:
-                        return 'private_no_expire';
-                    case Config::PARAM_SESSION_SAVE_PATH:
-                        return 'explicit_save_path';
-                    default:
-                        return null;
-                }
-            });
+            ->willReturnMap(
+                [
+                    [Config::PARAM_SESSION_SAVE_METHOD, null, 'redis'],
+                    [Config::PARAM_SESSION_SAVE_PATH, null, 'explicit_save_path'],
+                ]
+            );
 
-        $this->saveHandlerFactoryMock->expects($this->once())
-            ->method('create')
-            ->with($expected);
-        $sessionConfig = $this->objectManager->create(ConfigInterface::class);
-        $this->objectManager->create(SaveHandler::class, ['sessionConfig' => $sessionConfig]);
-
-        // Test expectation
-        $this->assertEquals(
-            $expected,
-            $sessionConfig->getOption('session.save_handler')
-        );
-    }
-
-    public function saveHandlerProvider()
-    {
-        return [
-            ['db'],
-            ['redis'],
-            ['files'],
-            [false],
-        ];
-    }
-
-    /**
-     * Retrieve expected session.save_handler
-     *
-     * @param string $deploymentConfigHandler
-     * @param string $iniHandler
-     * @return string
-     */
-    private function getExpectedSaveHandler($deploymentConfigHandler, $iniHandler)
-    {
-        if ($deploymentConfigHandler) {
-            return $deploymentConfigHandler;
-        } elseif ($iniHandler) {
-            return $iniHandler;
-        } else {
-            return SaveHandlerInterface::DEFAULT_HANDLER;
-        }
-    }
+        $redisHandlerMock = $this->getMockBuilder(SaveHandler\Redis::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $redisHandlerMock->method('open')
+            ->with('explicit_save_path', 'test_session_id')
+            ->willThrowException(new SessionException(new Phrase('Session Exception')));
 
-    public function testConstructorWithException()
-    {
-        $this->deploymentConfigMock->method('get')
-            ->willReturnCallback(function ($configPath) {
-                switch ($configPath) {
-                    case Config::PARAM_SESSION_SAVE_METHOD:
-                        return 'db';
-                    case Config::PARAM_SESSION_CACHE_LIMITER:
-                        return 'private_no_expire';
-                    case Config::PARAM_SESSION_SAVE_PATH:
-                        return 'explicit_save_path';
-                    default:
-                        return null;
-                }
-            });
+        $defaultHandlerMock = $this->getMockBuilder(SaveHandler\Native::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $defaultHandlerMock->expects($this->once())->method('open')->with('explicit_save_path', 'test_session_id');
 
         $this->saveHandlerFactoryMock->expects($this->at(0))
             ->method('create')
-            ->willThrowException(new SessionException(new Phrase('Session Exception')));
+            ->with('redis')
+            ->willReturn($redisHandlerMock);
         $this->saveHandlerFactoryMock->expects($this->at(1))
             ->method('create')
-            ->with(SaveHandlerInterface::DEFAULT_HANDLER);
-        $sessionConfig = $this->objectManager->create(ConfigInterface::class);
-        $this->objectManager->create(SaveHandler::class, ['sessionConfig' => $sessionConfig]);
+            ->with(SaveHandlerInterface::DEFAULT_HANDLER)
+            ->willReturn($defaultHandlerMock);
 
-        // Test expectation
-        $this->assertEquals(
-            'db',
-            $sessionConfig->getOption('session.save_handler')
-        );
+        $sessionConfig = $this->objectManager->create(ConfigInterface::class);
+        /** @var SaveHandler $saveHandler */
+        $saveHandler = $this->objectManager->create(SaveHandler::class, ['sessionConfig' => $sessionConfig]);
+        $saveHandler->open('explicit_save_path', 'test_session_id');
     }
 }

lib/internal/Magento/Framework/Session/SaveHandler.php

@@ -35,6 +35,11 @@ class SaveHandler implements SaveHandlerInterface
      */
     private $defaultHandler;
 
+    /**
+     * @var string
+     */
+    private $saveHandler;
+
     /**
      * @param SaveHandlerFactory $saveHandlerFactory
      * @param ConfigInterface $sessionConfig
@@ -48,20 +53,7 @@ public function __construct(
         $this->saveHandlerFactory = $saveHandlerFactory;
         $this->sessionConfig = $sessionConfig;
         $this->defaultHandler = $default;
-
-        /**
-         * Session handler
-         *
-         * Save handler may be set to custom value in deployment config, which will override everything else.
-         * Otherwise, try to read PHP settings for session.save_handler value. Otherwise, use 'files' as default.
-         */
-        $saveMethod = $this->sessionConfig->getOption('session.save_handler') ?: $this->defaultHandler;
-
-        try {
-            $this->saveHandlerAdapter = $this->saveHandlerFactory->create($saveMethod);
-        } catch (SessionException $e) {
-            $this->saveHandlerAdapter = $this->saveHandlerFactory->create($this->defaultHandler);
-        }
+        $this->saveHandler = $this->sessionConfig->getOption('session.save_handler') ?: $this->defaultHandler;
     }
 
     /**
@@ -145,6 +137,9 @@ public function gc($maxLifetime)
     private function callSafely(string $method, ...$arguments)
     {
         try {
+            if ($this->saveHandlerAdapter === null) {
+                $this->saveHandlerAdapter = $this->saveHandlerFactory->create($this->saveHandler);
+            }
             return $this->saveHandlerAdapter->{$method}(...$arguments);
         } catch (SessionException $exception) {
             $this->saveHandlerAdapter = $this->saveHandlerFactory->create($this->defaultHandler);