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

Specify Magento_Catalog module on template for sorting #1385

Closed
wants to merge 1 commit into from

Conversation

vernard
Copy link
Contributor

@vernard vernard commented Jun 18, 2015

I am trying to create an extension that enhances the sort order on product lists.

Problem

To extend the sorting feature, I had extend the block Magento\Catalog\Block\Product\ProductList\Toolbar. Without specifying that the template is on Magento_Catalog, the module that extends this block also needs a copy of the template.

What I tried

First, I tried using a plugin for this block and adding an afterSetCollection() method.

setCollection() method handles the sorting algorithm

Unfortunately, my plugin did not work. This block had no interceptor (I am still interested in this and open to answers why).

So, I did a rewrite using <preference> tag, then I made my changes in the block class from my module. After this, the toolbar was gone from the frontend! That is because it is searching for the template file (in my module) and not found. I worked it around by copying the product/list/toolbar.phtml file on my module.

@vpelipenko vpelipenko added the MX label Jun 18, 2015
@orlangur
Copy link
Contributor

Can't you just use template="Magento_Catalog::product/list/toolbar.phtml" template declaration inside your module? For Catalog itself such prefix looks redundant.

@vernard
Copy link
Contributor Author

vernard commented Jun 19, 2015

Such "redundant" declarations is used on mostly all the blocks in Catalog layout update and this one in particular looks like a black sheep to me. One of the reasons I wanted this was for uniformity.

@orlangur
Copy link
Contributor

The fact such prefix is present in some declarations does not mean it's the right thing to do. For uniformity it could be simply stripped out for all places where it does not change system behavior.

Do the template="Magento_Catalog::product/list/toolbar.phtml" declaration in your module solves the initial problem?

@Vinai
Copy link
Contributor

Vinai commented Jun 22, 2015

During exercises in the recent Magento 2 developer training some developers had a very similar experience. It was confusing that changing a block via preferences caused the rendering to break if the prefix on the template wasn't specified. I think it would help the allover expressiveness of the code and ease of development if the prefix where always specified.

@vernard
Copy link
Contributor Author

vernard commented Jun 22, 2015

I'm trying to do the template="Magento_Catalog::product/list/toolbar.phtml" from my own module but I can't seem to make it work. Any ideas?

@Vinai
Copy link
Contributor

Vinai commented Jun 22, 2015

Not knowing your code, the Magento_Catalog prefix will cause the view to look for the template within the Magento_Catalog subdirectory or the Magento\Catalog module.
If you want the view to locate the template within your own module, you need to use your module name as the prefix, or, in case of the rewrite, omit it so the block class name can be used to determine the prefix automatically.

@vernard
Copy link
Contributor Author

vernard commented Jun 22, 2015

@Vinai As I wrote on my initial post:

I worked it around by copying the product/list/toolbar.phtml file on my module.

As @orlangur suggested, I'm trying to do the template="Magento_Catalog::product/list/toolbar.phtml" from my own module.
I tried creating a layout XML for catalog_category_view and remove product_list_toolbar (and replace it afterwards), but it doesn't get removed. Any ideas?

@Vinai
Copy link
Contributor

Vinai commented Jun 22, 2015

Sure, I understand. I'm just chiming in here to express my opinion that based on my experience during the Magento 2 dev training, it would be a good idea to always add the module name prefix to templates declarations in layout XML. It would make the view layer work more like what developers expect, that is, less surprising in case of a rewrite preference change.

@vernard
Copy link
Contributor Author

vernard commented Jun 22, 2015

I'm totally with you in this idea. Thanks for the support. Any idea on my problem?

I tried creating a layout XML for catalog_category_view and remove product_list_toolbar (and replace it afterwards), but it doesn't get removed.

@Vinai
Copy link
Contributor

Vinai commented Jun 22, 2015

No idea without seeing your code. Can't promise I'll have time to test locally, even if you add it here though. But I might :)

@orlangur
Copy link
Contributor

I worked it around by copying the product/list/toolbar.phtml file on my module.

@vernard, this is surely an approach I would like to avoid, but if adding Magento_Catalog prefix in your module works I would not change a declaration in Catalog module itself.

@orlangur
Copy link
Contributor

I tried creating a layout XML for catalog_category_view and remove product_list_toolbar (and replace it afterwards), but it doesn't get removed. Any ideas?

Ok, now I see, need to change both block and template argument reliably in initial declaration.

@vernard
Copy link
Contributor Author

vernard commented Jun 22, 2015

@orlangur By "initial declaration", do you mean block declaration in Magento_Catalog module?

@orlangur
Copy link
Contributor

@vernard, yep.

@kokoc
Copy link
Member

kokoc commented Jul 7, 2015

@vernard thank you for the contribution! We are going to accept this PR. However, I suggest you to replace preference with <remove name="product_list_toolbar" /> + <referenceBlock name="category.products.list"><block name="product_list_toolbar" class="Your/Custom/Toolbar"> layout instructions.

Internal reference: MAGETWO-39816

@vrann
Copy link
Contributor

vrann commented Jul 14, 2015

@vernard Can you please merge it with the latest code?

If the block `Magento\Catalog\Block\Product\ProductList\Toolbar` is rewritten in another module, it is searching for the template in that module, not in `Magento_Catalog`.
@vernard
Copy link
Contributor Author

vernard commented Jul 15, 2015

@vrann Done.

@vpelipenko
Copy link
Contributor

@vernard, your contribution is added to 1.0.0-beta. This PR was not closed automatically by GitHub due to unknown reason, but your commit is available in history. Can we close this PR?

@vernard
Copy link
Contributor Author

vernard commented Jul 16, 2015

Sure. Thanks for your help

@vernard vernard closed this Jul 16, 2015
magento-team pushed a commit that referenced this pull request Aug 4, 2017
[firedrakes] MAGETWO-71231: Remove AR code from 2.2.0
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.

6 participants