Skip to content
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

makes $layerResolver protected in Category view #8641

Closed
wants to merge 1 commit into from
Closed

makes $layerResolver protected in Category view #8641

wants to merge 1 commit into from

Conversation

BlackIkeEagle
Copy link
Member

When you need to overwrite this View controller but want to inherit
since most of the functionality is exactly the same you have access to
everything but the $layerResolver which is kind of annoying. Its also
pointless to need to copy the constructor to your child class just
because of this. So make $layerResolver protected.

Signed-off-by: BlackEagle ike.devolder@gmail.com

When you *need* to overwrite this View controller but want to inherit
since most of the functionality is exactly the same you have access to
everything but the $layerResolver which is kind of annoying. Its also
pointless to need to copy the constructor to your child class just
because of this. So make $layerResolver protected.

Signed-off-by: BlackEagle <ike.devolder@gmail.com>
@orlangur
Copy link
Contributor

you have access to everything but the $layerResolver which is kind of annoying

Protected properties are just a bad smelling legacy.

It's also pointless to need to copy the constructor to your child class just because of this

It is the lesser evil. Takes some 0.5s in modern IDEs.

See #8420 (comment) for details.

@maghamed
Copy link
Contributor

@BlackIkeEagle Magento discourages usage of Inheritance as a mean for extension/customization of entities.
So, ideally there shouldn't be inheritance at all among the Magento entities. And all existing examples of inheritable you can see in our codebase could be considered as Technical Debt.
Thus, if there is no Inheritance -> you no need the protected property.

What you can do in this specific case:

if you need to overwrite/extend layer object being instantiated
$this->layerResolver->create(Resolver::CATALOG_LAYER_CATEGORY);
you could provide own version of Magento\Catalog\Model\Layer\Resolver object trough DI into \Magento\Catalog\Controller\Category\View

current implementation of \Magento\Catalog\Controller\Category\View couldn't be considered as a good example of Controller's code, because there is some logic which doesn't belong to controller (initCategory).

But nevertheless, I believe that you could just manipulate with external dependencies which are injected using DI, and substitute them if needed on own versions.
If you need several entities (read versions) of \Magento\Catalog\Controller\Category\View but configured differently - you could achieve this creating VirtualType(-s) based on \Magento\Catalog\Controller\Category\View.

@maghamed maghamed closed this Feb 21, 2017
@BlackIkeEagle
Copy link
Member Author

The change was made so we don't have to copy all code from the current Category View controller to our overriding controller. But we could do it with DI and have some duplication.

magento-devops-reposync-svc pushed a commit that referenced this pull request Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants