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) | ||
|
There was a problem hiding this comment. Please provide supported formats argument, configurable through DI. |
||
| { | ||
| $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,56 @@ | ||
| <?php | ||
| /** | ||
| * Copyright © Magento, Inc. All rights reserved. | ||
| * See COPYING.txt for license details. | ||
| */ | ||
| namespace Magento\Framework\App\Feed; | ||
|
|
||
| /** | ||
| * Feed importer | ||
| */ | ||
| class Importer implements \Magento\Framework\App\FeedImporterInterface | ||
| { | ||
| /** | ||
| * @var \Zend_Feed | ||
| */ | ||
| private $feedProcessor; | ||
|
|
||
| /** | ||
| * @var \Magento\Framework\App\FeedFactory | ||
| */ | ||
| private $feedFactory; | ||
|
|
||
| /** | ||
| * @param \Zend_Feed $feedProcessor | ||
| * @param \Magento\Framework\App\FeedFactory $feedFactory | ||
| */ | ||
| public function __construct( | ||
| \Zend_Feed $feedProcessor, | ||
| \Magento\Framework\App\FeedFactory $feedFactory | ||
| ) { | ||
| $this->feedProcessor = $feedProcessor; | ||
| $this->feedFactory = $feedFactory; | ||
| } | ||
|
|
||
| /** | ||
| * Get a new \Magento\Framework\App\Feed object from a custom array | ||
| * | ||
| * @throws \Magento\Framework\Exception\FeedImporterException | ||
| * @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) { | ||
| throw new \Magento\Framework\Exception\FeedImporterException( | ||
| new \Magento\Framework\Phrase($e->getMessage()), | ||
|
There was a problem hiding this comment. Should just use There was a problem hiding this comment. Nice one, makes sense, thanks 👍 |
||
| $e | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| <?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 |
||
| * @throws \Magento\Framework\Exception\FeedImporterException | ||
| * @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. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| <?php | ||
| /** | ||
| * Feed importer exception | ||
| * | ||
| * Copyright © Magento, Inc. All rights reserved. | ||
| * See COPYING.txt for license details. | ||
| */ | ||
| namespace Magento\Framework\Exception; | ||
|
There was a problem hiding this comment. don't think that we need to introduce new exception type. It's better to re-use some of the existing ones. Or at least to introduce new exception for ImportException There was a problem hiding this comment. If you look at the Zend_Feed module Zend_Feed_Exception can be thrown for a lot of things. The only common exception for all those cases would be RuntimeException, so I picked that one. Let me know what you think. |
||
|
|
||
| class FeedImporterException extends LocalizedException | ||
| { | ||
|
|
||
| } | ||
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.
That's backward incompatible, new parameter should be added last into the list, and that parameter should be optional. You can read more about it here - http://devdocs.magento.com/guides/v2.1/ext-best-practices/extension-coding/backwards-compatible-development/index.html "Adding a constructor parameter" section
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, it's fixed now.