[FEATURE] Don't load product collection in review observer #21200

Den4ik · 2019-02-13T17:30:45Z

Description (*)

Module observers executed after CatalogBlockProductCollectionBeforeToHtmlObserver don't have affect on collection. This PR is changing Review observer logic. If fact we can join only 2 reviews_count and rating_summary fields instead of send additional sql query for retrieving this data.

Fixed Issues (if relevant)

Changed append summary to product collection to mysql join

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

Manual testing scenarios (*)

Create new module
Add event observer for catalog_block_product_list_collection
Do some changes to collection in observer

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

magento-cicd2 · 2019-02-13T17:30:48Z

All committers have signed the CLA.

magento-engcom-team · 2019-02-13T17:30:49Z

Hi @Den4ik. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento-engcom-team give me test instance - deploy test instance based on PR changes
@magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

orlangur

Thanks @Den4ik for a new attempt to fix this, please help me with understanding its logic and let's make it actually happen (at last!).

app/code/Magento/Review/Observer/CatalogBlockProductListCollectionInitializeObserver.php

app/code/Magento/Review/Block/Product/ReviewRenderer.php

app/code/Magento/Review/Model/Review.php

app/code/Magento/Review/Observer/CatalogBlockProductListCollectionInitializeObserver.php

Den4ik · 2019-02-14T07:08:10Z

@magento-engcom-team give me test instance

magento-engcom-team · 2019-02-14T07:08:11Z

Hi @Den4ik. Thank you for your request. I'm working on Magento instance for you

magento-engcom-team · 2019-02-14T07:08:49Z

Hi @Den4ik, here is your new Magento instance.
Admin access: https://pr-21200.instances.magento-community.engineering/admin
Login: admin Password: 123123q

ghost · 2019-02-16T10:54:53Z

Hi @Den4ik, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Den4ik · 2019-02-17T18:13:39Z

@magento-engcom-team give me test instance

magento-engcom-team · 2019-02-17T18:13:42Z

Hi @Den4ik. Thank you for your request. I'm working on Magento instance for you

magento-engcom-team · 2019-02-17T18:14:20Z

Hi @Den4ik, here is your new Magento instance.
Admin access: https://pr-21200.instances.magento-community.engineering/admin
Login: admin Password: 123123q

Den4ik · 2019-02-18T14:09:05Z

@sivaschenko @orlangur Any progress on this request?
Do you have any question?

sivaschenko · 2019-02-18T17:44:47Z

Hi @Den4ik thanks for the pull request.

Can you mark Block methods as deprecated instead of removing them to preserve backward compatibility?

Also, our policies do not allow the introduction of new public/protected method for classes marked as API for patch releases (currently 2.3.X). Can you refactor the implementation to avoid adding such methods, or otherwise we'll have to delay delivery of this PR until version 2.4.

Den4ik · 2019-02-18T18:05:45Z

@magento-engcom-team give me test instance

magento-engcom-team · 2019-02-18T18:06:02Z

Hi @Den4ik. Thank you for your request. I'm working on Magento instance for you

magento-engcom-team · 2019-02-18T18:06:52Z

Hi @Den4ik, here is your new Magento instance.
Admin access: https://pr-21200.instances.magento-community.engineering/admin
Login: admin Password: 123123q

Den4ik · 2019-02-19T15:52:42Z

@magento-engcom-team give me test instance

magento-engcom-team · 2019-02-19T15:53:28Z

Hi @Den4ik. Thank you for your request. I'm working on Magento instance for you

magento-engcom-team · 2019-02-19T15:54:24Z

Hi @Den4ik, here is your new Magento instance.
Admin access: https://pr-21200.instances.magento-community.engineering/admin
Login: admin Password: 123123q

Den4ik · 2019-02-19T16:21:24Z

@sivaschenko Thanks for your answers. I have refactored my previous solution implementation

sivaschenko

Hi @Den4ik thanks for the update! Please take a look at my code review notes.

sivaschenko · 2019-02-20T16:22:01Z

app/code/Magento/Review/Block/Product/ReviewRenderer.php

Please add new constructor parameters in a backward compatible way: as a last optional parameter.
See more details in Backward compatible development guide "Adding Constructor parameters" section.

sivaschenko · 2019-02-20T16:26:24Z

app/code/Magento/Review/Test/Unit/Model/ReviewSummaryTest.php

Is that suppress warning necessary here? I don't see too many dependencies in this class.

You were right. Comment was removed

Den4ik · 2019-02-20T18:04:50Z

Hi @sivaschenko Thanks you for fast review. I have update the code according to your recommendations

orlangur · 2019-05-24T17:57:29Z

@p-bystritsky how d52a446 landed here?

…ew-summary-for-produc-list

m2-assistant · 2019-05-31T21:22:42Z

Hi @Den4ik, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

magento#21200

orlangur · 2019-06-01T00:57:23Z

@dvynograd here is something to learn for you 😉

korostii · 2019-07-09T11:34:39Z

lib/internal/Magento/Framework/Data/Collection/AbstractDb.php

        if ($this->_totalRecords === null) {
            $sql = $this->getSelectCountSql();
-            $this->_totalRecords = $this->getConnection()->fetchOne($sql, $this->_bindParams);
+            $this->_totalRecords = $this->_totalRecords ?? $this->getConnection()->fetchOne($sql, $this->_bindParams);


It seems that when if reach this point, $this->_totalRecords must be strictly null at the time null coalescence check. Or am i missing something?

@orlangur @Den4ik

@korostii Null coalescence check won't broke anything in this case, but looks strange :)
@p-bystritsky Do you remember what static tests you were fixed by this code?

magento-engcom-team added Component: Review Release Line: 2.3 labels Feb 13, 2019

orlangur requested changes Feb 13, 2019

View reviewed changes

orlangur self-assigned this Feb 13, 2019

Den4ik closed this Feb 16, 2019

Den4ik reopened this Feb 16, 2019

sivaschenko self-assigned this Feb 18, 2019

sivaschenko added the Progress: needs update label Feb 18, 2019

Den4ik force-pushed the feature-review-summary-for-produc-list branch from ca7b1d9 to 20df8ca Compare February 19, 2019 15:50

sivaschenko requested changes Feb 20, 2019

View reviewed changes

sivaschenko approved these changes Feb 20, 2019

View reviewed changes

p-bystritsky unassigned IvanPletnyov May 23, 2019

magento-engcom-team added the Component: CatalogSearch label May 24, 2019

magento#21200: Fulltext collection size fix.

9e491a6

p-bystritsky force-pushed the feature-review-summary-for-produc-list branch from 6ae319f to 9e491a6 Compare May 27, 2019 09:19

magento-engcom-team added Component: Data and removed Component: CatalogSearch labels May 27, 2019

Merge remote-tracking branch 'mainline/2.3-develop' into feature-revi…

f7ed420

…ew-summary-for-produc-list

p-bystritsky force-pushed the feature-review-summary-for-produc-list branch from df572e2 to 452a55c Compare May 27, 2019 14:39

p-bystritsky force-pushed the feature-review-summary-for-produc-list branch from 452a55c to 66084b2 Compare May 28, 2019 08:37

magento#21200: Static test fix.

f4fce7b

p-bystritsky force-pushed the feature-review-summary-for-produc-list branch from 66084b2 to f4fce7b Compare May 28, 2019 13:33

ghost added Progress: accept and removed Progress: extended testing labels May 29, 2019

p-bystritsky added the Award: complex label May 30, 2019

magento-engcom-team merged commit f4fce7b into magento:2.3-develop May 31, 2019

magento-engcom-team pushed a commit to okorshenko/magento2 that referenced this pull request May 31, 2019

ENGCOM-4346: [FEATURE] Don't load product collection in review observer

ded3799

magento#21200

magento-engcom-team added this to the Release: 2.3.3 milestone May 31, 2019

Den4ik deleted the feature-review-summary-for-produc-list branch May 31, 2019 21:33

Den4ik pushed a commit to Den4ik/magento2 that referenced this pull request Jun 1, 2019

magento#21200: Fulltext collection size fix.

643ea4f

Den4ik pushed a commit to Den4ik/magento2 that referenced this pull request Jun 1, 2019

magento#21200: Static test fix.

a29bbec

Den4ik mentioned this pull request Jun 1, 2019

[Backport] Don't load product collection in review observer #23094

Merged

4 tasks

korostii reviewed Jul 9, 2019

View reviewed changes

[FEATURE] Don't load product collection in review observer #21200

[FEATURE] Don't load product collection in review observer #21200

Uh oh!

Conversation

Den4ik commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

Uh oh!

magento-cicd2 commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magento-engcom-team commented Feb 13, 2019

Uh oh!

orlangur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Den4ik commented Feb 14, 2019

Uh oh!

magento-engcom-team commented Feb 14, 2019

Uh oh!

magento-engcom-team commented Feb 14, 2019

Uh oh!

ghost commented Feb 16, 2019

Uh oh!

Den4ik commented Feb 17, 2019

Uh oh!

magento-engcom-team commented Feb 17, 2019

Uh oh!

magento-engcom-team commented Feb 17, 2019

Uh oh!

Den4ik commented Feb 18, 2019

Uh oh!

sivaschenko commented Feb 18, 2019

Uh oh!

Den4ik commented Feb 18, 2019

Uh oh!

magento-engcom-team commented Feb 18, 2019

Uh oh!

magento-engcom-team commented Feb 18, 2019

Uh oh!

Den4ik commented Feb 19, 2019

Uh oh!

magento-engcom-team commented Feb 19, 2019

Uh oh!

magento-engcom-team commented Feb 19, 2019

Uh oh!

Den4ik commented Feb 19, 2019

Uh oh!

sivaschenko left a comment

Choose a reason for hiding this comment

Uh oh!

sivaschenko Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

Den4ik Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

sivaschenko Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

Den4ik Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

Den4ik commented Feb 20, 2019

Uh oh!

orlangur commented May 24, 2019

Uh oh!

m2-assistant bot commented May 31, 2019

Uh oh!

Den4ik commented Feb 13, 2019 •

edited

Loading

magento-cicd2 commented Feb 13, 2019 •

edited

Loading