Merge pull request #2814 from magento-qwerty/2.3-bugfixes-290618

Fixed issues:
- MAGETWO-88600: Error while saving Theme design configuration
- MAGETWO-88642: Sort Order Field Values

Author: dvoskoboinikov

app/code/Magento/Theme/Controller/Adminhtml/Design/Config/Save.php

@@ -7,6 +7,7 @@
 
 use Magento\Backend\App\Action;
 use Magento\Framework\App\Request\DataPersistorInterface;
+use Magento\Framework\Exception\NotFoundException;
 use Magento\Theme\Model\DesignConfigRepository;
 use Magento\Backend\App\Action\Context;
 use Magento\Framework\Exception\LocalizedException;
@@ -62,9 +63,15 @@ protected function _isAllowed()
 
     /**
      * @return \Magento\Framework\Controller\Result\Redirect
+     *
+     * @throws NotFoundException
      */
     public function execute()
     {
+        if (!$this->getRequest()->isPost()) {
+            throw new NotFoundException(__('Page not found.'));
+        }
+
         $resultRedirect = $this->resultRedirectFactory->create();
         $scope = $this->getRequest()->getParam('scope');
         $scopeId = (int)$this->getRequest()->getParam('scope_id');
@@ -73,7 +80,7 @@ public function execute()
         try {
             $designConfigData = $this->configFactory->create($scope, $scopeId, $data);
             $this->designConfigRepository->save($designConfigData);
-            $this->messageManager->addSuccess(__('You saved the configuration.'));
+            $this->messageManager->addSuccessMessage(__('You saved the configuration.'));
 
             $this->dataPersistor->clear('theme_design_config');
 
@@ -86,10 +93,10 @@ public function execute()
         } catch (LocalizedException $e) {
             $messages = explode("\n", $e->getMessage());
             foreach ($messages as $message) {
-                $this->messageManager->addError(__('%1', $message));
+                $this->messageManager->addErrorMessage(__('%1', $message));
             }
         } catch (\Exception $e) {
-            $this->messageManager->addException(
+            $this->messageManager->addExceptionMessage(
                 $e,
                 __('Something went wrong while saving this configuration:') . ' ' . $e->getMessage()
             );

app/code/Magento/Theme/Test/Unit/Controller/Adminhtml/Design/Config/SaveTest.php

@@ -62,15 +62,13 @@ public function setUp()
             '',
             false
         );
-        $this->request = $this->getMockForAbstractClass(
-            \Magento\Framework\App\RequestInterface::class,
-            [],
-            '',
-            false,
-            false,
-            true,
-            ['getFiles', 'getParam', 'getParams']
-        );
+        $this->request = $this->getMockBuilder(\Magento\Framework\App\Request\Http::class)
+            ->disableOriginalConstructor()->getMock();
+
+        $this->request->expects($this->atLeastOnce())
+            ->method('isPost')
+            ->willReturn(true);
+
         $this->context = $objectManager->getObject(
             \Magento\Backend\App\Action\Context::class,
             [
@@ -138,7 +136,7 @@ public function testSave()
             ->method('save')
             ->with($this->designConfig);
         $this->messageManager->expects($this->once())
-            ->method('addSuccess')
+            ->method('addSuccessMessage')
             ->with(__('You saved the configuration.'));
         $this->dataPersistor->expects($this->once())
             ->method('clear')
@@ -194,7 +192,7 @@ public function testSaveWithLocalizedException()
             ->with($this->designConfig)
             ->willThrowException(new \Magento\Framework\Exception\LocalizedException(__('Exception message')));
         $this->messageManager->expects($this->once())
-            ->method('addError')
+            ->method('addErrorMessage')
             ->with(__('Exception message')->render());
 
         $this->dataPersistor->expects($this->once())
@@ -249,7 +247,7 @@ public function testSaveWithException()
             ->with($this->designConfig)
             ->willThrowException($exception);
         $this->messageManager->expects($this->once())
-            ->method('addException')
+            ->method('addExceptionMessage')
             ->with($exception, 'Something went wrong while saving this configuration: Exception message');
 
         $this->dataPersistor->expects($this->once())

dev/tests/integration/testsuite/Magento/Theme/Controller/Adminhtml/System/Design/Config/SaveTest.php

@@ -14,6 +14,11 @@
  */
 class SaveTest extends AbstractBackendController
 {
+    /**
+     * @var FormKey
+     */
+    private $formKey;
+
     /**
      * @inheritdoc
      */
@@ -24,6 +29,15 @@ class SaveTest extends AbstractBackendController
      */
     protected $uri = 'backend/theme/design_config/save';
 
+    protected function setUp()
+    {
+        parent::setUp();
+
+        $this->formKey = $this->_objectManager->get(
+            FormKey::class
+        );
+    }
+
     /**
      * Test design configuration save valid values.
      *
@@ -89,7 +103,22 @@ private function getRequestParams()
             'watermark_swatch_image_imageOpacity' => '',
             'watermark_swatch_image_position' => 'stretch',
             'scope' => 'default',
-            'form_key' => $this->_objectManager->get(FormKey::class)->getFormKey(),
+            'form_key' => $this->formKey->getFormKey(),
         ];
     }
+
+    public function testAclHasAccess()
+    {
+        $this->getRequest()->setMethod(
+            \Zend\Http\Request::METHOD_POST
+        );
+
+        $this->getRequest()->setParams(
+            [
+                'form_key' => $this->formKey->getFormKey()
+            ]
+        );
+
+        parent::testAclHasAccess();
+    }
 }

lib/internal/Magento/Framework/Api/SortOrder.php

@@ -25,13 +25,17 @@ class SortOrder extends AbstractSimpleObject
      * Initialize object and validate sort direction
      *
      * @param array $data
+     * @throws InputException
      */
     public function __construct(array $data = [])
     {
         parent::__construct($data);
         if (null !== $this->getDirection()) {
             $this->validateDirection($this->getDirection());
         }
+        if ($this->getField() !== null) {
+            $this->validateField($this->getField());
+        }
     }
 
     /**
@@ -48,10 +52,14 @@ public function getField()
      * Set sorting field.
      *
      * @param string $field
+     * @throws InputException
+     *
      * @return $this
      */
     public function setField($field)
     {
+        $this->validateField($field);
+
         return $this->setData(SortOrder::FIELD, $field);
     }
 
@@ -69,6 +77,8 @@ public function getDirection()
      * Set sorting direction.
      *
      * @param string $direction
+     * @throws InputException
+     *
      * @return $this
      */
     public function setDirection($direction)
@@ -81,10 +91,10 @@ public function setDirection($direction)
      * Validate direction argument ASC or DESC
      *
      * @param mixed $direction
-     * @return null
+     * @return void
      * @throws InputException
      */
-    private function validateDirection($direction)
+    private function validateDirection($direction): void
     {
         $this->validateDirectionIsString($direction);
         $this->validateDirectionIsAscOrDesc($direction);
@@ -93,9 +103,9 @@ private function validateDirection($direction)
     /**
      * @param string $direction
      * @throws InputException
-     * @return null
+     * @return void
      */
-    private function validateDirectionIsString($direction)
+    private function validateDirectionIsString($direction): void
     {
         if (!is_string($direction)) {
             throw new InputException(new Phrase(
@@ -108,9 +118,9 @@ private function validateDirectionIsString($direction)
     /**
      * @param string $direction
      * @throws InputException
-     * @return null
+     * @return void
      */
-    private function validateDirectionIsAscOrDesc($direction)
+    private function validateDirectionIsAscOrDesc($direction): void
     {
         $normalizedDirection = $this->normalizeDirectionInput($direction);
         if (!in_array($normalizedDirection, [SortOrder::SORT_ASC, SortOrder::SORT_DESC], true)) {
@@ -129,4 +139,23 @@ private function normalizeDirectionInput($direction)
     {
         return strtoupper($direction);
     }
+
+    /**
+     * Check if given value can be used as sorting field.
+     *
+     * @param string $field
+     * @return void
+     * @throws InputException
+     */
+    private function validateField(string $field): void
+    {
+        if (preg_match('/[^a-z0-9\_]/i', $field)) {
+            throw new InputException(
+                new Phrase(
+                    'Sort order field %1 contains restricted symbols',
+                    [$field]
+                )
+            );
+        }
+    }
 }

lib/internal/Magento/Framework/Api/Test/Unit/SortOrderTest.php

@@ -92,4 +92,14 @@ public function testItValidatesADirectionAssignedDuringInstantiation()
             SortOrder::DIRECTION => 'not-asc-or-desc'
         ]);
     }
+
+    /**
+     * @expectedException \Magento\Framework\Exception\InputException
+     */
+    public function testValidateField()
+    {
+        $this->sortOrder = new SortOrder([
+            SortOrder::FIELD => 'invalid field (value);'
+        ]);
+    }
 }