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

Don't save mass_action event to db when saving stock item model #191

Closed

Conversation

bob2021
Copy link
Contributor

@bob2021 bob2021 commented Feb 27, 2017

This is part of a group of PRs containing the changes discussed in issue #152

When a stock item model is saved independently of a product model (eg. during checkout) and 'Display Out of Stock Products' = 'No', the following happens:

  • A mass_action index event is registered
  • The stock indexer runs
  • The mass_action event from earlier is processed; these indexers run:
    • catalog_product_attribute
    • catalog_product_price
    • catalogsearch_fulltext

The problems are:

  • Saving a mass_action event just to load it later on in the same request is inefficient
  • The mass_action event isn't actually required unless the stock status has changed
  • One mass_action record is shared between all requests:
    • If indexers are set to manual mode the event data can grow very large (megabytes)
    • When the event is loaded it could contain data from other requests; events could be processed more than once across different requests

This PR has been updated since I initially opened it, the original approach had the advantage that it indexed everything in one pass, but it was more complex and less backwards compatible. The original is here.

This PR keeps the mass_action event in memory instead of saving it to the database, and it only processes the mass_action if the stock status has changed.

@bob2021 bob2021 closed this Mar 1, 2017
@LeeSaferite
Copy link
Contributor

@bob2021 Why did you close the PR?

@bob2021
Copy link
Contributor Author

bob2021 commented Mar 1, 2017

Sorry, I should have added a comment - I've made some mistakes in my understanding of how mass_action events are processed. I think this PR is probably ok but I wanted to double check it before anyone else spent time looking at it - I'll reopen if it looks ok.

@bob2021 bob2021 reopened this Mar 3, 2017
@bob2021 bob2021 force-pushed the avoid-stock-item-mass_action-index branch from f12d40e to 629846b Compare March 3, 2017 16:18
@bob2021 bob2021 changed the title Avoid registering mass_action index events when saving stock item model Don't save mass_action event to db when saving stock item model Mar 3, 2017
@bob2021 bob2021 force-pushed the avoid-stock-item-mass_action-index branch from 629846b to 0613488 Compare March 6, 2017 11:22
@bob2021 bob2021 changed the base branch from 1.9.2.4 to 1.9.3.x March 6, 2017 11:22
@tmotyl tmotyl added the review needed Problem should be verified label Mar 22, 2017
@seansan
Copy link
Contributor

seansan commented Dec 15, 2017

+1 for the idea (not tested) any update on this one?

@sreichel sreichel added the Component: CatalogInventory Relates to Mage_CatalogInventory label Jun 5, 2020
@fballiano fballiano closed this Jun 2, 2022
@fballiano fballiano reopened this Jun 2, 2022
@fballiano fballiano changed the base branch from 1.9.3.x to 1.9.4.x June 2, 2022 11:19
@bob2021
Copy link
Contributor Author

bob2021 commented Jun 6, 2022

I am no longer involved with Magento development and I will not be contributing any further changes to this PR so I am closing it.

@bob2021 bob2021 closed this Jun 6, 2022
@addison74
Copy link
Contributor

I understand the situation but a simple statement that you are no longer involved in the project was enough without closing the PR, leaving it to the appreciation of others who are still active if your work so far can be useful OpenMage.

@addison74 addison74 reopened this Jun 6, 2022
@addison74 addison74 added needs investigation and removed review needed Problem should be verified labels Jun 6, 2022
@fballiano fballiano force-pushed the avoid-stock-item-mass_action-index branch from 68b58e2 to c8fa765 Compare May 13, 2023 08:25
@fballiano fballiano changed the base branch from 1.9.4.x to main May 13, 2023 08:25
@github-actions github-actions bot added Component: AdminNotification Relates to Mage_AdminNotification Component: Adminhtml Relates to Mage_Adminhtml Component: Admin Relates to Mage_Admin Component: Cm/RedisSession Relates to Cm_RedisSession ddev documentation environment labels May 13, 2023
@github-actions github-actions bot added phpstan PHPStorm phpunit and removed Component: CatalogInventory Relates to Mage_CatalogInventory labels May 13, 2023
@fballiano
Copy link
Contributor

I've solved the conflicts (hopefully correctly since this code is extremely old) and rebased to main. I think this PR is worth checking and testing, please some admins take a look at this.

@github-actions github-actions bot added Component: CatalogInventory Relates to Mage_CatalogInventory and removed Mage.php Relates to app/Mage.php htaccess php-cs-fixer phpunit phpstan phpcs PHPStorm Component: AdminNotification Relates to Mage_AdminNotification ddev Component: Cm/RedisSession Relates to Cm_RedisSession Component: Admin Relates to Mage_Admin Component: Adminhtml Relates to Mage_Adminhtml labels May 13, 2023
@fballiano
Copy link
Contributor

So, I took a better look at this and:

  • it could be needed to reindex a product also if the is_outofstock didn't change, because there could be low stock triggers or something like that
  • removing the "save to db" is interesting but everything has to be processed in the same run, or it won't be resumable
  • the way it's coded now kinda resemble a queue like approach, it's not the best since OM doesn't really have queues but I (today) feel it's better to have it this way in case we can snap-in a queue in the future

So, for the time being, I prefer to close it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants