MAGETWO-69430: Magento allows to save not valid data using config:set command

Author: shiftedreality

app/code/Magento/Config/Console/Command/ConfigSet/LockProcessor.php

@@ -81,13 +81,24 @@ public function process($path, $value, $scope, $scopeCode)
             $backendModel = $this->preparedValueFactory->create($path, $value, $scope, $scopeCode);
 
             if ($backendModel instanceof Value) {
-                $resourceModel = $backendModel->getResource();
-                $resourceModel->save($backendModel);
+                /**
+                 * Temporary solution until Magento introduce unified interface
+                 * for storing system configuration into database and configuration files.
+                 */
+                $backendModel->validateBeforeSave();
+                $backendModel->beforeSave();
 
                 $value = $backendModel->getValue();
 
+                $backendModel->afterSave();
+
+                /**
+                 * Because FS does not support transactions,
+                 * we'll write value just after all validations are triggered.
+                 */
                 $this->deploymentConfigWriter->saveConfig(
-                    [ConfigFilePool::APP_ENV => $this->arrayManager->set($configPath, [], $value)]
+                    [ConfigFilePool::APP_ENV => $this->arrayManager->set($configPath, [], $value)],
+                    false
                 );
             }
         } catch (\Exception $exception) {

app/code/Magento/Config/Test/Unit/Console/Command/ConfigSet/LockProcessorTest.php

@@ -13,7 +13,6 @@
 use Magento\Framework\App\DeploymentConfig;
 use Magento\Framework\Config\File\ConfigFilePool;
 use Magento\Framework\Exception\FileSystemException;
-use Magento\Framework\Model\ResourceModel\AbstractResource;
 use Magento\Framework\Stdlib\ArrayManager;
 use Magento\Store\Model\ScopeInterface;
 use PHPUnit_Framework_MockObject_MockObject as Mock;
@@ -56,11 +55,6 @@ class LockProcessorTest extends \PHPUnit_Framework_TestCase
      */
     private $valueMock;
 
-    /**
-     * @var AbstractResource|Mock
-     */
-    private $resourceMock;
-
     /**
      * @inheritdoc
      */
@@ -79,13 +73,9 @@ protected function setUp()
             ->disableOriginalConstructor()
             ->getMock();
         $this->valueMock = $this->getMockBuilder(Value::class)
-            ->setMethods(['save', 'getResource', 'setValue', 'getValue', 'afterSave'])
+            ->setMethods(['validateBeforeSave', 'beforeSave', 'setValue', 'getValue', 'afterSave'])
             ->disableOriginalConstructor()
             ->getMock();
-        $this->resourceMock = $this->getMockBuilder(AbstractResource::class)
-            ->setMethods(['save'])
-            ->disableOriginalConstructor()
-            ->getMockForAbstractClass();
 
         $this->model = new LockProcessor(
             $this->preparedValueFactory,
@@ -130,12 +120,6 @@ public function testProcess($path, $value, $scope, $scopeCode)
         $this->valueMock->expects($this->once())
             ->method('getValue')
             ->willReturn($value);
-        $this->valueMock->expects($this->once())
-            ->method('getResource')
-            ->willReturn($this->resourceMock);
-        $this->resourceMock->expects($this->once())
-            ->method('save')
-            ->with($this->valueMock);
         $this->deploymentConfigWriterMock->expects($this->once())
             ->method('saveConfig')
             ->with(
@@ -154,6 +138,12 @@ public function testProcess($path, $value, $scope, $scopeCode)
                 ],
                 false
             );
+        $this->valueMock->expects($this->once())
+            ->method('validateBeforeSave');
+        $this->valueMock->expects($this->once())
+            ->method('beforeSave');
+        $this->valueMock->expects($this->once())
+            ->method('afterSave');
 
         $this->model->process($path, $value, $scope, $scopeCode);
     }
@@ -185,12 +175,6 @@ public function testProcessNotReadableFs()
         $this->valueMock->expects($this->once())
             ->method('getValue')
             ->willReturn($value);
-        $this->valueMock->expects($this->once())
-            ->method('getResource')
-            ->willReturn($this->resourceMock);
-        $this->resourceMock->expects($this->once())
-            ->method('save')
-            ->with($this->valueMock);
         $this->configPathResolver->expects($this->once())
             ->method('resolve')
             ->willReturn('system/default/test/test/test');
@@ -223,10 +207,9 @@ public function testCustomException()
         $this->arrayManagerMock->expects($this->never())
             ->method('set');
         $this->valueMock->expects($this->once())
-            ->method('getResource')
-            ->willReturn($this->resourceMock);
-        $this->resourceMock->expects($this->once())
-            ->method('save')
+            ->method('getValue');
+        $this->valueMock->expects($this->once())
+            ->method('afterSave')
             ->willThrowException(new \Exception('Invalid values'));
         $this->deploymentConfigWriterMock->expects($this->never())
             ->method('saveConfig');