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

EAV Config Cache #2993

Merged
merged 30 commits into from
Feb 17, 2023
Merged

EAV Config Cache #2993

merged 30 commits into from
Feb 17, 2023

Conversation

davidhiendl
Copy link
Contributor

@davidhiendl davidhiendl commented Jan 24, 2023

Description (*)

Cache EAV entity and attributes as well as complementary data like store labels and attribute set information. This will eliminate several database queries per request and replace them with a single cache request.

Related Pull Requests

#532 similar PR (likely abandoned) with some similarities

Manual testing scenarios (*)

TBD, probably lots in addition to tests

@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Api2 Relates to Mage_Api2 Component: Catalog Relates to Mage_Catalog Component: CatalogIndex Relates to Mage_CatalogIndex Component: Downloadable Relates to Mage_Downloadable Component: Eav Relates to Mage_Eav Component: Sales Relates to Mage_Sales Component: Tax Relates to Mage_Tax documentation labels Jan 24, 2023
@davidhiendl
Copy link
Contributor Author

@luigifab It's mostly done, there is maybe one or two small improvements but they may break stuff so I will hold off on them for now.

@elidrissidev elidrissidev marked this pull request as draft January 24, 2023 10:14
@elidrissidev elidrissidev added the performance Performance related label Jan 24, 2023
@luigifab
Copy link
Contributor

luigifab commented Jan 24, 2023

Thanks! I applied, and for now, I have, (I haven't investigated yet, I will do):

  • Invalid attribute requested: url on product page: this is us ->addAttributeToSelect(['name', 'url'])
  • Warning: Undefined array key 2 in app/code/core/Mage/Eav/Model/Config.php on line 488 on home page: fixed with latest changes

@davidhiendl
Copy link
Contributor Author

Thanks! I applied, and for now, I have, (I haven't investigated yet, I will do):

  • Invalid attribute requested: url on product page
  • Warning: Undefined array key 2 in app/code/core/Mage/Eav/Model/Config.php on line 488 on home page

@luigifab

  • The 1st error I cannot reproduce. Is that with a default openmage installation (no custom modules/theme)? Because I didn't have those when testing.
  • The 2nd error is an oversight for the fallback resolve of entity attributes when there is no store_id/attribute_set_id specified.

@davidhiendl davidhiendl changed the title [WIP] EAV Config Cache EAV Config Cache Jan 25, 2023
@davidhiendl
Copy link
Contributor Author

After some minors adjustments I think this is ready for testing by someone else than me. I've already started testing it in a real shop (staging environment only so far) without any issues.

… attributes loaded at all times), rewrite product compare to use eav/config
@davidhiendl davidhiendl marked this pull request as ready for review January 26, 2023 12:03
Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we're releasing a "rc1" version, if we don't do this now...

@fballiano fballiano merged commit fc97382 into OpenMage:20.0 Feb 17, 2023
fballiano added a commit that referenced this pull request Feb 17, 2023
@luigifab
Copy link
Contributor

I forgot some comments ago: before this PR, you can ->addAttributeToSelect('test') when test product attribute does not exist, now you can't:

Invalid attribute requested: test

#0 app/code/core/Mage/Eav/Model/Entity/Collection/Abstract.php(443): Mage::exception()
#1 app/code/core/Mage/Catalog/Model/Resource/Product/Collection.php(504): Mage_Eav_Model_Entity_Collection_Abstract->addAttributeToSelect()
#2 app/code/core/Mage/Eav/Model/Entity/Collection/Abstract.php(421): Mage_Catalog_Model_Resource_Product_Collection->addAttributeToSelect()
#3 app/code/core/Mage/Catalog/Model/Resource/Product/Collection.php(504): Mage_Eav_Model_Entity_Collection_Abstract->addAttributeToSelect()
#4 app/code/community/Luigifab/Urlnosql/Controller/Router.php(201): Mage_Catalog_Model_Resource_Product_Collection->addAttributeToSelect()
#5 app/code/community/Luigifab/Urlnosql/Controller/Router.php(82): Luigifab_Urlnosql_Controller_Router->getProduct()
#6 app/code/core/Mage/Core/Controller/Varien/Front.php(188): Luigifab_Urlnosql_Controller_Router->match()
#7 app/code/core/Mage/Core/Model/App.php(371): Mage_Core_Controller_Varien_Front->dispatch()
#8 app/Mage.php(750): Mage_Core_Model_App->run()
#9 index.php(41): Mage::run()
#10 {main}
  thrown in app/Mage.php on line 648
  catched by Mage::printException()

@luigifab
Copy link
Contributor

I found a new bug (perhaps it's only me, perhaps this is other PR that I also use... I don't searched yet).

  • Go to customer edit page in backend (I have multiple store views, one website, and customer/account_share/scope is per website).
  • Clear all cache.
  • Refresh the page.

I have (with OpenMage 20.1.0-rc1 and PHP 8.2):

Cannot call addStoreLabel for different store views on the same collection

#0 app/code/core/Mage/Eav/Model/Config.php(213): Mage_Eav_Model_Resource_Entity_Attribute_Collection->addStoreLabel()
#1 app/code/core/Mage/Eav/Model/Config.php(172): Mage_Eav_Model_Config->_loadEntityAttributes()
#2 app/code/core/Mage/Eav/Model/Config.php(532): Mage_Eav_Model_Config->_initializeStore()
#3 app/code/core/Mage/Eav/Model/Entity/Abstract.php(503): Mage_Eav_Model_Config->getEntityAttributeCodes()
#4 app/code/core/Mage/Eav/Model/Entity/Abstract.php(938): Mage_Eav_Model_Entity_Abstract->loadAllAttributes()
#5 app/code/core/Mage/Core/Model/Abstract.php(292): Mage_Eav_Model_Entity_Abstract->load()
#6 app/code/core/Mage/Adminhtml/controllers/CustomerController.php(61): Mage_Core_Model_Abstract->load()
#7 app/code/core/Mage/Adminhtml/controllers/CustomerController.php(112): Mage_Adminhtml_CustomerController->_initCustomer()
#8 app/code/core/Mage/Core/Controller/Varien/Action.php(428): Mage_Adminhtml_CustomerController->editAction()
#9 app/code/core/Mage/Core/Controller/Varien/Router/Standard.php(262): Mage_Core_Controller_Varien_Action->dispatch()
#10 app/code/core/Mage/Core/Controller/Varien/Front.php(188): Mage_Core_Controller_Varien_Router_Standard->match()
#11 app/code/core/Mage/Core/Model/App.php(371): Mage_Core_Controller_Varien_Front->dispatch()
#12 app/Mage.php(750): Mage_Core_Model_App->run()
#13 index.php(41): Mage::run()
#14 {main}
  thrown in app/code/core/Mage/Eav/Model/Resource/Entity/Attribute/Collection.php on line 441
  catched by Mage::printException()

@davidhiendl
Copy link
Contributor Author

I will create a follow-up PR for the non-existent attribute, that's definitely a bug and will cause problems. I will take a look at the other problem after that, but I can't reproduce it just yet.

@darinda
Copy link
Contributor

darinda commented Feb 24, 2023

i can confirm the Exception(Cannot call addStoreLabel for different store views on the same collection) appearing in the backend when viewing a placed order ([..]sales_order/view/order_id/%orderID%) and when the caches are purged.

It happens with OpenMage 20.1.0-rc1 and PHP 8.0 as well as PHP 8.1. Didn't test 8.2 yet.

@davidhiendl
Copy link
Contributor Author

Okay, I figured out the reason. It's because Mage_Eav_Model_Entity_Type::getAttributeCollection re-uses the same collection. I'm testing a rewrite of this method right now and will submit another followup PR shortly.

@luigifab
Copy link
Contributor

luigifab commented Mar 3, 2023

With #2044 and #3057, I approve.
For us it works so good with PHP 8.0 and 8.2.

@ADDISON74
Copy link
Contributor

This PR requires revision because it introduced two issues that we reported today. IT is possible that find more.

davidhiendl added a commit to davidhiendl/magento-lts that referenced this pull request Jul 1, 2024
…orrectly pre-filtered for the layered navigation
davidhiendl added a commit to davidhiendl/magento-lts that referenced this pull request Jul 1, 2024
…orrectly pre-filtered for the layered navigation
davidhiendl added a commit to davidhiendl/magento-lts that referenced this pull request Jul 1, 2024
…nger sorted correctly by attribute group order in attribute comparison
fballiano pushed a commit that referenced this pull request Jul 1, 2024
…orted correctly by attribute group order in attribute comparison (#4064)
fballiano pushed a commit that referenced this pull request Jul 2, 2024
…ly pre-filtered for the layered navigation (#4063)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml Component: Api2 Relates to Mage_Api2 Component: Catalog Relates to Mage_Catalog Component: CatalogIndex Relates to Mage_CatalogIndex Component: CatalogSearch Relates to Mage_CatalogSearch Component: Downloadable Relates to Mage_Downloadable Component: Eav Relates to Mage_Eav Component: Sales Relates to Mage_Sales Component: Tax Relates to Mage_Tax documentation performance Performance related phpstan
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants