Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In System/Export controlers use MessageManager instead of throwing exceptions #26746

Merged
merged 6 commits into from
Mar 17, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@

use Magento\Backend\App\Action;
use Magento\Framework\App\Action\HttpPostActionInterface;
use Magento\Framework\Controller\ResultFactory;
use Magento\Framework\Exception\FileSystemException;
use Magento\Framework\Exception\LocalizedException;
use Magento\Framework\App\Filesystem\DirectoryList;
use Magento\ImportExport\Controller\Adminhtml\Export as ExportController;
use Magento\Framework\Filesystem;
Expand Down Expand Up @@ -56,29 +54,33 @@ public function __construct(
/**
* Controller basic method implementation.
*
* @return \Magento\Framework\App\ResponseInterface|\Magento\Framework\Controller\ResultInterface
* @throws LocalizedException
* @return \Magento\Framework\Controller\ResultInterface
*/
public function execute()
{
/** @var \Magento\Framework\Controller\Result\Redirect $resultRedirect */
$resultRedirect = $this->resultRedirectFactory->create();
$resultRedirect->setPath('adminhtml/export/index');
$fileName = $this->getRequest()->getParam('filename');
if (empty($fileName) || preg_match('/\.\.(\\\|\/)/', $fileName) !== 0) {
$this->messageManager->addErrorMessage(__('Please provide valid export file name'));

return $resultRedirect;
}
try {
if (empty($fileName = $this->getRequest()->getParam('filename'))) {
throw new LocalizedException(__('Please provide export file name'));
}
$directory = $this->filesystem->getDirectoryRead(DirectoryList::VAR_DIR);
$path = $directory->getAbsolutePath() . 'export/' . $fileName;

if (!$directory->isFile($path)) {
throw new LocalizedException(__('Sorry, but the data is invalid or the file is not uploaded.'));
if ($directory->isFile($path)) {
$this->file->deleteFile($path);
$this->messageManager->addSuccessMessage(__('File %1 deleted', $fileName));
} else {
$this->messageManager->addErrorMessage(__('%1 is not a valid file', $fileName));
}

$this->file->deleteFile($path);
/** @var \Magento\Backend\Model\View\Result\Redirect $resultRedirect */
$resultRedirect = $this->resultFactory->create(ResultFactory::TYPE_REDIRECT);
$resultRedirect->setPath('adminhtml/export/index');
return $resultRedirect;
} catch (FileSystemException $exception) {
throw new LocalizedException(__('There are no export file with such name %1', $fileName));
$this->messageManager->addErrorMessage($exception->getMessage());
}

return $resultRedirect;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
use Magento\Backend\App\Action;
use Magento\Framework\App\Action\HttpGetActionInterface;
use Magento\Framework\App\Response\Http\FileFactory;
use Magento\Framework\Exception\LocalizedException;
use Magento\Framework\App\Filesystem\DirectoryList;
use Magento\ImportExport\Controller\Adminhtml\Export as ExportController;
use Magento\Framework\Filesystem;
Expand Down Expand Up @@ -55,12 +54,17 @@ public function __construct(
* Controller basic method implementation.
*
* @return \Magento\Framework\App\ResponseInterface
* @throws LocalizedException
*/
public function execute()
{
if (empty($fileName = $this->getRequest()->getParam('filename'))) {
throw new LocalizedException(__('Please provide export file name'));
/** @var \Magento\Framework\Controller\Result\Redirect $resultRedirect */
$resultRedirect = $this->resultRedirectFactory->create();
$resultRedirect->setPath('adminhtml/export/index');
$fileName = $this->getRequest()->getParam('filename');
if (empty($fileName) || preg_match('/\.\.(\\\|\/)/', $fileName) !== 0) {
$this->messageManager->addErrorMessage(__('Please provide valid export file name'));

return $resultRedirect;
}
try {
$path = 'export/' . $fileName;
Expand All @@ -72,8 +76,11 @@ public function execute()
DirectoryList::VAR_DIR
);
}
} catch (LocalizedException | \Exception $exception) {
throw new LocalizedException(__('There are no export file with such name %1', $fileName));
$this->messageManager->addErrorMessage(__('%1 is not a valid file', $fileName));
} catch (\Exception $exception) {
$this->messageManager->addErrorMessage($exception->getMessage());
}

return $resultRedirect;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,181 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
eduard13 marked this conversation as resolved.
Show resolved Hide resolved
namespace Magento\ImportExport\Controller\Adminhtml\Export\File;

use Magento\Framework\TestFramework\Unit\Helper\ObjectManager as ObjectManagerHelper;

/**
* @SuppressWarnings(PHPMD.CouplingBetweenObjects)
*/
class DeleteTest extends \PHPUnit\Framework\TestCase
{
/**
* @var \Magento\Backend\App\Action\Context|\PHPUnit_Framework_MockObject_MockObject
eduard13 marked this conversation as resolved.
Show resolved Hide resolved
*/
protected $context;
eduard13 marked this conversation as resolved.
Show resolved Hide resolved

/**
* @var \Magento\Framework\TestFramework\Unit\Helper\ObjectManager
*/
protected $objectManagerHelper;

/**
* @var \Magento\Framework\App\Request\Http|\PHPUnit_Framework_MockObject_MockObject
*/
protected $request;

/**
* @var \Magento\Framework\Controller\Result\Raw|\PHPUnit_Framework_MockObject_MockObject
*/
protected $redirect;

/**
* @var \Magento\Framework\Controller\Result\RedirectFactory|\PHPUnit_Framework_MockObject_MockObject
*/
protected $resultRedirectFactory;

/**
* @var \Magento\Framework\Filesystem|\PHPUnit_Framework_MockObject_MockObject
*/
protected $fileSystem;

/**
* @var \Magento\Framework\Filesystem\DriverInterface|\PHPUnit_Framework_MockObject_MockObject
*/
protected $file;

/**
* @var \Magento\ImportExport\Controller\Adminhtml\Export\File\Delete|\PHPUnit_Framework_MockObject_MockObject
*/
protected $deleteController;

/**
* @var \Magento\Framework\Message\ManagerInterface|\PHPUnit_Framework_MockObject_MockObject
*/
protected $messageManager;

/**
* @var \Magento\Framework\Filesystem\Directory\ReadInterface|\PHPUnit_Framework_MockObject_MockObject
*/
protected $directory;

/**
* Set up
*/
protected function setUp()
{
$this->request = $this->getMockBuilder(\Magento\Framework\App\Request\Http::class)
->disableOriginalConstructor()
->getMock();

$this->fileSystem = $this->getMockBuilder(\Magento\Framework\Filesystem::class)
->disableOriginalConstructor()
->getMock();

$this->directory = $this->getMockBuilder(\Magento\Framework\Filesystem\Directory\ReadInterface::class)
->disableOriginalConstructor()
->getMock();

$this->file = $this->getMockBuilder(\Magento\Framework\Filesystem\DriverInterface::class)
->disableOriginalConstructor()
->getMock();

$this->messageManager = $this->getMockBuilder(\Magento\Framework\Message\ManagerInterface::class)
->disableOriginalConstructor()
->getMock();

$this->context = $this->createPartialMock(
\Magento\Backend\App\Action\Context::class,
['getRequest', 'getResultRedirectFactory', 'getMessageManager']
);

$this->redirect = $this->createPartialMock(\Magento\Backend\Model\View\Result\Redirect::class, ['setPath']);

$this->resultRedirectFactory = $this->createPartialMock(
\Magento\Framework\Controller\Result\RedirectFactory::class,
['create']
);
$this->resultRedirectFactory->expects($this->any())->method('create')->willReturn($this->redirect);
$this->context->expects($this->any())->method('getRequest')->willReturn($this->request);
$this->context->expects($this->any())
->method('getResultRedirectFactory')
->willReturn($this->resultRedirectFactory);

$this->context->expects($this->any())
->method('getMessageManager')
->willReturn($this->messageManager);


$this->objectManagerHelper = new ObjectManagerHelper($this);
$this->deleteController = $this->objectManagerHelper->getObject(
Delete::class,
[
'context' => $this->context,
'filesystem' => $this->fileSystem,
'file' => $this->file
]
);
}

/**
* Tests download controller with different file names in request.
*/
public function testExecuteSuccess()
{
$this->request->method('getParam')
->with('filename')
->willReturn('sampleFile');

$this->fileSystem->expects($this->once())->method('getDirectoryRead')->will($this->returnValue($this->directory));
$this->directory->expects($this->once())->method('isFile')->willReturn(true);
$this->file->expects($this->once())->method('deleteFile')->willReturn(true);
$this->messageManager->expects($this->once())->method('addSuccessMessage');

$this->deleteController->execute();
}

/**
* Tests download controller with different file names in request.

eduard13 marked this conversation as resolved.
Show resolved Hide resolved
*/
public function testExecuteFileDoesntExists()
{
$this->request->method('getParam')
->with('filename')
->willReturn('sampleFile');

$this->fileSystem->expects($this->once())->method('getDirectoryRead')->will($this->returnValue($this->directory));
$this->directory->expects($this->once())->method('isFile')->willReturn(false);
$this->messageManager->expects($this->once())->method('addErrorMessage');

$this->deleteController->execute();
}

/**
* Test execute() with invalid file name
* @param string $requestFilename
* @dataProvider executeDataProvider
*/
public function testExecuteInvalidFileName($requestFilename)
{
$this->request->method('getParam')->with('filename')->willReturn($requestFilename);
$this->messageManager->expects($this->once())->method('addErrorMessage');

$this->deleteController->execute();
}

/**
* @return array
eduard13 marked this conversation as resolved.
Show resolved Hide resolved
*/
public function executeDataProvider()
{
return [
'Relative file name' => ['../.htaccess'],
'Empty file name' => [''],
'Null file name' => [null],
];
}
}
Loading