If a product cannot be added to the cart because hook_add_to_cart indicated failure, any querystring in the referring URL is lost.

I have set up a simple demo at http://ubercart.lwdev.net/products?page=1 - the product displayed is out of stock, but if you add it to your cart you are redirected to /products, which displays a different product. No standard modules can fail on this hook, so uc_multi_stock is installed which triggers the bug.

Modifying uc_cart_add_item() to return uc_referer_uri() on failure appears to fix this problem, a simple patch is attached.

CommentFileSizeAuthor
#1 uc_cart.add_to_cart.redirect.patch5.04 KBcha0s
hook_add_to_cart_redirect.patch381 byteslongwave

Comments

cha0s’s picture

cha0s commented
StatusFileSize
new uc_cart.add_to_cart.redirect.patch5.04 KB

I'm wondering why redirects upon failed adds only take place when the result is 'silent'. Should the cart redirect regardless?

I'm submitting what I feel might be a better approach, with a couple of other changes in the vicinity.

rszrama’s picture

rszrama commented

Hmm... tbh, I feel like the patch in #1 may be overreaching, esp. for this particular issue. Any major restructuring I wouldn't want to do in 1.x, and at this point I'm not sure I really want to change the way this function is working (just thinking of module dependencies, internal and external, on the current functionality). It might be helpful to note that a redirect will still happen if the result isn't silent, it's just not working properly - I think that's the isolated fix in the OP.

cha0s’s picture

cha0s commented

Well, it's not really a restructuring, just a cleanup and fully implementing the redirect the OP suggested, as opposed to only using the referrer_uri(), and only if $check_redirect is TRUE. Indentation may look like the patch is changing much more than it actually is, just changing the flow to be more error-tolerant.

rszrama’s picture

rszrama commented

Alrighty, I'll apply it and try to review if I get a minute... still hesitant to introduce this sort of change on 1.x, tho.

cha0s’s picture

cha0s commented

Any new thoughts on this one? We should mark it as postponed otherwise...

rszrama’s picture

rszrama commented
Version: 5.x-1.6 » 5.x-1.7

Sorry, will try to review again. Dunno what happened to my last review. :-/

cha0s’s picture

cha0s commented
Status: Needs review » Postponed
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 5.x-1.7 » 6.x-2.x-dev
Status: Postponed » Needs review

Bumping again as this is still an issue for me in both 5.x-1.x and 6.x-2.x, on my own sites I am still applying this patch against both branches. The patch in the original post still applies to 6.x-2.x with some fuzz. I think the rewrite in #1 is not necessarily needed but I would still like to see the original issue addressed!

cha0s’s picture

cha0s commented

One line takes a year to review. DC expected in early 2016...

joachim’s picture

joachim commented

Related issue: #776652: uc_cart_add_item() gives no indication of success or failure.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Fixed

Committed the patch from #0 to 6.x and 7.x. I think this might have been the first patch I submitted to Ubercart!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.