Zend feed refactoring #9347
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| <?php | ||
| /** | ||
| * Copyright © Magento, Inc. All rights reserved. | ||
| * See COPYING.txt for license details. | ||
| */ | ||
| namespace Magento\Framework\App; | ||
|
|
||
| class Feed implements \Magento\Framework\App\FeedInterface | ||
| { | ||
| /** | ||
| * @var \Magento\Framework\App\FeedImporterInterface | ||
| */ | ||
| private $feed; | ||
|
|
||
| /** | ||
| * @param \Zend_Feed_Abstract $feed | ||
| */ | ||
| public function __construct(\Zend_Feed_Abstract $feed) | ||
| { | ||
| $this->feed = $feed; | ||
| } | ||
|
|
||
| /** | ||
| * Get the xml from Zend_Feed_Abstract object | ||
| * | ||
| * @return string | ||
| */ | ||
| public function asXml() | ||
| { | ||
| return $this->feed->saveXml(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| <?php | ||
| /** | ||
| * Copyright © Magento, Inc. All rights reserved. | ||
| * See COPYING.txt for license details. | ||
| */ | ||
| namespace Magento\Framework\App\Feed; | ||
|
|
||
| use Magento\Framework\App\FeedFactory; | ||
| use Psr\Log\LoggerInterface; | ||
|
|
||
| /** | ||
| * Feed importer | ||
| */ | ||
| class Importer implements \Magento\Framework\App\FeedImporterInterface | ||
| { | ||
| /** | ||
| * @var \Zend_Feed | ||
| */ | ||
| private $feedProcessor; | ||
|
|
||
| /** | ||
| * @var FeedFactory | ||
| */ | ||
| private $feedFactory; | ||
|
|
||
| /** | ||
| * @var LoggerInterface | ||
| */ | ||
| private $logger; | ||
|
|
||
| /** | ||
| * @param \Zend_Feed $feedProcessor | ||
| * @param FeedFactory $feedFactory | ||
| * @param LoggerInterface $logger | ||
| */ | ||
| public function __construct( | ||
| \Zend_Feed $feedProcessor, | ||
| FeedFactory $feedFactory, | ||
| LoggerInterface $logger | ||
| ) { | ||
| $this->feedProcessor = $feedProcessor; | ||
| $this->feedFactory = $feedFactory; | ||
| $this->logger = $logger; | ||
| } | ||
|
|
||
| /** | ||
| * Get a new \Magento\Framework\App\Feed object from a custom array | ||
| * | ||
| * @throws \Magento\Framework\Exception\RuntimeException | ||
| * @param array $data | ||
| * @param string $format | ||
| * @return \Magento\Framework\App\FeedInterface | ||
| */ | ||
| public function importArray(array $data, $format = 'atom') | ||
| { | ||
|
|
||
| try { | ||
| $feed = $this->feedProcessor->importArray($data, $format); | ||
| return $this->feedFactory->create(['feed' => $feed]); | ||
| } catch (\Zend_Feed_Exception $e) { | ||
| $this->logger->error($e->getMessage()); | ||
| throw new \Magento\Framework\Exception\RuntimeException( | ||
| __('There has been an error with import'), | ||
| $e | ||
| ); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| <?php | ||
| /** | ||
| * Copyright © Magento, Inc. All rights reserved. | ||
| * See COPYING.txt for license details. | ||
| */ | ||
| namespace Magento\Framework\App; | ||
|
|
||
| interface FeedImporterInterface | ||
|
There was a problem hiding this comment. maybe make sense to rename this interface to There was a problem hiding this comment. I am not sure about this, why do you think that it should be renamed to factory, it doesn't create new objects? There was a problem hiding this comment. Looks like it creates. Description from the importArray method says: $feed = $this->feedProcessor->importArray($data, $format);
return $this->feedFactory->create(['feed' => $feed]);for now it's not clear why it's called importer? From where it imports data? For me it's not clear why we need to keep both objects Because both of them responsible for \Magento\Framework\App\FeedInterface creation. I propose to get rid of \Magento\Framework\App\Feed\Importer and move it's logic to FeedFactory. Thus, FeedFactoryInterface would look like: class FeedFactoryInterface
{
public function importArray(array $data, $format = SupportedFeedFormatsInterface::DEFAULT_FORMAT)
} |
||
| { | ||
|
|
||
| /** | ||
|
There was a problem hiding this comment. please provide a precise description what this method is actually do |
||
| * Returns FeedInterface object from a custom array | ||
| * | ||
| * @throws \Magento\Framework\Exception\RuntimeException | ||
| * @param array $data | ||
| * @param string $format | ||
| * @return FeedInterface | ||
| */ | ||
| public function importArray(array $data, $format = 'atom'); | ||
|
There was a problem hiding this comment. it's not good from interface point of view, that 'atom' passed as a default parameter. What other formats does this Importer support? There was a problem hiding this comment. I see your point but I did it because I wanted to replicate the original importArray method from Zend_Feed which has those arguments. What would be the best thing to do in this case? There was a problem hiding this comment. It's not always a good Idea to replicate contracts from Zend. Because sometimes we would provide a wider contract than Zend does, and sometimes their contracts are not so ideal. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| <?php | ||
| /** | ||
| * Copyright © Magento, Inc. All rights reserved. | ||
| * See COPYING.txt for license details. | ||
| */ | ||
| namespace Magento\Framework\App; | ||
|
|
||
|
There was a problem hiding this comment. please provide description for the interface |
||
| interface FeedInterface | ||
| { | ||
| /** | ||
|
There was a problem hiding this comment. please provide description of input data in PHP Doc Block |
||
| * @return string | ||
| */ | ||
| public function asXml(); | ||
|
There was a problem hiding this comment. I prefer to see this method as public function getFormatedContentAs($format = SupportedFeedFormatsInterface::DEFAULT_FORMAT);Naming could be changed, but main idea is to make FeedInteface agnostic to the possible representation formats. |
||
| } | ||
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.
Please provide supported formats argument, configurable through DI.
As described below, to have a possibility verify whether supported format passed to getFormatedContentAs method
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.
Makes sense.