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

Zend feed refactoring #9347

Merged
merged 28 commits into from May 12, 2018

Conversation

@ldusan84
Collaborator

ldusan84 commented Apr 21, 2017

Refactoring in order to move hardcoded Zend_Feed dependency into interface for further refactoring. Ticket #9240

Description

I have created an interface that replaces Zend_Feed::importArray static call. The concrete class takes Zend_Feed object and returns it's own result that's a wrapper around Zend_Feed_Abstract.

I am not sure what the policies are on this type of things so I just went with the bare minimum of what is required.

Would appreciate hearing your thoughts on this one.

ldusan84 added some commits Apr 20, 2017

@dmanners

This comment has been minimized.

Contributor

dmanners commented Apr 24, 2017

@ldusan84 I think this is a great start! I would love to see it migrated over to https://github.com/zendframework/zend-feed but I understand with going with the bare minimum to start with.

@maghamed maghamed self-assigned this Apr 24, 2017

@maghamed maghamed self-requested a review Apr 24, 2017

@ldusan84

This comment has been minimized.

Collaborator

ldusan84 commented Apr 24, 2017

@dmanners thanks, yes, that would be the next step and I would like to do that after this one.

return $this->feedFactory->create(['feed' => $feed]);
} catch (\Zend_Feed_Exception $e) {
throw new \Magento\Framework\Exception\FeedImporterException(
new \Magento\Framework\Phrase($e->getMessage()),

This comment has been minimized.

@adragus-inviqa

adragus-inviqa Apr 30, 2017

Contributor

Should just use __($e->getMessage). It's the same thing, but shorter.

This comment has been minimized.

@ldusan84

ldusan84 Apr 30, 2017

Collaborator

Nice one, makes sense, thanks 👍

@ldusan84

This comment has been minimized.

Collaborator

ldusan84 commented May 15, 2017

There is an error on Travis CI - "The command "phpenv global 7.1" failed and exited with 1 during .", anything I can do about it?

@ishakhsuvarov

This comment has been minimized.

Contributor

ishakhsuvarov commented May 20, 2017

@ldusan84 Looks like it was some temporary error with the Travis CI. I've restarted the job in question and its green now.

* @param SerializerInterface|null $serializer
*/
public function __construct(
\Magento\Framework\App\CacheInterface $cache,
\Magento\Framework\App\FeedImporterInterface $feedImporter,

This comment has been minimized.

@maghamed

maghamed May 21, 2017

Contributor

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

This comment has been minimized.

@ldusan84

ldusan84 May 24, 2017

Collaborator

Makes sense, it's fixed now.

@@ -34,6 +34,27 @@ class RssTest extends \PHPUnit_Framework_TestCase
];
/**
* @var string
*/
protected $feedXml = '<?xml version="1.0" encoding="UTF-8"?>

This comment has been minimized.

@maghamed

maghamed May 21, 2017

Contributor

better to make variable as private

This comment has been minimized.

@ldusan84

ldusan84 May 24, 2017

Collaborator

Agreed, fixed.

return $this->feedFactory->create(['feed' => $feed]);
} catch (\Zend_Feed_Exception $e) {
throw new \Magento\Framework\Exception\FeedImporterException(
__($e->getMessage()),

This comment has been minimized.

@maghamed

maghamed May 21, 2017

Contributor

it's not correct to translate low level exception providing its output to translation function.
Because there is no guarantee that the phrase would be translated .

All you need to do - is to log initial exception.
And provide new description which says about the problem in general words

This comment has been minimized.

@ldusan84

ldusan84 May 24, 2017

Collaborator

This is done, can you please check?

* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
namespace Magento\Framework\Exception;

This comment has been minimized.

@maghamed

maghamed May 21, 2017

Contributor

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

This comment has been minimized.

@ldusan84

ldusan84 May 24, 2017

Collaborator

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.

* @param string $format
* @return FeedInterface
*/
public function importArray(array $data, $format = 'atom');

This comment has been minimized.

@maghamed

maghamed May 21, 2017

Contributor

it's not good from interface point of view, that 'atom' passed as a default parameter.

What other formats does this Importer support?
It's not clear from the interface.
And not clear how to check this out.
Interface should be clear enough to get this knowledge

This comment has been minimized.

@ldusan84

ldusan84 May 24, 2017

Collaborator

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?

This comment has been minimized.

@maghamed

maghamed Jul 6, 2017

Contributor

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.
Above I described my idea what we can do with this

interface FeedImporterInterface
{
/**

This comment has been minimized.

@maghamed

maghamed May 21, 2017

Contributor

please provide a precise description what this method is actually do

This comment has been minimized.

@ldusan84

ldusan84 May 24, 2017

Collaborator

Done.

*/
namespace Magento\Framework\App;
interface FeedImporterInterface

This comment has been minimized.

@maghamed

maghamed May 21, 2017

Contributor

maybe make sense to rename this interface to
FeedFactoryInterface ?

This comment has been minimized.

@ldusan84

ldusan84 May 24, 2017

Collaborator

I am not sure about this, why do you think that it should be renamed to factory, it doesn't create new objects?

This comment has been minimized.

@maghamed

maghamed Jul 6, 2017

Contributor

Looks like it creates. Description from the importArray method says:
Get a new \Magento\Framework\App\Feed object from a custom array
and implementation looks like:

$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?
Looking at the interface and implementation I see FeedFactory which accepts array of data and format

For me it's not clear why we need to keep both objects
\Magento\Framework\App\Feed\Importer
and
FeedFactory

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)
}
@maghamed

This comment has been minimized.

Contributor

maghamed commented May 21, 2017

hey @ldusan84 , sorry for late response.
Just provided feedback for your PR. Hope you are still interested to deliver these changes

@ldusan84

This comment has been minimized.

Collaborator

ldusan84 commented May 21, 2017

Hi @maghamed no problem, thanks for the feedback. I'll look into the stuff you mentioned.

@ldusan84

This comment has been minimized.

Collaborator

ldusan84 commented May 24, 2017

Hi @maghamed I made some changes and posted comments to your feedback. Let me know what you think please.

@ldusan84

This comment has been minimized.

Collaborator

ldusan84 commented Jun 29, 2017

@ishakhsuvarov @maghamed @dmanners any news on this one guys?

@benmarks

This comment has been minimized.

Contributor

benmarks commented Jul 5, 2017

@maksek can your team check?

@ishakhsuvarov

This comment has been minimized.

Contributor

ishakhsuvarov commented Jul 6, 2017

@ldusan84 @benmarks We are looking into this PR. Review takes somewhat more time then usual, since it introduces updates to the API, which must be in ideal desired-state if updated. We attempt to design APIs which would not require and additional changes in foreseeable future.

Thank you for your patience.

*/
namespace Magento\Framework\App;
interface FeedImporterInterface

This comment has been minimized.

@maghamed

maghamed Jul 6, 2017

Contributor

Looks like it creates. Description from the importArray method says:
Get a new \Magento\Framework\App\Feed object from a custom array
and implementation looks like:

$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?
Looking at the interface and implementation I see FeedFactory which accepts array of data and format

For me it's not clear why we need to keep both objects
\Magento\Framework\App\Feed\Importer
and
FeedFactory

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)
}
/**
* @return string
*/
public function asXml();

This comment has been minimized.

@maghamed

maghamed Jul 6, 2017

Contributor

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.
Because we no need to have coupling between Data and different ways how these data could be presented.
Thus, all that we need to have is kind of "output" method which accepts a format in which data should be presented. And we need to have a dedicated interface/entity responsible for format registration.
I mentioned SupportedFeedFormatsInterface here

* @param string $format
* @return FeedInterface
*/
public function importArray(array $data, $format = 'atom');

This comment has been minimized.

@maghamed

maghamed Jul 6, 2017

Contributor

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.
Above I described my idea what we can do with this

@maghamed

This comment has been minimized.

Contributor

maghamed commented Jul 14, 2017

hey @ldusan84 do you have some time to make requested changes for current PR?

@ldusan84

This comment has been minimized.

Collaborator

ldusan84 commented Jul 14, 2017

Hi @maghamed yes sure, I would like to, sorry I read your comments but didn't have time to respond these days.

@maghamed

This comment has been minimized.

Contributor

maghamed commented Jul 14, 2017

Cool, thanks! Take your time

@okorshenko okorshenko added this to the July 2017 milestone Jul 20, 2017

@okorshenko okorshenko added the develop label Jul 20, 2017

@okorshenko okorshenko self-assigned this Jul 25, 2017

@ihor-sviziev ihor-sviziev requested a review from ishakhsuvarov Jan 18, 2018

@okorshenko okorshenko self-assigned this Jan 26, 2018

@magento-engcom-team

This comment has been minimized.

Contributor

magento-engcom-team commented Jan 26, 2018

@ldusan84 thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@ihor-sviziev

This comment has been minimized.

Contributor

ihor-sviziev commented Mar 23, 2018

Hi,

Currently ZF1 zend feed usage was replaced with ZF 2 Zend\Feed in magento-engcom/php-7.2-support#89 and we have conflicts in this PR.
@ishakhsuvarov @maghamed @okorshenko will it be handled by magento team, or adjusting this PR is required?

@okorshenko okorshenko modified the milestones: March 2018, April 2018 Apr 16, 2018

@sidolov sidolov self-assigned this Apr 28, 2018

@magento-engcom-team magento-engcom-team moved this from Review In Progress to Testing In Progress in Community Pull Requests Apr 30, 2018

@okorshenko

This comment has been minimized.

Contributor

okorshenko commented Apr 30, 2018

Hi @ldusan84
Thank you for your patience. We fixed some broken code in your branch and sent the fix to QA department. We are waiting for QA report now. If everything is ok, this Pull Request should be merged soon. @sidolov is working on this PR now

Please do not change or modify the code in your branch till merge.

@magento-engcom-team magento-engcom-team moved this from Testing In Progress to Merging In Progress in Community Pull Requests May 10, 2018

@magento-engcom-team magento-engcom-team merged commit 4462eaf into magento:2.3-develop May 12, 2018

2 of 3 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

Community Pull Requests automation moved this from Merging In Progress to Done May 12, 2018

magento-engcom-team pushed a commit that referenced this pull request May 12, 2018

@magento-engcom-team

This comment has been minimized.

Contributor

magento-engcom-team commented May 12, 2018

Hi @ldusan84. Thank you for your contribution.
Changes from your Pull Request will be available with the upcoming 2.3.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment