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.
Comment | File | Size | Author |
---|---|---|---|
#8 | 603356_tax_implements_alter.patch | 1.11 KB | Island Usurper |
uc_taxes_module_weight_install.patch | 1.05 KB | hanoii | |
Comments
Comment #1
hanoiiComment #2
hanoiiBumping this issue, any comments/thoughts?
Comment #3
longwaveMakes 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.
Comment #4
hanoiiBumping 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.
Comment #5
longwaveBumping 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.
Comment #6
longwaveWould still like a review of this, see also #900338: Tax calculation and other module
Comment #7
longwaveCommitted to D6. Needs porting to D7, perhaps to use hook_module_implements_alter() instead.
Comment #8
Island Usurper CreditAttribution: Island Usurper commentedHere'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.
Comment #9
longwaveCommitted, with docblock instead of inline comments.