I just played around with commerce and I was evil :D
While playing around I deleted one of the referenced line items of an order. And as expected I got a "WSOD" because the entity metadata wrapper failed.
I know, under normal conditions this shouldn't happen, but for the sake of ultra defensive coding and usability (avoiding a WSOD is great usability ;) ) I thought I could add some "self healing".
I've added a check if a line item really exists in commerce_cart_order_refresh() before cloning it. If this fails, the reference is removed (To properly work this needs following entity fix #1423174: Use array_key_exists() instead isset() in offsetExists()). That way the consistency of the references is restored.
Overhead of the whole thing should be insignificant.

Btw. the other two changes in the patch are only replaced tabs.

CommentFileSizeAuthor
#5 commerce-add-some-self-healing-to-order-calculate-total-1423204-5.patch1.43 KBjsacksick
#4 commerce-add-some-self-healing-to-order-calculate-total-1423204-4.patch871 bytesjsacksick
commerce-add-some-self-healing-to-order-refresh.patch1.6 KBdas-peter
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rszrama’s picture

rszrama CreditAttribution: rszrama commented
Status: Needs review » Fixed

Simple enough! Committed.

Status: Fixed » Closed (fixed)

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

aidanlis’s picture

aidanlis CreditAttribution: aidanlis commented

This doesn't just happen in the hypothetical - if you get to the "redirect to external payment" step, then use the browsers back button to click around, then add another item to your cart you'll WSOD. Thanks for the patch!

jsacksick’s picture

jsacksick CreditAttribution: jsacksick commented
Status: Closed (fixed) » Needs review
FileSize
commerce-add-some-self-healing-to-order-calculate-total-1423204-4.patch871 bytes

I was having an issue with an order discount causing a fatal error and the similar fix applied to commerce_order_calculate_total() somehow did the trick.
Related issues : #1905234: Commerce Discount cause an error when reusing shipping address for billing and #1408464: EntityMetadataWrapperException

jsacksick’s picture

jsacksick CreditAttribution: jsacksick commented
FileSize
commerce-add-some-self-healing-to-order-calculate-total-1423204-5.patch1.43 KB

Same patch with the use of the offsetUnset() method

jwjoshuawalker’s picture

jwjoshuawalker CreditAttribution: jwjoshuawalker commented
Status: Needs review » Fixed

Yes, yes, yes.
jsacksick++

I don't think this issue will get attention since the original was committed. I created a new post here and attached your patch, marking as RTBC:

#1993938: Add some self healing for order line item references (again)

rszrama’s picture

rszrama CreditAttribution: rszrama commented
Priority: Minor » Normal
Status: Fixed » Needs review

Any open issue will get addressed. No need to create a duplicate.

jwjoshuawalker’s picture

jwjoshuawalker CreditAttribution: jwjoshuawalker commented
Status: Needs review » Reviewed & tested by the community

@rszrama

Sorry Ryan. Thanks for the attention on this one.

rockaholiciam’s picture

rockaholiciam CreditAttribution: rockaholiciam commented

encountered the problem where after updating cart the shipping checkbox would start throwing the commerce total exception and the patches provided fixed it. thanks.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Reviewed & tested by the community » Needs review

@jsacksick re #5 Why do you need to use offsetUnset() instead of unset()?

I may be dense, but isn't the purpose of that method to allow the magic __unset() to call it? Or is there some bug or performance gain from calling this directly?

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman commented

Patch fixed my issue that was occurring when Commerce Discount discount active, explained in #1905234: Commerce Discount cause an error when reusing shipping address for billing

bojanz’s picture

bojanz CreditAttribution: bojanz commented
Status: Needs review » Reviewed & tested by the community

The wrapper currently doesn't implement the magic __unset method (even though it does implement __isset).

rszrama’s picture

rszrama CreditAttribution: rszrama commented
Status: Reviewed & tested by the community » Fixed

Committed, thanks guys.

Status: Fixed » Closed (fixed)

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