Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.function uc_cart_update_item_object is called when a user hits 'checkout' or 'udpate' and it loops through all cart items calling hook_update_cart_item. This in turn fires uc_product_update_cart_item multiple times and every time it calls uc_cart_get_contents(NULL, 'rebuild'), rebuilding the static cart contents which is trying to be built.
We have a product kit with 45 products in it and when we go to checkout we get WSOD and find that about 500 node loads were run because the static cart cache keeps getting cleaned out. The cart should not be rebuilt multiple times.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | 499090.cart_cache_clear.1.patch | 4.08 KB | rszrama |
| #9 | 499090.cart_cache_clear.patch | 4.04 KB | rszrama |
| #3 | 499090.better_rebuilding.2.x.patch | 4.26 KB | cha0s |
| #1 | ubercart.cvs_.patch | 5.17 KB | Anonymous (not verified) |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedand here is a patch.
the high level description is, just rebuild things once, not per item, otherwise we end up with as many node-loads as your memory limit can handle, then WSOD.
Comment #2
rszrama CreditAttribution: rszrama commentedComment #3
cha0s CreditAttribution: cha0s commentedWell done. I did reroll the patch though since the final bit of context didn't match, and it wouldn't apply.
Comment #4
rszrama CreditAttribution: rszrama commentedComment #5
rszrama CreditAttribution: rszrama commentedI'm really not sure what to make of this patch. I'm having a hard time specifically reconciling justinrandell's high level description with the fact that it simply seems to be sprinkling cache flushes about in various parts of the code. Is anyone using either of these patches live?
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous commentedits not just additions - its a removal of the rebuild from uc_product_update_cart_item. that rebuild was *very, very, very ... * expensive, and in fact recursive in some cases, when adding items to a cart.
so the extra rebuilds in code actually lead to substantially less calls at runtime, because we try to rebuild the whole cart when it changes, rather than rebuild it for each item added.
make any more sense now?
Comment #7
rszrama CreditAttribution: rszrama commentedI believe so. That was my hunch.
So, you're taking the cart rebuild out of a loop and moving it to the functions that would be responsible for actually altering cart contents? Is it actually necessary, in fact, to put it all over the place? Or can we just put it at the end of a single cart updating function, if such a beast exists?
Also, do you have any feedback on cha0s's version of the patch?
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedi didn't see such a beast, but it would make sense if for it to exist from an API point of view.
can't say i have time to work on it right now, but i'd review a patch that introduced it.
Comment #9
rszrama CreditAttribution: rszrama commentedOk, so I had to reroll this. As I did it I noticed that all the places we're manually rebuilding the cache are after a call to uc_cart_add_item() (for obvious reasons). What isn't obvious is why we don't simply change uc_cart_add_item() to rebuild the cache if it does indeed add an item to the cart. Wouldn't that get around having to sprinkle the calls to clear the cache in all the other modules? Can you think of any adverse affects?
(Attaching the rerolled version of the prior patch.)
Comment #10
Island Usurper CreditAttribution: Island Usurper commentedWe shouldn't do this because uc_cart_add_item() is used inside a loop in cart links, uc_cart_login_update(), and uc_product_kit_add_to_cart(). When I mentioned this to Ryan, he brought up adding "yet another" parameter to uc_cart_add_item() to control whether it rebuilds the cart cache or not. I think that could work. I would default the $rebuild parameter to TRUE since I think we use it that way more often.
Comment #11
rszrama CreditAttribution: rszrama commentedOk, re-rolled this based on my suggestion and Lyle's concurrence.
Comment #12
Island Usurper CreditAttribution: Island Usurper commentedDoesn't seem to break anything. I'm not really set up to test how it performs, though.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedok, i'll look test this shortly. i'm not sure if this will help the recursive calls, but this looks like an ok approach.
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedbackported this to our production code, and it works well. this is RTBC as far as i'm concerned.
Comment #15
rszrama CreditAttribution: rszrama commentedHuttah!