search

magento / community-features

Magento Features Development is an Initiative to Allows Community Memebers Join to Development of Magento Features
46 stars 18 forks source link

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

Closed orlangur closed 5 years ago

orlangur commented 6 years ago

Issues: https://github.com/magento/magento2/issues/10928 https://github.com/magento/magento2/issues/11910

Poor attempts to fix: https://github.com/magento/magento2/pull/8485 https://github.com/magento/magento2/pull/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 commented 6 years ago

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

dvynograd commented 6 years ago

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

orlangur commented 6 years ago

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

apasare commented 5 years ago

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 commented 5 years ago

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

apasare commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

Yay, @Den4ik did it 💪