Skip to content

Review observer does load() on collection, which causes issues for other observers. #11910

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
Floddy opened this issue Oct 31, 2017 · 5 comments
Labels
duplicate Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed non-issue

Comments

@Floddy
Copy link

Floddy commented Oct 31, 2017

Preconditions

  1. Install Magento 2.2 via composer

Steps to reproduce

  1. Create a module that hooks into the event catalog_block_product_list_collection
  2. The module should have the sequence so it's ran after Magento_Review
  3. Use addAttributeToSelect to add a product attribute to the collection in the observer

Alternatively:

  1. Use the addAttribute method inside the Magento\Catalog\Block\Product\ListProduct block to add a product attribute to the collection

Expected result

  1. The product attribute is included in the collection

Actual result

  1. The product attribute is not included in the collection

Cause and solution

The cause of this is because:
Magento\Review\Observer\CatalogBlockProductCollectionBeforeToHtmlObserver.php uses the catalog_block_product_list_collection event, and does a $collection->load in the observer, which makes it not possible to use addAttributeToSelect afterwards.

Magento\Review should IMO instead use a plugin, that runs 'after' Magento\Catalog\Block\Product\ListProduct, which does the same thing as the observer - because what the functionality really needs to do, is to add the review data to the loaded collection:

<type name="Magento\Catalog\Block\Product\ListProduct">
    <plugin name="reviewCollectionInitializer" type="Magento\Review\Block\Product\ListProduct\Plugin\Review" />
</type>
<?php

namespace Magento\Review\Block\Product\ListProduct\Plugin;

class Review
{
    protected $_reviewFactory;
    
    public function __construct(
        \Magento\Review\Model\ReviewFactory $reviewFactory
    ) { 
        $this->_reviewFactory = $reviewFactory;
    }   
    
    public function afterGetLoadedProductCollection($subject, $result)
    {
        $productCollection = $result;
        if ($productCollection instanceof \Magento\Framework\Data\Collection) {
            $this->_reviewFactory->create()->appendSummary($productCollection);
        }   
        return $productCollection;
    }   
}   

There seems to be a similar plugin for the product compare list block, so I'm not sure why this isn't implemented in a similar way?

@magento-engcom-team magento-engcom-team added the Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed label Oct 31, 2017
@orlangur
Copy link
Contributor

orlangur commented Nov 1, 2017

Hi @Floddy, this issue is a duplicate of #10928. As I mentioned there, you should not use this event in the first place

Use addAttributeToSelect to add a product attribute to the collection in the observer

some before_load of collection would be much more suitable.

I do agree that Review observer is written poorly and pull request improving it would be much appreciated. However, there is no bug in current implementation.

afterGetLoadedProductCollection is not a good idea for injection as it will be executed each time we call getLoadedProductCollection.

@Floddy
Copy link
Author

Floddy commented Nov 2, 2017

I'll probably just use another event, not really sure what should be done if it should just be ran on this block explicitly, but that's another story.

Regarding bug or not, it renders Magento\Catalog\Block\Product\ListProduct::addAttribute() none-useable as far as I can see?

addAttribute($code) calls _getProductCollection()->addAttributeToSelect($code)
_getProductCollection() calls initializeProductCollection()
initializeProductCollection() dispatches the event in which the collection is being loaded by the review module - which means when ->addAttributeToSelect($code) is called, it's too late.

Or is addAttribute to be deprecated?

Agreed on afterGetLoadedProductCollection, I was too quick on that one. Can't find another public method to use a plugin for though.

@Sarvesh-A
Copy link
Contributor

Sarvesh-A commented Jun 10, 2018

Hello @Floddy,

I was working on event catalog_block_product_list_collection as you mentioned Magento_Review module has used same event and used load() for collection.

Which disables to filter anything in collection for other modules which using catalog_block_product_list_collection event.

I was frustrated for almost a day why its not getting filter!!!

Then for a check I disabled Magento_Review module. And guess what!!! It worked like charm! But its not the solution cause I use Reviews as well.

So, The thing is if Magento has dispatched event catalog_block_product_list_collection for third party modules to extend collection then why they have used load() and it disables all filters! This looks like Magento's default bug which needs be resolved!

@orlangur , Please take care of this if it's possible cause these small things makes developers more sick and wasting more time!! I am not commanding or anything ( cause I love Magento! ;) ) just frustrated a bit cause it was not developer's bug! :)

@orlangur
Copy link
Contributor

@Sarvesh-A reported it into https://github.com/magento/community-features/. Feel free to fix it the right way :)

@Sarvesh-A
Copy link
Contributor

@orlangur Thanks to reopen it! Appreciate that. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed non-issue
Projects
None yet
Development

No branches or pull requests

4 participants