Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The {uc_taxed_product_types} is not updated when node type machine name of a product class changes. This could result in incorrect tax calculations.
This could be fixed with a hook_node_type_update() implementation.
Comments
Comment #1
Jordan_Fei CreditAttribution: Jordan_Fei commentedHi DanZ,
I tried to fix this bug as you described as the attached patch. Please review and test it.
Regards,
Jordan
Comment #2
DanZ CreditAttribution: DanZ commentedThanks for this. It's good to see some initiative from someone new.
Interesting approach, adding an additional hook to do this. I was expecting just a uc_tax_node_type_update() implementation. It's worth discussing.
What is the advantage of adding an Ubercart-specific hook over just using hook_node_type_update()? Can you see a lot of modules using this? The Drupal hook provides more information.
Actually, I might use it for the Ubercart Stamps.com module, since it does need to respond to product type machine name changes and doesn't care about anything else.
If you're going to add the hook, though, make sure it only responds to Ubercart product type and subtype changes. There's no point in having an Ubercart hook that gets invoked on events that have nothing to do with Ubercart. There's also no point in calling it if the old type and the new type are the same.
You need to have spaces after your commas to meet Drupal style standards. See http://drupal.org/node/1354.
Comment #3
Jordan_Fei CreditAttribution: Jordan_Fei commentedHi DanZ,
Before I thought uc_taxes module depends on ubercart core module uc_product, and {uc_taxed_product_types} should depend on product type in uc_product module(although product type
is just content type there), so I added a hook in uc_product product to decouple the uc_taxes with
node type.
But as you said, if there are other modules using the hook_node_type_update, your solution is more simple.
Therefore, I created two patches:
ubercart-taxed_product_type_udpate-1899458-s1.patch: My solution just do some coding format as you mentioned.
ubercart-taxed_product_type_udpate-1899458-s2.patch: Your simple solution.
It is up to you to use which one.
Regards,
Jordan
Comment #4
TR CreditAttribution: TR commentedHi Jordan_Fei,
Thanks for your contribution. It's looking good - hopefully we can get some testing from some users to confirm that this works without breaking anything. In the meantime, please take a look at http://drupal.org/coding-standards because there are some things in your patch that need to be changed to conform to Drupal coding standards.
Comment #5
longwaveI think the "s2" patch is simpler. I fixed up some more coding standards issues, and added a test (rearranging the other tax tests while I was at it).
Comment #7
longwaveComment #8
longwaveCommitted. @Jordan_Fei, thanks for fixing this!
Comment #9
DanZ CreditAttribution: DanZ commentedBoth of the new tests have:
Shouldn't the second be something like this?
...or does something informative show up in the text anyway? I still don't know much about tests....
Comment #10
longwaveI committed your suggestion. Generally, when debugging test failures you have to look at the source code of the test to figure out what's happening anyway, but improving the assertion messages can't hurt.
Comment #11
Jordan_Fei CreditAttribution: Jordan_Fei commentedhi TR and longwave,
I came back from vacation just now, so didn't reply to you. Thanks for your continuous work on this issue.
Regards,
Jordan
Comment #13
fcortez CreditAttribution: fcortez commentedComment #14
fcortez CreditAttribution: fcortez commented