fix(wishlist): stop recalculating item count on every request - #41152
Open
Eddcapone wants to merge 4 commits into
Open
fix(wishlist): stop recalculating item count on every request#41152Eddcapone wants to merge 4 commits into
Eddcapone wants to merge 4 commits into
Conversation
hasDisplayOutOfStockProducts() only checked whether a session value was ever set, not whether it changed, unlike its sibling condition right below it. Since calculate() sets that value unconditionally, this made the third condition permanently true after the first call, defeating the caching getItemCount() is supposed to provide for the rest of the customer session. Fixes magento#41151
|
Hi @Eddcapone. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
Adds a second unit test proving that removing the buggy hasDisplayOutOfStockProducts() existence check did not weaken change detection: with that check completely absent, a genuine change to the out-of-stock display setting alone still forces calculate() to run (verified via isLoggedIn()/setWishlistItemCount() expectations). Also mocks Context::getEventManager(), which calculate() now reaches for the first time in this test class.
Author
|
@magento run all tests |
The new scopeConfig/eventManager mocks needed to cover the stale section-count cache check pushed DataTest to 16 properties, exceeding PHPMD's TooManyFields threshold of 15. Suppress it the same way WishlistTest already does for the same rule.
Author
|
@magento run all tests |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description (*)
Magento\Wishlist\Helper\Data::getItemCount()decides whether to recalculate the customer's wishlist item count based on four conditions. Three of them correctly compare a stored value against the current one. The third,hasDisplayOutOfStockProducts(), only checks whether a value was ever stored, not whether it changed.calculate()unconditionally callssetDisplayOutOfStockProducts()every time it runs, which means that condition becomes permanentlytruefor the rest of the customer session after the very first call togetItemCount(). From that point on, this single condition forces a full recalculation on every subsequent call, regardless of whether the wishlist display type, the out-of-stock display setting, or the actual wishlist contents changed. The caching this method is supposed to provide is effectively disabled for the rest of the session.This fix removes the faulty existence check. The remaining value comparison (
$currentDisplayOutOfStockProducts != $storedDisplayOutOfStockProducts) already covers "changed since last time", including the first-ever call, since!hasWishlistItemCount()already forces recalculation then regardless.Impact in practice: every
customer/section/loadrequest that includes thewishlistsection (e.g. triggered by the default header mini-wishlist link) pays the full cost of the wishlist collection query and item resolution again, on every single request, for the rest of the customer's session, instead of reusing the cached count.Two things worth noting about correctness, since removing a condition always raises the question of whether it silently changes behavior:
Controller/Index/Add.php,Controller/Index/Remove.phpandModel/ItemCarrier.phpall callcalculate()directly and unconditionally right after the action. The removed condition played no role there.Related Pull Requests
None.
Fixed Issues (if relevant)
Wishlist\Helper\Data::getItemCount()#41151Manual testing scenarios (*)
wishlistcustomer-data section is requested at least once (e.g. via the default header wishlist link).Stores > Configuration > Customers > Wishlist > General Options > Display Wishlist SummaryorStores > Configuration > Catalog > Inventory > Product Stock Options > Display Out of Stock Products, trigger thewishlistsection again, e.g. by callingcustomer/section/load?sections=wishlista second time, or by opening another page.dev/debugquery log as a repeated wishlist item collection query, and as consistently elevated response time for that request). After the fix: the cached value from the customer session is reused, and no wishlist collection query runs again, as long as nothing relevant changed.Display Wishlist SummaryorDisplay Out of Stock Products, or add/remove a wishlist item, and confirm the count still recalculates and reflects the change correctly, i.e. the cache is still invalidated when something actually changes.Questions or comments
None.
Contribution checklist (*)