-
Notifications
You must be signed in to change notification settings - Fork 9.4k
#26065 isSaleable cache and optimize result for configurable products #26071
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
#26065 isSaleable cache and optimize result for configurable products #26071
Conversation
Hi @ilnytskyi. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
93c3901
to
87842d3
Compare
@@ -585,7 +592,7 @@ protected function getGalleryReadHandler() | |||
* @param \Magento\Catalog\Model\Product $product | |||
* @return \Magento\ConfigurableProduct\Model\ResourceModel\Product\Type\Configurable\Product\Collection | |||
*/ | |||
public function getUsedProductCollection($product) | |||
protected function getLinkedProductCollection($product) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello and thank you for your contribution. Just a question: what's the reason behind the introduction of this new method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aleron75
as described in #26065
the swatches plugin is not needed Magento\Swatches\Model\Plugin\Configurable::afterGetUsedProductCollection
so isSaleable
can call internal method. It slightly improves performance if swatches are disabled on category page (listing page). Before, when the code called isSaleable
it was always called afterGetUsedProductCollection
that just adds attributes to select, however they are not needed in this check.
It also helps if isSaleable
is called a few times in different places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, @ilnytskyi I didn't explain myself. I got the point of the optimization, I was just wondering why did you introduce the additional getLinkedProductCollection()
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, to avoid code duplication inside isSalable
. (collection building)
getUsedProductCollection
is used inside isSalable
so i decided to use it as wrapper for the collection building then move its logic to the new method. This way
getUsedProductCollection
can be used as previously and
getLinkedProductCollection only internal method that is not affected by plugins
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok great, thank you!
Hi @aleron75, thank you for the review.
|
Hi @ilnytskyi, thank you for your contribution! |
Description (*)
Improves performance of
\Magento\ConfigurableProduct\Model\Product\Type\Configurable::isSalable
for multiple callsExclude swatches plugin
branch 2.4-develop + demo data
Before
https://user-images.githubusercontent.com/3192770/71307970-9eb7e400-23f6-11ea-86f3-e60951e639da.png
After
https://user-images.githubusercontent.com/3192770/71307989-be4f0c80-23f6-11ea-8081-9abd5539f030.png
Fixed Issues (if relevant)
Manual testing scenarios (*)
\Magento\ConfigurableProduct\Model\Product\Type\Configurable::isSalable
many timesQuestions or comments
Contribution checklist (*)