hook_line_item_alter() is called twice for stored line items, once with $line as an object, once with $line as an array. Apparently, the latter is the correct one because it alters consistently all line items, not only the unstored ones.

Comments

damien tournoud’s picture

Status: Active » Needs review
StatusFileSize
new1.15 KB

Here is a patch.

Island Usurper’s picture

StatusFileSize
new2.27 KB

I think this was partly the result of a bad merge when the work on VAT went in, and partly a moment of insanity on my part when I thought it was a good idea to make line items objects, but only part of the time.

So, this is a good start, but we need to make sure that line items are always handled as arrays throughout the code. Especially important is when they are passed to uc_price() and conditional actions, because I know that uc_vat and uc_discount are relying on that. Fortunately, it looks like uc_vat is already forcing the line item to be an array, but uc_discount is already out of date, so I'll have to work on it for several reasons.

Does this patch take care of all the places where line items are loaded as objects?

rszrama’s picture

I put this up on the Livetest, and everything seems to work fine. All I did was test a checkout with shipping quotes and look at an order, though, so I don't know if it should have any more vigorous review or not.

This could break modules dependent on the existing line item data type, so it should definitely come with a mention in the 2.x upgrade docs in the developer's guide.

Island Usurper’s picture

hook_line_item_alter() is rather new, and line items have always been arrays before. Like I said earlier, for some reason I thought that they would be better as objects, but I didn't make that switch very well. I don't think there's any need to change the upgrade documentation regarding this. However, we do need to make sure that the hook_line_item_alter() documentation gets updated. I just noticed it still says "object".

Island Usurper’s picture

Status: Needs review » Fixed

OK, fixed the hook documentation and committed.

Status: Fixed » Closed (fixed)

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