Mview Indexers getList should return integer values of id's and not strings

[mhodge13]

Description (*)

When using Maria DB I have noticed that when the indexer_update_all_views cron job runs that some of the indexer queries have a mix of entity_id values that are strings and some that are integers. When running a describe on the query I have confirmed that Maria DB will look at every row in the table and not use the entity_id values that are in the WHERE clause. This does not seem to be an issue when using MySql but is in an issue when using Maria DB.

Manual testing scenarios (*)

When reviewing the queries for the catalog_product_attribute indexer I saw a mix of string and integer id values.

1. Run a describe on the query, notice mix of string and integer
   DESC DELETE FROM catalog_product_index_eav WHERE (entity_id IN('2764369', 2766213));
   rows returned shows all rows in table: 9,133,650 and Extra shows "Using Where"

2. Run the same query but change the entity_ids to be integers
   DESC DELETE FROM catalog_product_index_eav WHERE (entity_id IN(2764369, 2766213));
   rows returned shows only 23 rows and Extran shows "Using Where"

Maria DB version tested on: 10.1.38-MariaDB-1~xenial mariadb.org binary distribution

[m2-assistant]

Hi @mhodge13. 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

[magento-cicd2]

All committers have signed the CLA.

[swnsma]

Hi @mhodge13,

Thank you for your contribution!

I want to say that you are digging really interesting topic.
However I would like to ask you clarify exact case how described by you issue can be reproduced.
Could you also clarify which version of Magento you have used to reproduce this issue?

I have made small research and I have found that since 2.2.8 return of this function is converted to int in

magento2/lib/internal/Magento/Framework/Mview/View.php

Line 288 in 0c78690

$ids = array_map('intval', $this->getChangelog()->getList($versionFrom, $versionTo));

Also these changes are present in 2.3.1:

magento2/lib/internal/Magento/Framework/Mview/View.php

Line 288 in 3609773

$ids = array_map('intval', $this->getChangelog()->getList($vsFrom, $versionTo));

But your fix I like more your fix, because it force \Magento\Framework\Mview\View\ChangelogInterface::getList to follow described contract in PHPDOC.

Lets check together:

1. Have your issue was reproduced (or can be reproduced) on version which I mentioned?
2. If your issue was fixed in 2.2.8/2.3.1 - I would like propose to replace fix which was done in
   lib/internal/Magento/Framework/Mview/View.php:288 with your changes. Then please revert changes in this line and fix all failed tests in build.

[mhodge13]

Hi @swnsma

The version that was causing this issue is in Magento 2.2.7. It seems that the changes in 2.2.8 and 2.3.1 do in fact fix this issue. I was checking the PHPDOC as well and saw that the return value of the function should in fact be integers. I will revert the other changes and recommit to trigger the checks again.

Some of the issues found during the static tests were not changes made by me. Do you want me to fix those as well?

[swnsma]

Hi @mhodge13,
According to Magento refactoring policy, we need to have a green build to approve the pull request.
So, it will be nice if you fix static tests as well

[mhodge13]

@swnsma Everything is passing now except a banned keyword in the Static Test Build

https://testing-service.magento-community.engineering/reports/magento/magento2/pull/22920/623b69a1-db69-4947-8ed6-3856492c9548/30865/Statics/report-sanity-ce.html

Can you let me know how you want me to fix this one please?

[swnsma]

Hi @mhodge13,

We almost done.
Please check and fix my minor notices.

[mhodge13]

@swnsma Can you please review the Unit Test build issue since it is calling out a js error and the static test issue is because of a banned word ("changelog").

Thanks, Mark

[magento-engcom-team]

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

[magento-engcom-team]

@mhodge13 thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

[Nazar65]

QA Passed ✔️

[m2-assistant]

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