Skip to content
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

[Backport] catalog:images:resize total images count calculates incorrectly #18387 #18809

Conversation

vpodorozh
Copy link
Contributor

@vpodorozh vpodorozh commented Oct 25, 2018

Original pull request (#18807)

Description (#18387)

Assumption from issue: >>"catalog:images:resize fails to process all images -> Possible underlying Magento/Framework/DB/Query/Generator issue".
However, there is a problem with total images count calculation select (distinct by image path is not used in count(*) ) - functionality by itself works correctly and process all images.

Fixed Issues

#18387: catalog:images:resize fails to process all images -> Possible underlying Magento/Framework/DB/Query/Generator issue

  1. fixed getCountAllProductImages method that calculates total images count incorrectly;
  2. covered \Magento\Catalog\Model\ResourceModel\Product\Image with unit tests;

UPD from 27.10.2018:
3. Ranged selects always miss the last range - [backport] of #12624

Manual testing scenarios

Variant I

  1. Magento 2.3-develop
  2. Sample data deployed (in order to easily have several test images in store)
Expected result

You do see total images count 801.

Actual result

You do see total images count 3422.

Variant II (synthetic case)

  1. Magento 2.3-develop
  2. Generate products iwth images:
    php bin/magento setup:performance:generate-fixtures setup/performance-toolkit/profiles/ce/medium.xml
  3. apply the same image paths for all products - so images for product will become duplicated.
    For details check catalog_product_entity_media_gallery table.
    example SQL to do this:
    create temporary table zzz as (select NULL as value_id, c.attribute_id, c.value, c.media_type, c.disabled from catalog_product_entity_media_gallery as c); insert into catalog_product_entity_media_gallery select * from zzz;
Expected result

Total images count will be equal to the value from setup/performance-toolkit/profiles/ce/medium.xml in images-count node ( 1000 ).

Actual result

Total images count will be 2 times higher that their exact value.

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)

catalog:images:resize fails to process all images -> Possible underlying Magento/Framework/DB/Query/Generator issue
- fix getCountAllProductImages select and cover class with unit tests.

(cherry picked from commit 6a6079d)
@vpodorozh vpodorozh requested a review from duhon October 25, 2018 12:55
@magento-engcom-team magento-engcom-team added Partner: ISM eCompany Pull Request is created by partner ISM eCompany partners-contribution Pull Request is created by Magento Partner Component: Catalog Release Line: 2.2 labels Oct 25, 2018
@magento-engcom-team
Copy link
Contributor

Hi @vpodorozh. 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 $VERSION instance - deploy vanilla Magento instance

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

vpodorozh and others added 6 commits October 27, 2018 18:06
catalog:images:resize fails to process all images -> Possible underlying Magento/Framework/DB/Query/Generator issue
- fix code style;

(cherry picked from commit 466daaa)
catalog:images:resize fails to process all images -> Possible underlying Magento/Framework/DB/Query/Generator issue
- fix unit test - now it can test also not only full batches, but partial as well (139 images with batch size 100);

(cherry picked from commit f6b0a2a)
catalog:images:resize fails to process all images -> Possible underlying Magento/Framework/DB/Query/Generator issue
- fix unit test - now it can test also not only full batches, but partial as well (139 images with batch size 100);

(cherry picked from commit 546e7ce)
catalog:images:resize fails to process all images -> Possible underlying Magento/Framework/DB/Query/Generator issue
- remove method return type 'void' which can be used in php7.1 or higher.
catalog:images:resize fails to process all images -> Possible underlying Magento/Framework/DB/Query/Generator issue

Ranged selects always miss the last range

Fix unit test. 11 batches expected to handle 105 items:

1: 1-10
2: 11-20
3: 21-30
4: 31-40
5: 41-50
6: 51-60
7: 61-70
8: 71-80
9: 81-90
10: 91-100
11: 101-105

(cherry picked from commit 6c24c0e)
@vpodorozh
Copy link
Contributor Author

@duhon - The other Bug is present in 2.2-develop branch related to the current issue, check #18387 (comment) (I've updated PR description)

This issue #12624 has fixed problem with "Ranged selects always miss the last range" in 2.3-develop branch, however, backport to 2.2-develop branch was not made. So I've made it in the scope of current PR #18809 - you can check the fix here 76bd089 .

Could you please review my changes one more time?
Thank you and sorry for double check - issue become more complicated than it was expected.

vpodorozh and others added 3 commits October 30, 2018 13:26
catalog:images:resize fails to process all images -> Possible underlying Magento/Framework/DB/Query/Generator issue
- fix code style;
catalog:images:resize fails to process all images -> Possible underlying Magento/Framework/DB/Query/Generator issue
- fix code style;

(cherry picked from commit c95ce3c)
@magento-engcom-team magento-engcom-team added this to the Release: 2.2.8 milestone Jan 28, 2019
@magento-engcom-team magento-engcom-team added the Component: Framework/DB USE ONLY for FRAMEWORK RELATED BUG! label Jan 28, 2019
@okorshenko okorshenko removed this from the Release: 2.2.8 milestone Jan 28, 2019
@magento-engcom-team magento-engcom-team merged commit 10a1a88 into magento:2.2-develop Jan 31, 2019
@ghost
Copy link

ghost commented Jan 31, 2019

Hi @vpodorozh, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Catalog Component: Framework/DB USE ONLY for FRAMEWORK RELATED BUG! Partner: ISM eCompany Pull Request is created by partner ISM eCompany partners-contribution Pull Request is created by Magento Partner Progress: accept Release Line: 2.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants