Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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)
# for free to join this conversation on GitHub. Already have an account? # 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