Skip to content

Magento_Catalog: Image attribute should have different parent class #8587

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

Closed
koenner01 opened this issue Feb 17, 2017 · 4 comments
Closed

Magento_Catalog: Image attribute should have different parent class #8587

koenner01 opened this issue Feb 17, 2017 · 4 comments
Labels
Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development

Comments

@koenner01
Copy link
Contributor

koenner01 commented Feb 17, 2017

Currently the 'Magento\Catalog\Model\Product\Attribute\Frontend\Image' model is inheriting from the abstract class 'Magento\Eav\Model\Entity\Attribute\Frontend\AbstractFrontend'.

The class should inherit from 'Magento\Eav\Model\Entity\Attribute\Frontend\DefaultFrontend' instead (which inherits from the abstract). This would be more consistent with other frontend attributes (like datetime) and more flexible in extending the functionalities of the frontend attributes.

Preconditions

  1. PHP7.0
  2. MG2.1.2, MG2.1.3, MG2.1.4

Steps to reproduce

  1. Look at file Magento\Catalog\Model\Product\Attribute\Frontend\Image

Expected result

  1. class should inherit from Magento\Eav\Model\Entity\Attribute\Frontend\DefaultFrontend

Actual result

  1. class inherits from Magento\Eav\Model\Entity\Attribute\Frontend\AbstractFrontend

This fix would allow more flexibility when adding functionalities on the frontend attributes. You would not have to provide an extra preference to fix the 'incorrect' parent class for the image attributes.

An example of where this is already done in the core:
Magento\Eav\Model\Entity\Attribute\Frontend\Datetime

@maghamed
Copy link
Contributor

maghamed commented Mar 1, 2017

@koenner01 Why do you need Image to be successor of DefaultFrontend?

An example of where this is already done in the core:
Magento\Eav\Model\Entity\Attribute\Frontend\Datetime

We don't have these neither in 2.0.*
https://github.com/magento/magento2/blob/2.0.13/app/code/Magento/Eav/Model/Entity/Attribute/Frontend/Datetime.php#L11
nor in 2.1.*
https://github.com/magento/magento2/blob/2.1.4/app/code/Magento/Eav/Model/Entity/Attribute/Frontend/Datetime.php#L11
nor in develop branch
https://github.com/magento/magento2/blob/develop/app/code/Magento/Eav/Model/Entity/Attribute/Frontend/Datetime.php#L11

Actually for now Magento\Eav\Model\Entity\Attribute\Frontend\DefaultFrontend class doesn't have any successor.

Currently the only place where Default Frontend class is used is in the AbstractAttribute class

    /**
     * Retrieve frontend instance
     *
     * @return \Magento\Eav\Model\Entity\Attribute\Frontend\AbstractFrontend
     */
    public function getFrontend()
    {
        if (empty($this->_frontend)) {
            if (!$this->getFrontendModel()) {
                $this->setFrontendModel($this->_getDefaultFrontendModel());
            }
            $this->_frontend = $this->_universalFactory->create($this->getFrontendModel())->setAttribute($this);
        }

        return $this->_frontend;
    }

and this behavior looks correct for me.
What customization abilities you are losing, having such hierarchy of inheritance ?

Magento discourages usage of inheritance. So, a deep inheritance tree is a "smell" from architectural point of view.

@koenner01
Copy link
Contributor Author

Yes, weird. I will check which version I was working on where the DateTime had a different parent class.

The logic behind this was to expose the publics methods so they can have plugins injected, because you can't plugin an abstract class.

We (and probably other people) are inserting custom logic into all EAV attributes and the most logic place to do this was in the DefaultFrontend. Should we set a preference instead on the abstract class ?

@magento-team magento-team added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development develop labels Jul 31, 2017
@magento-team
Copy link
Contributor

Internal ticket to track issue progress: MAGETWO-64844

@magento-engcom-team magento-engcom-team added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development develop Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed and removed G1 Passed labels Sep 5, 2017
@magento-engcom-team
Copy link
Contributor

Hi @koenner01
We are closing the issue. Related PR has been rejected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development
Projects
None yet
Development

No branches or pull requests

4 participants