Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jul 12, 2019
Merged

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

merged 7 commits into from
Jul 12, 2019

Conversation

Den4ik
Copy link
Contributor

@Den4ik Den4ik commented Jun 1, 2019

Original Pull Request

#21200

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

  1. 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 (*)

  1. Create new module
  2. Add event observer for catalog_block_product_list_collection
  3. 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)

@m2-assistant
Copy link

m2-assistant bot commented Jun 1, 2019

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 give me test instance - deploy test instance based on PR changes
  • @magento give me 2.2-develop instance - deploy vanilla Magento instance

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

@ghost ghost assigned orlangur Jun 1, 2019
@magento-engcom-team
Copy link
Contributor

Hi @orlangur, thank you for the review.
ENGCOM-5199 has been created to process this Pull Request

ysapiga
ysapiga reviewed Jun 14, 2019
View reviewed changes
@ysapiga ysapiga self-assigned this Jun 14, 2019
@Den4ik
Copy link
Contributor Author

Den4ik commented Jun 14, 2019

@ysapiga Done

@engcom-Golf engcom-Golf assigned engcom-Golf and unassigned ysapiga Jun 18, 2019
@engcom-Golf
Copy link
Contributor

@orlangur please approve changes

@Den4ik
Copy link
Contributor Author

Den4ik commented Jun 24, 2019

@orlangur

please approve changes

@Den4ik Den4ik requested a review from sivaschenko June 25, 2019 19:59
@ghost ghost assigned sidolov Jul 5, 2019
korostii
korostii reviewed Jul 9, 2019
View reviewed changes
lib/internal/Magento/Framework/Data/Collection/AbstractDb.php
@@ -217,7 +218,7 @@ public function getSize()
{
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@korostii no. Value can be set during getSelectCountSql method execution, and Elasticsearch adapter does it at some point.

@engcom-Delta engcom-Delta self-assigned this Jul 12, 2019
@sivaschenko
Copy link
Member

@magento run all tests

@engcom-Delta
Copy link
Contributor

✔️ QA passed

@magento-engcom-team magento-engcom-team merged commit 4fdc872 into magento:2.2-develop Jul 12, 2019
@m2-assistant
Copy link

m2-assistant bot commented Jul 12, 2019

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-engcom-team pushed a commit that referenced this pull request Jul 12, 2019
@sidolov
ENGCOM-5199: [Backport] Don't load product collection in review obser…
6488645 
…ver #23094
@magento-engcom-team magento-engcom-team added this to the Release: 2.2.10 milestone Jul 12, 2019
@sidolov sidolov added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Sep 14, 2019
@Den4ik Den4ik deleted the back-port-pull-21200 branch November 4, 2021 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@p-bystritsky p-bystritsky p-bystritsky left review comments

@korostii korostii korostii left review comments

@ysapiga ysapiga ysapiga left review comments

@orlangur orlangur orlangur approved these changes

@sidolov sidolov sidolov approved these changes

@sivaschenko sivaschenko Awaiting requested review from sivaschenko

Assignees

@orlangur orlangur

@sivaschenko sivaschenko

@sidolov sidolov

Labels
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Component: Data Component: Review Progress: accept Release Line: 2.2
Projects
None yet
Milestone
Release: 2.2.10
Development

Successfully merging this pull request may close these issues.