DO NOT do load() of product collection in Review observer as it causes issues for other observers

[orlangur]

Issues:
magento/magento2#10928
magento/magento2#11910

Poor attempts to fix:
magento/magento2#8485
magento/magento2#12770

CollectionLoader does not make any sense from domain perspective. Observer needs to be reworked to use other event/be a plugin. Please try to not break BC as others may fix this undesired behavior on their side with some ugly hacks.

[orlangur]

Cc: @dvynograd you may be interested in getting this done.

[dvynograd]

Sounds interesting, I will try to finish it, say next week. Didn't think it still actual.

[orlangur]

@dvynograd Magento Contribution Day is coming next week in Kiev 👍 Not sure if it is distributed this time or not.

[apasare]

this might sound silly, but why are we not introducing a new event like this:

    $this->_eventManager->dispatch(
        'catalog_block_product_list_collection',
        ['collection' => $collection]
    );
    $collection->load();
    $this->_eventManager->dispatch(
        'catalog_block_product_list_collection_loaded',
        ['collection' => $collection]
    );

and move the CatalogBlockProductCollectionBeforeToHtmlObserver observer to catalog_block_product_list_collection_loaded event?

[orlangur]

@godvsdeity how is that different from converting CatalogBlockProductCollectionBeforeToHtmlObserver to a afterLoad plugin for collection?

[apasare]

@orlangur i was thinking that maybe the context matters, and you will want to keep that. catalog_block_product_list_collection gives a more granular context than afterLoad which is fired every time the collection is loaded, right?

[orlangur]

@godvsdeity yes, it fires each time, so we should be careful.

Adding a random event in a random place in code is no better than some https://github.com/magento/magento2/pull/12770/files#diff-6d08440fa49b7b33f7d87b9da7ec863f - introducing entity which does not make sense from business logic perspective.

Why we have many places where collection is loaded? Maybe we should refactor that?

[orlangur]

Yay, @Den4ik did it 💪