The accepted behaviors for contrib modules is that they have to namespace their hooks. Thus, ubercart should be using hook_ubercart_add_to_cart as a hook, not hook_add_to_cart.
There are other carts in Drupal... There are other "add to cart" modules. There are other functions called add_to_cart(). It's important to change all hooks to include the ubercart namespace.
This problem was responsible for #494304: Add to Ubercart Cart creates Amazon Cart error messages in Amazon Store module. Amazon store had an innocent amazon_store_add_to_cart() function, which unfortunately got called when Ubercart did an add to cart.
Thanks,
-Randy
Comment | File | Size | Author |
---|---|---|---|
#14 | 510382_normalize_hooks.patch | 84.57 KB | Island Usurper |
#11 | 510382_normalize_hooks.patch | 86.69 KB | Island Usurper |
#9 | 510382_normalize_hooks.patch | 81.73 KB | Island Usurper |
Comments
Comment #1
cha0s CreditAttribution: cha0s commentedThis got postponed last time I tried...
Comment #2
rfayThis raises its ugly head again in #611044: Fatal error: Unsupported operand types in .../uc_order.module on line 1459. Changing to critical.
Comment #3
TR CreditAttribution: TR commentedI think everyone agrees on this - the issue has been mentioned in dozens of other threads and I've not heard any objections. Can you roll a patch to make it happen?
I'm strongly in favor of hook_uc_add_to_cart() as the new name, as that is much more consistent with Ubercart naming conventions.
Comment #4
rfayAll of the hooks will be namespaced for the next version of ubercart/Drupal Commerce - this is in the vision/plan for D7, etc. There's no reason to do it piecemeal.
I should mention that it hasn't been done to date because of the number of affected contrib modules. But it will get done, especially since the 1.0 release broke on this issue.
-Randy
Comment #5
TR CreditAttribution: TR commentedWell, OK, then let's mark it as non-critical and postpone it until later.
Comment #6
cha0s CreditAttribution: cha0s commentedWas that a death knell I just heard?
Comment #7
TR CreditAttribution: TR commentedNah, that's just tinnitus. You need to turn your iPod down, dude.
But seriously, if even the OP doesn't care about the issue anymore I don't see any sense keeping it open. I'm certainly not going to do the work myself if no one wants it done. I don't have CVS privs so I can't personally fix anything, but I *can* attempt to reform the community process here by contributing and managing the issue queue instead of leaving it to rot. I get that you're disillusioned with the past dysfunctionality, but the constant sniping doesn't help those of us trying to change that. If you've given up on Ubercart, then let it go already, otherwise we could use some help investigating bug reports and reviewing patches.
Comment #8
Island Usurper CreditAttribution: Island Usurper commentedOK, since Ubercart is being ported to D7, it seems like a good time to fix this problem. It's a contrib-breaking API change, so if it's ever going to happen, the port is the time to do it. It would be naïve of us to expect all of the Ubercart contrib modules out there to upgrade their hook implementations during the 2.x branch, and it's not right for us to break people's websites without a major version change.
Comment #9
Island Usurper CreditAttribution: Island Usurper commentedI think I found them all, from the uses of module_invoke_all(), to drupal_alter, to the example hooks in the .api.php files. The patch also rearranges the examples so that they are still in alphabetical order, so it is a little bigger than expected.
Comment #10
BenK CreditAttribution: BenK commentedSubscribing....
Comment #11
Island Usurper CreditAttribution: Island Usurper commentedRe-roll of the patch, since it no longer applies cleanly.
Comment #12
TR CreditAttribution: TR commentedI had some problems applying the patch. First, I did a cvs update -C to ensure I had the current revisions of the code, then I ran the patch. Most of the changes were successful, but a few failed and a few looked like they were reversed:
Comment #13
Island Usurper CreditAttribution: Island Usurper commentedAh, I guess I committed that change for uc_payment_process(). There's no getting around that change since there's a new, undocumented hook_process() in D7. That needs its own issue in the Documentation queue.
Comment #14
Island Usurper CreditAttribution: Island Usurper commentedRerolled.
Comment #15
TR CreditAttribution: TR commentedPatch works fine. Let's get it committed!
Comment #16
Island Usurper CreditAttribution: Island Usurper commentedWahoo! Committed to CVS.
I really need to get CHANGELOG.txt started, eh?
Comment #17
rfayCongratulations and thanks!