diff --git a/app/code/Magento/Sitemap/Block/Adminhtml/Edit/Form.php b/app/code/Magento/Sitemap/Block/Adminhtml/Edit/Form.php index 5e90cf6e12f6e..91b6a8446894b 100644 --- a/app/code/Magento/Sitemap/Block/Adminhtml/Edit/Form.php +++ b/app/code/Magento/Sitemap/Block/Adminhtml/Edit/Form.php @@ -48,6 +48,8 @@ protected function _construct() } /** + * Configure form for sitemap. + * * @return $this */ protected function _prepareForm() @@ -73,7 +75,8 @@ protected function _prepareForm() 'name' => 'sitemap_filename', 'required' => true, 'note' => __('example: sitemap.xml'), - 'value' => $model->getSitemapFilename() + 'value' => $model->getSitemapFilename(), + 'class' => 'validate-length maximum-length-32' ] ); diff --git a/app/code/Magento/Sitemap/Controller/Adminhtml/Sitemap/Save.php b/app/code/Magento/Sitemap/Controller/Adminhtml/Sitemap/Save.php index 1e0d1cb248f00..5230de0429778 100644 --- a/app/code/Magento/Sitemap/Controller/Adminhtml/Sitemap/Save.php +++ b/app/code/Magento/Sitemap/Controller/Adminhtml/Sitemap/Save.php @@ -5,12 +5,74 @@ */ namespace Magento\Sitemap\Controller\Adminhtml\Sitemap; -use Magento\Backend\App\Action; +use Magento\Backend\App\Action\Context; +use Magento\Framework\App\Action\HttpPostActionInterface; use Magento\Framework\App\Filesystem\DirectoryList; use Magento\Framework\Controller; +use Magento\Framework\Validator\StringLength; +use Magento\MediaStorage\Model\File\Validator\AvailablePath; +use Magento\Sitemap\Model\SitemapFactory; -class Save extends \Magento\Sitemap\Controller\Adminhtml\Sitemap +/** + * Save sitemap controller. + */ +class Save extends \Magento\Sitemap\Controller\Adminhtml\Sitemap implements HttpPostActionInterface { + /** + * Maximum length of sitemap filename + */ + const MAX_FILENAME_LENGTH = 32; + + /** + * @var StringLength + */ + private $stringValidator; + + /** + * @var AvailablePath + */ + private $pathValidator; + + /** + * @var \Magento\Sitemap\Helper\Data + */ + private $sitemapHelper; + + /** + * @var \Magento\Framework\Filesystem + */ + private $filesystem; + + /** + * @var SitemapFactory + */ + private $sitemapFactory; + + /** + * Save constructor. + * @param Context $context + * @param StringLength $stringValidator + * @param AvailablePath $pathValidator + * @param \Magento\Sitemap\Helper\Data $sitemapHelper + * @param \Magento\Framework\Filesystem $filesystem + * @param SitemapFactory $sitemapFactory + */ + public function __construct( + Context $context, + StringLength $stringValidator = null, + AvailablePath $pathValidator = null, + \Magento\Sitemap\Helper\Data $sitemapHelper = null, + \Magento\Framework\Filesystem $filesystem = null, + SitemapFactory $sitemapFactory = null + ) { + parent::__construct($context); + $this->stringValidator = $stringValidator ?: $this->_objectManager->get(StringLength::class); + $this->pathValidator = $pathValidator ?: $this->_objectManager->get(AvailablePath::class); + $this->sitemapHelper = $sitemapHelper ?: $this->_objectManager->get(\Magento\Sitemap\Helper\Data::class); + $this->filesystem = $filesystem ?: $this->_objectManager->get(\Magento\Framework\Filesystem::class); + $this->sitemapFactory = $sitemapFactory ?: $this->_objectManager->get(SitemapFactory::class); + } + /** * Validate path for generation * @@ -23,17 +85,25 @@ protected function validatePath(array $data) if (!empty($data['sitemap_filename']) && !empty($data['sitemap_path'])) { $data['sitemap_path'] = '/' . ltrim($data['sitemap_path'], '/'); $path = rtrim($data['sitemap_path'], '\\/') . '/' . $data['sitemap_filename']; - /** @var $validator \Magento\MediaStorage\Model\File\Validator\AvailablePath */ - $validator = $this->_objectManager->create(\Magento\MediaStorage\Model\File\Validator\AvailablePath::class); - /** @var $helper \Magento\Sitemap\Helper\Data */ - $helper = $this->_objectManager->get(\Magento\Sitemap\Helper\Data::class); - $validator->setPaths($helper->getValidPaths()); - if (!$validator->isValid($path)) { - foreach ($validator->getMessages() as $message) { + $this->pathValidator->setPaths($this->sitemapHelper->getValidPaths()); + if (!$this->pathValidator->isValid($path)) { + foreach ($this->pathValidator->getMessages() as $message) { + $this->messageManager->addErrorMessage($message); + } + // save data in session + $this->_session->setFormData($data); + // redirect to edit form + return false; + } + + $filename = rtrim($data['sitemap_filename']); + $this->stringValidator->setMax(self::MAX_FILENAME_LENGTH); + if (!$this->stringValidator->isValid($filename)) { + foreach ($this->stringValidator->getMessages() as $message) { $this->messageManager->addErrorMessage($message); } // save data in session - $this->_objectManager->get(\Magento\Backend\Model\Session::class)->setFormData($data); + $this->_session->setFormData($data); // redirect to edit form return false; } @@ -49,9 +119,8 @@ protected function validatePath(array $data) */ protected function clearSiteMap(\Magento\Sitemap\Model\Sitemap $model) { - /** @var \Magento\Framework\Filesystem\Directory\Write $directory */ - $directory = $this->_objectManager->get(\Magento\Framework\Filesystem::class) - ->getDirectoryWrite(DirectoryList::ROOT); + /** @var \Magento\Framework\Filesystem $directory */ + $directory = $this->filesystem->getDirectoryWrite(DirectoryList::ROOT); if ($this->getRequest()->getParam('sitemap_id')) { $model->load($this->getRequest()->getParam('sitemap_id')); @@ -74,7 +143,7 @@ protected function saveData($data) { // init model and set data /** @var \Magento\Sitemap\Model\Sitemap $model */ - $model = $this->_objectManager->create(\Magento\Sitemap\Model\Sitemap::class); + $model = $this->sitemapFactory->create(); $this->clearSiteMap($model); $model->setData($data); @@ -85,13 +154,13 @@ protected function saveData($data) // display success message $this->messageManager->addSuccessMessage(__('You saved the sitemap.')); // clear previously saved data from session - $this->_objectManager->get(\Magento\Backend\Model\Session::class)->setFormData(false); + $this->_session->setFormData(false); return $model->getId(); } catch (\Exception $e) { // display error message $this->messageManager->addErrorMessage($e->getMessage()); // save data in session - $this->_objectManager->get(\Magento\Backend\Model\Session::class)->setFormData($data); + $this->_session->setFormData($data); } return false; } diff --git a/app/code/Magento/Sitemap/Test/Unit/Controller/Adminhtml/Sitemap/SaveTest.php b/app/code/Magento/Sitemap/Test/Unit/Controller/Adminhtml/Sitemap/SaveTest.php index f77954101df7c..00f51b7e6c23f 100644 --- a/app/code/Magento/Sitemap/Test/Unit/Controller/Adminhtml/Sitemap/SaveTest.php +++ b/app/code/Magento/Sitemap/Test/Unit/Controller/Adminhtml/Sitemap/SaveTest.php @@ -7,51 +7,83 @@ use Magento\Framework\TestFramework\Unit\Helper\ObjectManager as ObjectManagerHelper; use Magento\Framework\Controller\ResultFactory; +use Magento\Sitemap\Controller\Adminhtml\Sitemap\Save; +/** + * @SuppressWarnings(PHPMD.CouplingBetweenObjects) + */ class SaveTest extends \PHPUnit\Framework\TestCase { /** * @var \Magento\Sitemap\Controller\Adminhtml\Sitemap\Save */ - protected $saveController; + private $saveController; /** * @var \Magento\Backend\App\Action\Context */ - protected $context; - - /** - * @var \Magento\Framework\TestFramework\Unit\Helper\ObjectManager; - */ - protected $objectManagerHelper; + private $contextMock; /** * @var \Magento\Framework\HTTP\PhpEnvironment\Request|\PHPUnit_Framework_MockObject_MockObject */ - protected $requestMock; + private $requestMock; /** * @var \Magento\Framework\Controller\ResultFactory|\PHPUnit_Framework_MockObject_MockObject */ - protected $resultFactoryMock; + private $resultFactoryMock; /** * @var \Magento\Backend\Model\View\Result\Redirect|\PHPUnit_Framework_MockObject_MockObject */ - protected $resultRedirectMock; + private $resultRedirectMock; /** * @var \Magento\Framework\ObjectManagerInterface|\PHPUnit_Framework_MockObject_MockObject */ - protected $objectManagerMock; + private $objectManagerMock; /** * @var \Magento\Framework\Message\ManagerInterface|\PHPUnit_Framework_MockObject_MockObject */ - protected $messageManagerMock; + private $messageManagerMock; + + /** + * @var \Magento\Framework\Validator\StringLength|\PHPUnit_Framework_MockObject_MockObject + */ + private $lengthValidator; + + /** + * @var \Magento\MediaStorage\Model\File\Validator\AvailablePath|\PHPUnit_Framework_MockObject_MockObject + */ + private $pathValidator; + + /** + * @var \Magento\Sitemap\Helper\Data|\PHPUnit_Framework_MockObject_MockObject + */ + private $helper; + + /** + * @var \Magento\Framework\Filesystem|\PHPUnit_Framework_MockObject_MockObject + */ + private $fileSystem; + + /** + * @var \Magento\Sitemap\Model\SitemapFactory|\PHPUnit_Framework_MockObject_MockObject + */ + private $siteMapFactory; + + /** + * @var \Magento\Backend\Model\Session|\PHPUnit_Framework_MockObject_MockObject + */ + private $session; protected function setUp() { + $this->contextMock = $this->getMockBuilder(\Magento\Backend\App\Action\Context::class) + ->disableOriginalConstructor() + ->getMock(); $this->requestMock = $this->getMockBuilder(\Magento\Framework\App\RequestInterface::class) ->disableOriginalConstructor() ->setMethods(['getPostValue']) @@ -66,27 +98,48 @@ protected function setUp() ->getMock(); $this->messageManagerMock = $this->getMockBuilder(\Magento\Framework\Message\ManagerInterface::class) ->getMock(); - + $this->helper = $this->getMockBuilder(\Magento\Sitemap\Helper\Data::class) + ->disableOriginalConstructor() + ->getMock(); $this->resultFactoryMock->expects($this->once()) ->method('create') ->with(ResultFactory::TYPE_REDIRECT) ->willReturn($this->resultRedirectMock); + $this->session = $this->getMockBuilder(\Magento\Backend\Model\Session::class) + ->disableOriginalConstructor() + ->setMethods(['setFormData']) + ->getMock(); - $this->objectManagerHelper = new ObjectManagerHelper($this); - $this->context = $this->objectManagerHelper->getObject( - \Magento\Backend\App\Action\Context::class, - [ - 'resultFactory' => $this->resultFactoryMock, - 'request' => $this->requestMock, - 'messageManager' => $this->messageManagerMock, - 'objectManager' => $this->objectManagerMock - ] - ); - $this->saveController = $this->objectManagerHelper->getObject( - \Magento\Sitemap\Controller\Adminhtml\Sitemap\Save::class, - [ - 'context' => $this->context - ] + $this->contextMock->expects($this->once()) + ->method('getMessageManager') + ->willReturn($this->messageManagerMock); + $this->contextMock->expects($this->once()) + ->method('getRequest') + ->willReturn($this->requestMock); + $this->contextMock->expects($this->once()) + ->method('getResultFactory') + ->willReturn($this->resultFactoryMock); + $this->contextMock->expects($this->once()) + ->method('getSession') + ->willReturn($this->session); + + $this->lengthValidator = $this->getMockBuilder(\Magento\Framework\Validator\StringLength::class) + ->disableOriginalConstructor() + ->getMock(); + $this->pathValidator = + $this->getMockBuilder(\Magento\MediaStorage\Model\File\Validator\AvailablePath::class) + ->disableOriginalConstructor() + ->getMock(); + $this->fileSystem = $this->createMock(\Magento\Framework\Filesystem::class); + $this->siteMapFactory = $this->createMock(\Magento\Sitemap\Model\SitemapFactory::class); + + $this->saveController = new Save( + $this->contextMock, + $this->lengthValidator, + $this->pathValidator, + $this->helper, + $this->fileSystem, + $this->siteMapFactory ); } @@ -105,11 +158,8 @@ public function testSaveEmptyDataShouldRedirectToDefault() public function testTryToSaveInvalidDataShouldFailWithErrors() { - $validatorClass = \Magento\MediaStorage\Model\File\Validator\AvailablePath::class; - $helperClass = \Magento\Sitemap\Helper\Data::class; $validPaths = []; $messages = ['message1', 'message2']; - $sessionClass = \Magento\Backend\Model\Session::class; $data = ['sitemap_filename' => 'sitemap_filename', 'sitemap_path' => '/sitemap_path']; $siteMapId = 1; @@ -121,37 +171,83 @@ public function testTryToSaveInvalidDataShouldFailWithErrors() ->with('sitemap_id') ->willReturn($siteMapId); - $validator = $this->createMock($validatorClass); - $validator->expects($this->once()) + $this->pathValidator->expects($this->once()) ->method('setPaths') ->with($validPaths) ->willReturnSelf(); - $validator->expects($this->once()) + $this->pathValidator->expects($this->once()) ->method('isValid') ->with('/sitemap_path/sitemap_filename') ->willReturn(false); - $validator->expects($this->once()) + $this->pathValidator->expects($this->once()) ->method('getMessages') ->willReturn($messages); - $helper = $this->createMock($helperClass); - $helper->expects($this->once()) + $this->helper->expects($this->once()) ->method('getValidPaths') ->willReturn($validPaths); - $session = $this->createPartialMock($sessionClass, ['setFormData']); - $session->expects($this->once()) + $this->session->expects($this->once()) ->method('setFormData') ->with($data) ->willReturnSelf(); - $this->objectManagerMock->expects($this->once()) - ->method('create') - ->with($validatorClass) - ->willReturn($validator); - $this->objectManagerMock->expects($this->any()) - ->method('get') - ->willReturnMap([[$helperClass, $helper], [$sessionClass, $session]]); + $this->messageManagerMock->expects($this->at(0)) + ->method('addErrorMessage') + ->withConsecutive( + [$messages[0]], + [$messages[1]] + ) + ->willReturnSelf(); + + $this->resultRedirectMock->expects($this->once()) + ->method('setPath') + ->with('adminhtml/*/edit', ['sitemap_id' => $siteMapId]) + ->willReturnSelf(); + + $this->assertSame($this->resultRedirectMock, $this->saveController->execute()); + } + + public function testTryToSaveInvalidFileNameShouldFailWithErrors() + { + $validPaths = []; + $messages = ['message1', 'message2']; + $data = ['sitemap_filename' => 'sitemap_filename', 'sitemap_path' => '/sitemap_path']; + $siteMapId = 1; + + $this->requestMock->expects($this->once()) + ->method('getPostValue') + ->willReturn($data); + $this->requestMock->expects($this->once()) + ->method('getParam') + ->with('sitemap_id') + ->willReturn($siteMapId); + + $this->lengthValidator->expects($this->once()) + ->method('isValid') + ->with('sitemap_filename') + ->willReturn(false); + $this->lengthValidator->expects($this->once()) + ->method('getMessages') + ->willReturn($messages); + + $this->pathValidator->expects($this->once()) + ->method('setPaths') + ->with($validPaths) + ->willReturnSelf(); + $this->pathValidator->expects($this->once()) + ->method('isValid') + ->with('/sitemap_path/sitemap_filename') + ->willReturn(true); + + $this->helper->expects($this->once()) + ->method('getValidPaths') + ->willReturn($validPaths); + + $this->session->expects($this->once()) + ->method('setFormData') + ->with($data) + ->willReturnSelf(); $this->messageManagerMock->expects($this->at(0)) ->method('addErrorMessage')