Refactored Source For ImportExport module, related to #7469 #9480
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| <?php | ||
| /** | ||
| * Copyright © Magento, Inc. All rights reserved. | ||
| * See COPYING.txt for license details. | ||
| */ | ||
|
|
||
| namespace Magento\ImportExport\Model\Import\Source; | ||
|
|
||
| use Magento\ImportExport\Model\Import\AbstractSource; | ||
|
|
||
| class File extends AbstractSource | ||
| { | ||
| private $parser; | ||
|
|
||
| public function __construct(FileParser\ParserInterface $parser) | ||
|
|
||
| { | ||
| $this->parser = $parser; | ||
| parent::__construct($this->parser->getColumnNames()); | ||
|
There was a problem hiding this comment. it's not correct to make computation in constructors, Magento encourages developers to use constructors just for assignments |
||
| } | ||
|
|
||
| protected function _getNextRow() | ||
| { | ||
| return $this->parser->fetchRow(); | ||
| } | ||
|
|
||
| public function rewind() | ||
|
|
||
| { | ||
| $this->parser->reset(); | ||
| parent::rewind(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| <?php | ||
| /** | ||
| * Copyright © Magento, Inc. All rights reserved. | ||
| * See COPYING.txt for license details. | ||
| */ | ||
|
|
||
| namespace Magento\ImportExport\Model\Import\Source; | ||
|
|
||
|
|
||
| use Magento\Framework\ObjectManagerInterface; | ||
| use Magento\ImportExport\Model\Import\AbstractSource; | ||
| use Magento\ImportExport\Model\Import\Source\FileParser\ParserFactoryInterface; | ||
| use Magento\ImportExport\Model\Import\Source\FileParser\ParserInterface; | ||
|
|
||
| class FileFactory | ||
| { | ||
| /** @var ParserFactoryInterface */ | ||
| private $parserFactory; | ||
|
|
||
| /** @var ObjectManagerInterface */ | ||
| private $objectManager; | ||
|
|
||
| public function __construct(ParserFactoryInterface $parserFactory, ObjectManagerInterface $objectManager) | ||
| { | ||
| $this->parserFactory = $parserFactory; | ||
| $this->objectManager = $objectManager; | ||
| } | ||
|
|
||
| /** | ||
| * Creates file source from file path | ||
| * | ||
| * @param string $path | ||
| * @param array $options | ||
| * @return AbstractSource | ||
| */ | ||
| public function createFromFilePath($path, array $options = []) | ||
| { | ||
| return $this->createFromFileParser( | ||
| $this->parserFactory->create($path, $options) | ||
| ); | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. why do we have two different methods of creation in the same class? |
||
| /** | ||
| * Creates file source from file parser | ||
| * | ||
| * @param ParserInterface $parser | ||
| * @return AbstractSource | ||
| */ | ||
| public function createFromFileParser(ParserInterface $parser) | ||
| { | ||
| return $this->objectManager->create(File::class, ['parser' => $parser]); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| <?php | ||
| /** | ||
| * Copyright © Magento, Inc. All rights reserved. | ||
| * See COPYING.txt for license details. | ||
| */ | ||
|
|
||
| namespace Magento\ImportExport\Model\Import\Source\FileParser; | ||
|
|
||
| /** | ||
| * File Parser Composite | ||
|
There was a problem hiding this comment. should be marked as @ api , because used as an Extension Point |
||
| * | ||
| */ | ||
| class CompositeParserFactory implements ParserFactoryInterface | ||
| { | ||
| /** @var ParserFactoryInterface[] */ | ||
| private $parserFactories; | ||
|
|
||
| public function __construct(array $parserFactories = []) | ||
| { | ||
| $this->parserFactories = []; | ||
|
|
||
| foreach ($parserFactories as $parserFactory) { | ||
| $this->addParserFactory($parserFactory); | ||
| } | ||
|
There was a problem hiding this comment. there should not be logic in the constructors , just assignments There was a problem hiding this comment.
There was a problem hiding this comment. Variadic types are not supported by DI and I doubt it should be supported. For now, there are two possibilities to make a type check :
// Type check for array to make sure
// that all parser factories implement ParserFactoryInterface
$this->parserFactories = (function(ParserFactoryInterface ...$parserFactories) {
return $parserFactories;
})(...$parserFactories); |
||
| } | ||
|
|
||
| public function create($path, array $options = []) | ||
| { | ||
| foreach ($this->parserFactories as $parserFactory) { | ||
| $parser = $this->createParserIfSupported($parserFactory, $path, $options); | ||
|
There was a problem hiding this comment. this does not look like a proper Object Oriented way of polymorphic object creation. because we instantiate in advance all the factories and just looping through all of them trying to instantiate needed parser |
||
|
|
||
| if ($parser === false) { | ||
| continue; | ||
| } | ||
|
|
||
| return $parser; | ||
| } | ||
|
|
||
| $this->thereWasNoParserFound($path); | ||
| } | ||
|
|
||
| public function addParserFactory(ParserFactoryInterface $parserFactory) | ||
|
There was a problem hiding this comment. this method should not be public, actually I doubt we need it at all , because all it does is just add item to the array |
||
| { | ||
| $this->parserFactories[] = $parserFactory; | ||
| } | ||
|
|
||
| private function thereWasNoParserFound($path) | ||
| { | ||
| throw new UnsupportedPathException($path); | ||
| } | ||
|
|
||
| private function createParserIfSupported(ParserFactoryInterface $parserFactory, $path, array $options) | ||
| { | ||
| try { | ||
| $parser = $parserFactory->create($path, $options); | ||
| } catch (UnsupportedPathException $e) { | ||
| return false; | ||
|
There was a problem hiding this comment. this does not look like a proper way of work with Exceptions There was a problem hiding this comment. Can you elaborate on this? There was a problem hiding this comment. The root cause of the problem described above
Because we don't use proper Object Oriented design to choose correct factory to be used and instantiate needed parser. We just look over all possible factories and try to instantiate parser with the help of each next factory in the list. So, our Exception is not actually exception (because Exception represents unexpected behavior, but here we have expected false results) that's why we have a juggling of Exception -> FALSE There was a problem hiding this comment. It is a valid case for actual CSV file parser factory or for ZIP file parser factory as if they are used directly by a client, returning false is a terrible thing. Adding if ($status) is worse practice as it increases cyclomatic complexity of client code and hidden bugs for unchecked conditions, exceptions look better in this case in my opinion. Also, a CompositeFactory does throw the same exception if no other parser factories gave a valid parser instance, so unexpected path gets notified up the chain. We cannot use here common for Magento 2 "file extension based" strategy pattern, as file path might be a stream or even a temporary file without the extension, so offloading logic to parser factory looks more clean and extensible. As well factory always throws UnsupportedPathException if the file cannot be read by any of the parsers. If you have a better idea how to solve this issue, I would be happy to change the code to that structure. There was a problem hiding this comment. Idea is to have a decision point. The decision point in your case is your composite factory. In this case, Exception would be a real exceptional state, not like it's now when Exception is Expected behavior.
Yes, that's why I am advising you to make your code more Object Oriented friendly to get rid of such code $parser = $this->createParserIfSupported($parserFactory, $path, $options);
if ($parser === false) {
continue;
}Problems you have with Exceptions and False handling is just a side-effect of wrong OO design with choosing right Factory to handle the precise business case There was a problem hiding this comment. If you'd like I can move it to RegExp based detection on a path via factory selection strategy. But I wouldn't change actual CSV and ZIP factories for path validation, as they must throw an exception if the file is not in the expected format. |
||
| } | ||
|
|
||
| return $parser; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| <?php | ||
| /** | ||
| * Copyright © Magento, Inc. All rights reserved. | ||
| * See COPYING.txt for license details. | ||
| */ | ||
|
|
||
| namespace Magento\ImportExport\Model\Import\Source\FileParser; | ||
|
|
||
| class CorruptedFileException extends \RuntimeException | ||
| { | ||
| private $fileName; | ||
|
|
||
| public function __construct($fileName = '', $message = '') | ||
| { | ||
| $this->fileName = $fileName; | ||
| parent::__construct($message ?: sprintf('File "%s" is corrupted', $fileName)); | ||
|
There was a problem hiding this comment. something wrong with this code, because it has two default parameters |
||
| } | ||
|
|
||
|
|
||
| public function getFileName() | ||
| { | ||
| return $this->fileName; | ||
| } | ||
|
There was a problem hiding this comment. why do we need public getter? There was a problem hiding this comment. The file name can be of a help for client code that catches this specific exception. I can remove it if filename is redundant error information for parsing files. |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't recommend to use Inheritance Based API, so all the methods should be whether public or private