Take required product attributes into account when caching used products #32698
Conversation
|
Hi @Maikel-Koek. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. 🕙 You can find the schedule on the Magento Community Calendar page. 📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket. 🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
|
@magento run all tests |
$skipStockFilter was accidentally changed to true
|
@magento run Integration Tests |
|
@magento run all tests |
gabrieldagama
added
the
Priority: P3
|
@magento create issue |
There was a problem hiding this comment.
@Maikel-Koek Thanks for you contribution.
Please look at my comments below
| $requiredAttributeIds = $requiredAttributeIds ?: []; | ||
| array_unshift($requiredAttributeIds, $product->getId()); | ||
|
|
||
| $cacheKey = $this->getUsedProductsCacheKey($requiredAttributeIds); | ||
| if (!$product->hasData($this->_usedProducts) || !isset($product->getData($this->_usedProducts)[$cacheKey])) { | ||
| $collection = $this->getConfiguredUsedProductCollection($product, false, $requiredAttributeIds); | ||
| $usedProducts = array_values($collection->getItems()); | ||
| $product->setData($this->_usedProducts, $usedProducts); | ||
| $usedProductsCache = $product->getData($this->_usedProducts); | ||
| $usedProductsCache[$cacheKey] = $usedProducts; | ||
| $product->setData($this->_usedProducts, $usedProductsCache); | ||
| } | ||
|
|
||
| return $product->getData($this->_usedProducts); | ||
| return $product->getData($this->_usedProducts)[$cacheKey]; |
There was a problem hiding this comment.
I believe that this method could be reverted to 2487db1b#diff-d9457baecd561c1374a935c202ad3a4ebf82a9c26b80c486dd97ac44da81d383L1245-L1257 with some changes:
- use
$this->customerSessioninstead of deprecatedgetCustomerSession() - add
$requiredAttributeIdsintoloadUsedProductscall
And refactoring loadUsedProducts method
m2-community-project
bot
added
Progress: needs update
and removed
Progress: pending review
labels
Den4ik
added
Severity: S2
|
Hi I am closing this PR for now as it seems like it has become stale and still needs work. Please feel free to reopen if you wish to continue. Thanks! |
Description (*)
When calling
Magento\ConfigurableProduct\Model\Product\Type\Configurable::getUsedProducts()with custom required attributes after that method has already been called before, the collection doesn't load your custom defined attributes.The caching plugin around this method,
Magento\ConfigurableProduct\Model\Plugin\Frontend\UsedProductsCache::aroundGetUsedProducts()actually does take the passed required attributes into account, by generating a cache key, but once proceeded to the original method, this is lost again, when$product->getData($this->_usedProducts)already contains a product collection.Manual testing scenarios (*)
This can be confirmed by running the added integration test without the changes I proposed, you'll see it will fail, because the
meta_descriptionattribute of the product will benullat all times.Contribution checklist (*)
Resolved issues: