I was going to report this sometime ago, but then I though it wasn't really necessary, but I just found the hard way that it is.

Because uc_taxes process tax on hook_order(), the order of module's execution is important. If any other module adds a line item which should be taxed and works some logic in the same hook_order(), we should make sure the tax module (which may affect line items from other modules) is run last of all so all line items are properly taxed.

A good example is uc_shipping, although because of the modules' alphabetically ordering, uc_shipping is executed first.

I ran into this because I created a module that was incidentally run after uc_taxes, and then, the order line item added by this module was not properly taxed.

Although I can make my module's weight ligher than uc_taxes, I believe this belongs be on ubercart's core because it's likely that any other order line items from other modules may face this problem in the future.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hanoii’s picture

Title: uc_taxes module's weight should be set on install » uc_taxes module's weight should be set heavier on install
hanoii’s picture

Bumping this issue, any comments/thoughts?

longwave’s picture

Makes sense to me, I can see why this wouldn't be spotted under most conditions as hooks with the same weight are fired in alphabetical order, so uc_taxes comes last by default in most setups even with additional contrib modules that may implement hook_order. I can't see any reason why moving uc_taxes to run last would cause side effects with anything else.

hanoii’s picture

Bumping this issue once more, doing it only because I just went over some personal notes on my projects and this one was one issue to follow.

longwave’s picture

Bumping again, as I just ran into this when fixing uc_pma: http://drupal.org/node/841348#comment-3447498 - it makes sense to set uc_taxes' weight heavier, rather than a lighter weight for any line item module that does work in hook_order.

longwave’s picture

Would still like a review of this, see also #900338: Tax calculation and other module

longwave’s picture

Version: 6.x-2.x-dev » 7.x-3.x-dev
Status: Needs review » Patch (to be ported)

Committed to D6. Needs porting to D7, perhaps to use hook_module_implements_alter() instead.

Island Usurper’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.11 KB

Here's an implementation of hook_module_implements_alter(), which is basically the same as the example from system.api.php. Has no effect on the normal working of the module, which is good. I don't have a line item module for D7 that I can suborn so that it normally comes after uc_taxes, but I bet it works fine.

longwave’s picture

Status: Needs review » Fixed

Committed, with docblock instead of inline comments.

Status: Fixed » Closed (fixed)

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