Skip to content

Reindex of Price Indexer (On Save) Occurs Even When Irrelevent Attributes are Changed #39790

Open
@hewgim

Description

@hewgim

Preconditions and environment

Tested on 2.4.5_p10 and 2.4.6
Put all indexers into On Save mode.
Use sample data

Steps to reproduce

  1. Navigate to backend product edit page for any product. I used Proteus Fitness Jackshirt-XS-Black SKU:MJ12-XS-Black
  2. Change any attribute that shouldn't impact the price. I set the Material attibute to 'Burlap'
  3. Click on Save

Expected result

The price index is not updated

Actual result

Any change of any attribute results in price reindexing (and therefore cache flushing). This potentially results in a huge amount of unneccesary reindexing and cache flushing which potentially directly impacts the 'speed' of ALL Magento websites and probably adds to the general belief that Magento is 'resource hungry' and requires super powerful servers.

Additional information

It appears that Magento has logic to only reindex the price index when certain attributes are changed (in save mode)

In vendor/magento/module-catalog/Model/Product.php we see the following code that appears to imply this

    /**
     * Callback for entity reindex
     *
     * @return void
     */
    public function priceReindexCallback()
    {
        if ($this->isObjectNew() || **$this->_catalogProduct->isDataForPriceIndexerWasChanged($this)**) {
            $this->_productPriceIndexerProcessor->reindexRow($this->getEntityId());
        }
    }

In vendor/magento/module-catalog/Helper/Product.php we can find the method isDataForPriceIndexerWasChanged()

    /**
     * Retrieve data for price indexer update
     *
     * @param \Magento\Catalog\Model\Product|array $data
     * @return bool
     */
    public function isDataForPriceIndexerWasChanged($data)
    {
        if ($data instanceof ModelProduct) {
            foreach ($this->_reindexPriceIndexerData['byDataResult'] as $param) {
                if ($data->getData($param)) {
                    return true;
                }
            }
            foreach ($this->_reindexPriceIndexerData['byDataChange'] as $param) {
                if ($data->dataHasChangedFor($param)) {
                    return true;
                }
            }
        } elseif (is_array($data)) {
            foreach ($this->_reindexPriceIndexerData['byDataChange'] as $param) {
                if (isset($data[$param])) {
                    return true;
                }
            }
        }
        return false;
    }

THIS METHOD ALWAYS RETURNS TRUE and so a price reindex always gets kicked off when any attribute is changed !!!

This is because of a bug in the dataHasChangedFor method in vendor/magento/framework/Model/AbstractModel.php

    /**
     * Compare object data with original data
     *
     * @param string $field
     * @return bool
     */
    public function dataHasChangedFor($field)
    {
        $newData = $this->getData($field);
        $origData = $this->getOrigData($field);
        **return $newData != $origData;**  THIS CODE DOES NOT WORK FOR ARRAYS
    }

For the price index the attributes that appear to affect the price reindexing are status, price, special_price, special_to_date, special_from_date, website_ids, gift_wrapping_price, tax_class_id, msrp, and msrp_display_actual_price_type

However, the website_ids variable is actually an array. Whilst the values of website_ids for $newData, and $origData are both ["1"], the array indices are different

$newData is [ 1 => "1" ] and $origData is [ 0 => "1"] and because of this return $newData != $origData returns TRUE even though there has been no change in website ids !! This then kicks off unnecessary price reindexing (and possibly cache flushing)

Image Image

I'm not sure how this passed testing and how no one has spotted this considering the impact it has on indexing and the full page cache and given that it appears to potentially affect every Magento store and has done for years.

A quick and dirty way of fixing this would be to change the code to something like:

    public function dataHasChangedFor($field)
    {
        $newData = $this->getData($field);
        $origData = $this->getOrigData($field);

        if (is_array($newData)) {
            sort($newData);
        }

        if (is_array($origData)) {
            sort($origData);
        }

        return $newData != $origData;
    }

Release note

No response

Triage and priority

  • Severity: S0 - Affects critical data or functionality and leaves users without workaround.
  • Severity: S1 - Affects critical data or functionality and forces users to employ a workaround.
  • Severity: S2 - Affects non-critical data or functionality and forces users to employ a workaround.
  • Severity: S3 - Affects non-critical data or functionality and does not force users to employ a workaround.
  • Severity: S4 - Affects aesthetics, professional look and feel, “quality” or “usability”.

Metadata

Metadata

Assignees

Labels

Issue: needs updateAdditional information is require, waiting for responseReported on 2.4.6Indicates original Magento version for the Issue report.Triage: Dev.ExperienceIssue related to Developer Experience and needs help with Triage to Confirm or Reject it

Type

No type

Projects

Status

Needs Update

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions