Skip to content

Commit c5bb3cb

Browse files
authored
ENGCOM-7083: In System/Export controlers use MessageManager instead of throwing exceptions #26746
2 parents 3a803bb + 27d165c commit c5bb3cb

File tree

5 files changed

+438
-22
lines changed

5 files changed

+438
-22
lines changed

app/code/Magento/ImportExport/Controller/Adminhtml/Export/File/Delete.php

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,7 @@
99

1010
use Magento\Backend\App\Action;
1111
use Magento\Framework\App\Action\HttpPostActionInterface;
12-
use Magento\Framework\Controller\ResultFactory;
1312
use Magento\Framework\Exception\FileSystemException;
14-
use Magento\Framework\Exception\LocalizedException;
1513
use Magento\Framework\App\Filesystem\DirectoryList;
1614
use Magento\ImportExport\Controller\Adminhtml\Export as ExportController;
1715
use Magento\Framework\Filesystem;
@@ -56,29 +54,33 @@ public function __construct(
5654
/**
5755
* Controller basic method implementation.
5856
*
59-
* @return \Magento\Framework\App\ResponseInterface|\Magento\Framework\Controller\ResultInterface
60-
* @throws LocalizedException
57+
* @return \Magento\Framework\Controller\ResultInterface
6158
*/
6259
public function execute()
6360
{
61+
/** @var \Magento\Framework\Controller\Result\Redirect $resultRedirect */
62+
$resultRedirect = $this->resultRedirectFactory->create();
63+
$resultRedirect->setPath('adminhtml/export/index');
64+
$fileName = $this->getRequest()->getParam('filename');
65+
if (empty($fileName) || preg_match('/\.\.(\\\|\/)/', $fileName) !== 0) {
66+
$this->messageManager->addErrorMessage(__('Please provide valid export file name'));
67+
68+
return $resultRedirect;
69+
}
6470
try {
65-
if (empty($fileName = $this->getRequest()->getParam('filename'))) {
66-
throw new LocalizedException(__('Please provide export file name'));
67-
}
6871
$directory = $this->filesystem->getDirectoryRead(DirectoryList::VAR_DIR);
6972
$path = $directory->getAbsolutePath() . 'export/' . $fileName;
7073

71-
if (!$directory->isFile($path)) {
72-
throw new LocalizedException(__('Sorry, but the data is invalid or the file is not uploaded.'));
74+
if ($directory->isFile($path)) {
75+
$this->file->deleteFile($path);
76+
$this->messageManager->addSuccessMessage(__('File %1 deleted', $fileName));
77+
} else {
78+
$this->messageManager->addErrorMessage(__('%1 is not a valid file', $fileName));
7379
}
74-
75-
$this->file->deleteFile($path);
76-
/** @var \Magento\Backend\Model\View\Result\Redirect $resultRedirect */
77-
$resultRedirect = $this->resultFactory->create(ResultFactory::TYPE_REDIRECT);
78-
$resultRedirect->setPath('adminhtml/export/index');
79-
return $resultRedirect;
8080
} catch (FileSystemException $exception) {
81-
throw new LocalizedException(__('There are no export file with such name %1', $fileName));
81+
$this->messageManager->addErrorMessage($exception->getMessage());
8282
}
83+
84+
return $resultRedirect;
8385
}
8486
}

app/code/Magento/ImportExport/Controller/Adminhtml/Export/File/Download.php

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
use Magento\Backend\App\Action;
1111
use Magento\Framework\App\Action\HttpGetActionInterface;
1212
use Magento\Framework\App\Response\Http\FileFactory;
13-
use Magento\Framework\Exception\LocalizedException;
1413
use Magento\Framework\App\Filesystem\DirectoryList;
1514
use Magento\ImportExport\Controller\Adminhtml\Export as ExportController;
1615
use Magento\Framework\Filesystem;
@@ -55,12 +54,17 @@ public function __construct(
5554
* Controller basic method implementation.
5655
*
5756
* @return \Magento\Framework\App\ResponseInterface
58-
* @throws LocalizedException
5957
*/
6058
public function execute()
6159
{
62-
if (empty($fileName = $this->getRequest()->getParam('filename'))) {
63-
throw new LocalizedException(__('Please provide export file name'));
60+
/** @var \Magento\Framework\Controller\Result\Redirect $resultRedirect */
61+
$resultRedirect = $this->resultRedirectFactory->create();
62+
$resultRedirect->setPath('adminhtml/export/index');
63+
$fileName = $this->getRequest()->getParam('filename');
64+
if (empty($fileName) || preg_match('/\.\.(\\\|\/)/', $fileName) !== 0) {
65+
$this->messageManager->addErrorMessage(__('Please provide valid export file name'));
66+
67+
return $resultRedirect;
6468
}
6569
try {
6670
$path = 'export/' . $fileName;
@@ -72,8 +76,11 @@ public function execute()
7276
DirectoryList::VAR_DIR
7377
);
7478
}
75-
} catch (LocalizedException | \Exception $exception) {
76-
throw new LocalizedException(__('There are no export file with such name %1', $fileName));
79+
$this->messageManager->addErrorMessage(__('%1 is not a valid file', $fileName));
80+
} catch (\Exception $exception) {
81+
$this->messageManager->addErrorMessage($exception->getMessage());
7782
}
83+
84+
return $resultRedirect;
7885
}
7986
}
Lines changed: 198 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,198 @@
1+
<?php
2+
/**
3+
* Copyright © Magento, Inc. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
declare(strict_types=1);
7+
8+
namespace Magento\ImportExport\Test\Unit\Controller\Adminhtml\Export\File;
9+
10+
use Magento\Backend\App\Action\Context;
11+
use Magento\Backend\Model\View\Result\Redirect;
12+
use Magento\Framework\App\Request\Http;
13+
use Magento\Framework\Controller\Result\Raw;
14+
use Magento\Framework\Controller\Result\RedirectFactory;
15+
use Magento\Framework\Filesystem;
16+
use Magento\Framework\Filesystem\Directory\ReadInterface;
17+
use Magento\Framework\Filesystem\DriverInterface;
18+
use Magento\Framework\Message\ManagerInterface;
19+
use Magento\Framework\TestFramework\Unit\Helper\ObjectManager as ObjectManagerHelper;
20+
use Magento\ImportExport\Controller\Adminhtml\Export\File\Delete;
21+
use PHPUnit\Framework\MockObject\MockObject;
22+
use PHPUnit\Framework\TestCase;
23+
24+
/**
25+
* @SuppressWarnings(PHPMD.CouplingBetweenObjects)
26+
*/
27+
class DeleteTest extends TestCase
28+
{
29+
/**
30+
* @var Context|MockObject
31+
*/
32+
private $contextMock;
33+
34+
/**
35+
* @var ObjectManagerHelper
36+
*/
37+
private $objectManagerHelper;
38+
39+
/**
40+
* @var Http|MockObject
41+
*/
42+
private $requestMock;
43+
44+
/**
45+
* @var Raw|MockObject
46+
*/
47+
private $redirectMock;
48+
49+
/**
50+
* @var RedirectFactory|MockObject
51+
*/
52+
private $resultRedirectFactoryMock;
53+
54+
/**
55+
* @var Filesystem|MockObject
56+
*/
57+
private $fileSystemMock;
58+
59+
/**
60+
* @var DriverInterface|MockObject
61+
*/
62+
private $fileMock;
63+
64+
/**
65+
* @var Delete|MockObject
66+
*/
67+
private $deleteControllerMock;
68+
69+
/**
70+
* @var ManagerInterface|MockObject
71+
*/
72+
private $messageManagerMock;
73+
74+
/**
75+
* @var ReadInterface|MockObject
76+
*/
77+
private $directoryMock;
78+
79+
/**
80+
* Set up
81+
*/
82+
protected function setUp()
83+
{
84+
$this->requestMock = $this->getMockBuilder(Http::class)
85+
->disableOriginalConstructor()
86+
->getMock();
87+
88+
$this->fileSystemMock = $this->getMockBuilder(Filesystem::class)
89+
->disableOriginalConstructor()
90+
->getMock();
91+
92+
$this->directoryMock = $this->getMockBuilder(ReadInterface::class)
93+
->disableOriginalConstructor()
94+
->getMock();
95+
96+
$this->fileMock = $this->getMockBuilder(DriverInterface::class)
97+
->disableOriginalConstructor()
98+
->getMock();
99+
100+
$this->messageManagerMock = $this->getMockBuilder(ManagerInterface::class)
101+
->disableOriginalConstructor()
102+
->getMock();
103+
104+
$this->contextMock = $this->createPartialMock(
105+
Context::class,
106+
['getRequest', 'getResultRedirectFactory', 'getMessageManager']
107+
);
108+
109+
$this->redirectMock = $this->createPartialMock(Redirect::class, ['setPath']);
110+
111+
$this->resultRedirectFactoryMock = $this->createPartialMock(
112+
RedirectFactory::class,
113+
['create']
114+
);
115+
$this->resultRedirectFactoryMock->expects($this->any())->method('create')->willReturn($this->redirectMock);
116+
$this->contextMock->expects($this->any())->method('getRequest')->willReturn($this->requestMock);
117+
$this->contextMock->expects($this->any())
118+
->method('getResultRedirectFactory')
119+
->willReturn($this->resultRedirectFactoryMock);
120+
121+
$this->contextMock->expects($this->any())
122+
->method('getMessageManager')
123+
->willReturn($this->messageManagerMock);
124+
125+
$this->objectManagerHelper = new ObjectManagerHelper($this);
126+
$this->deleteControllerMock = $this->objectManagerHelper->getObject(
127+
Delete::class,
128+
[
129+
'context' => $this->contextMock,
130+
'filesystem' => $this->fileSystemMock,
131+
'file' => $this->fileMock
132+
]
133+
);
134+
}
135+
136+
/**
137+
* Tests download controller with different file names in request.
138+
*/
139+
public function testExecuteSuccess()
140+
{
141+
$this->requestMock->method('getParam')
142+
->with('filename')
143+
->willReturn('sampleFile');
144+
145+
$this->fileSystemMock->expects($this->once())
146+
->method('getDirectoryRead')
147+
->will($this->returnValue($this->directoryMock));
148+
$this->directoryMock->expects($this->once())->method('isFile')->willReturn(true);
149+
$this->fileMock->expects($this->once())->method('deleteFile')->willReturn(true);
150+
$this->messageManagerMock->expects($this->once())->method('addSuccessMessage');
151+
152+
$this->deleteControllerMock->execute();
153+
}
154+
155+
/**
156+
* Tests download controller with different file names in request.
157+
*/
158+
public function testExecuteFileDoesntExists()
159+
{
160+
$this->requestMock->method('getParam')
161+
->with('filename')
162+
->willReturn('sampleFile');
163+
164+
$this->fileSystemMock->expects($this->once())
165+
->method('getDirectoryRead')
166+
->will($this->returnValue($this->directoryMock));
167+
$this->directoryMock->expects($this->once())->method('isFile')->willReturn(false);
168+
$this->messageManagerMock->expects($this->once())->method('addErrorMessage');
169+
170+
$this->deleteControllerMock->execute();
171+
}
172+
173+
/**
174+
* Test execute() with invalid file name
175+
* @param string $requestFilename
176+
* @dataProvider invalidFileDataProvider
177+
*/
178+
public function testExecuteInvalidFileName($requestFilename)
179+
{
180+
$this->requestMock->method('getParam')->with('filename')->willReturn($requestFilename);
181+
$this->messageManagerMock->expects($this->once())->method('addErrorMessage');
182+
183+
$this->deleteControllerMock->execute();
184+
}
185+
186+
/**
187+
* Data provider to test possible invalid filenames
188+
* @return array
189+
*/
190+
public function invalidFileDataProvider()
191+
{
192+
return [
193+
'Relative file name' => ['../.htaccess'],
194+
'Empty file name' => [''],
195+
'Null file name' => [null],
196+
];
197+
}
198+
}

0 commit comments

Comments
 (0)