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

Reindex EAV values only for active storeviews #2651

Merged
merged 5 commits into from
Oct 14, 2022
Merged

Reindex EAV values only for active storeviews #2651

merged 5 commits into from
Oct 14, 2022

Conversation

fballiano
Copy link
Contributor

EAV indexers now are indexing all datas also from storeviews that are disabled (manage by configuration field core_store.is_active), creating a lot of records in the catalog_product_index_eav and catalog_product_index_eav_decimal tables.

This PR modifies the idexers in order to exclude disabled storeviews from the EAV indexing.

Fixed Issues

  1. Fixes Indexers shouldn't reindex disabled storeviews #2650

Manual testing scenarios (*)

  1. create a multi store view environment
  2. create indexable EAV attributes
  3. create one or more products with indexable attributes
  4. disable one or more store views
  5. run php shell/indexer.php --reindex catalog_product_attribute
  6. check tables catalog_product_index_eav and catalog_product_index_eav_decimal, you'll see that records related to the disabled storeviews are created (column store_id)
  7. apply this PR
  8. run php shell/indexer.php --reindex catalog_product_attribute
  9. check tables catalog_product_index_eav and catalog_product_index_eav_decimal, you'll see that records related to the disabled storeviews are not created anymore

@github-actions github-actions bot added the Component: Catalog Relates to Mage_Catalog label Oct 11, 2022
@sreichel
Copy link
Contributor

When enable/disable a store, we should display a notice that reindixing is required (or run it automatically)?

@elidrissidev
Copy link
Member

elidrissidev commented Oct 12, 2022

@sreichel got a point, we should change the process's status to require_reindex upon enabling a store.

@github-actions github-actions bot added the Component: Adminhtml Relates to Mage_Adminhtml label Oct 12, 2022
@fballiano
Copy link
Contributor Author

hey guys, good idea, I just added a commit that sets require_reindex or catalog_product_attribute when a store view gets activated :-)

@elidrissidev
Copy link
Member

Functionality wise, all works well for me. But I'd prefer if the code that changes process status was inside the indexer model (app/code/core/Mage/Catalog/Model/Product/Indexer/Eav.php) by adding Store model to $_matchedEntities:

protected $_matchedEntities = [
    ...
    Mage_Core_Model_Store::ENTITY => [
        Mage_Index_Model_Event::TYPE_SAVE
    ]
];

then changing the status inside the _registerEvent method when store status is changed to enabled:

...
} elseif ($entity == Mage_Core_Model_Store::ENTITY) {
    if ($event->getType() === Mage_Index_Model_Event::TYPE_SAVE) {
        /** @var Mage_Core_Model_Store $store */
        $store = $event->getDataObject();
        if ($store->getOrigData('is_active') != $store->getIsActive() && $store->getIsActive()) {
            $event->getProcess()->changeStatus(Mage_Index_Model_Process::STATUS_REQUIRE_REINDEX);
        }
    }
}

anyways it's just a matter of preference.

@github-actions github-actions bot removed the Component: Adminhtml Relates to Mage_Adminhtml label Oct 12, 2022
@fballiano
Copy link
Contributor Author

wow man, good stuff! Added your code, tested and it's great!

@fballiano fballiano merged commit dcf174b into OpenMage:1.9.4.x Oct 14, 2022
@fballiano fballiano deleted the reindex_enabled_storeviews branch October 14, 2022 09:09
@github-actions
Copy link
Contributor

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit dcf174b. ± Comparison against base commit 1641eab.

@fballiano
Copy link
Contributor Author

@elidrissidev totally agree, I was thinking about it, going to work on the v19->v20 PR asap

@fballiano
Copy link
Contributor Author

@elidrissidev #2697 updated with everything

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Catalog Relates to Mage_Catalog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indexers shouldn't reindex disabled storeviews
3 participants