This is a consequence of #1210194: Sales tax not shown in order total preview on initial checkout. That modification moved calculation of the tax-exclusive subtotal line-item into a line-item 'display' call (instead of a 'load' call). I'm assuming this was intended to make sure that all other line-items had been loaded before calculating this one. However, as a result, this line item is no longer alterable via hook_line_item_alter(). This breaks compatibility with uc_coupon, which had to alter the pretax subtotal when dealing with tax-inclusive prices (for reasons too complicated to go into here).

This patch (which I'll attach as soon as I get an issue number) is a slight modification to the way uc_line_item_tax_subtotal() calculates its value. Instead of summing all the line items which aren't taxes, it sums all the taxes, and then subtracts that amount from the result of uc_order_get_total(). The order total as calculated by that function IS alterable (in fact, uc_coupon is already altering it) - so this resolves the problem as far as uc_coupon is concerned.

Marking this as "major" because uc_coupon is currently the only discount system ported to D7, and it would be a shame for a new release of ubercart to break it.

#1 1281346-tax-subtotal.patch1.33 KBwodenx


new1.33 KB

Here's the patch.

What do you think of my proposal in #1153086: Inclusive tax not visible in cart block or checkout pane? A uc_product_adjust() hook would hopefully let uc_coupon do its work earlier in the product price calculation process and avoid this late recalculation of subtotals, at the expense of changing the API.

I like the idea - but it wouldn't help in this case - uc_coupon doesn't need to alter the product prices - it needs to alter its own value as it contributes to the total when there are products with tax-inclusive prices. That's because we want the face value of the coupon to apply to the final total - and some of this is applied through the tax adjustment.

I did think of an alternative way to do this, but it would require a different change to Ubercart core.

The goal is to have the coupon line-item display the face value of the coupon, but have the actual value of the coupon be such that the net discount after taxes should also be the face value. This means that in calculating the order subtotals, the coupon has to count for less than its face value (because a portion of its face value goes to reduce the tax line-items).

This could be accomplished if it were possible to invoke the 'display' callback for a line-item even if it were not marked "display_only". I'm not sure I see the rationale in the current setup - it makes more sense to me to have the "display_only" marker indicate that only the display callback will be invoked (i.e. not the load callback) - and that other line-items should have both invoked - similar to the way cart items work. Thoughts about this? Thanks.

In #1737442 I identified a scenario where uc_line_item_tax_subtotal() tries to load parse the second argument as an object regardless of how the function is called, and as you mentioned here, the API seems to have been changed so that this function's arguments changed but it hasn't been updated accordingly. Basically the second argument for uc_line_item_tax_subtotal() and uc_line_item_tax() shouldn't be $order, it should be $arg1 and then handled differently depending on the contents of $op.

FYI I discovered this by having error_reporting set to E_ALL and adding this to my settings.php file:

['error_level'] = 2;