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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cha0s’s picture

This got postponed last time I tried...

rfay’s picture

Priority: Normal » Critical

This raises its ugly head again in #611044: Fatal error: Unsupported operand types in .../uc_order.module on line 1459. Changing to critical.

TR’s picture

Version: 6.x-2.0-rc3 » 6.x-2.x-dev

I 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.

rfay’s picture

All 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

TR’s picture

Priority: Critical » Normal
Status: Active » Postponed

Well, OK, then let's mark it as non-critical and postpone it until later.

cha0s’s picture

Was that a death knell I just heard?

TR’s picture

Nah, 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.

Island Usurper’s picture

Title: Ubercart must not use hook_add_to_cart() - Change to hook_ubercart_add_to_cart() (and namespace any other hooks) » Ubercart must namespace its hooks
Version: 6.x-2.x-dev » 7.x-3.x-dev
Priority: Normal » Critical
Status: Postponed » Active

OK, 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.

Island Usurper’s picture

Status: Active » Needs review
FileSize
81.73 KB

I 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.

BenK’s picture

Subscribing....

Island Usurper’s picture

FileSize
86.69 KB

Re-roll of the patch, since it no longer applies cleanly.

TR’s picture

I 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:

# patch -p0 < 510382_normalize_hooks_0.patch
patching file payment/uc_2checkout/uc_2checkout.module
patching file payment/uc_authorizenet/uc_authorizenet.module
patching file payment/uc_credit/test_gateway.module
patching file payment/uc_credit/uc_credit.admin.inc
Reversed (or previously applied) patch detected!  Assume -R? [n]
Apply anyway? [n]
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file payment/uc_credit/uc_credit.admin.inc.rej
patching file payment/uc_credit/uc_credit.module
Hunk #3 FAILED at 315.
1 out of 3 hunks FAILED -- saving rejects to file payment/uc_credit/uc_credit.module.rej
patching file payment/uc_cybersource/uc_cybersource.module
patching file payment/uc_google_checkout/uc_google_checkout.module
patching file payment/uc_payment/uc_payment.admin.inc
Reversed (or previously applied) patch detected!  Assume -R? [n]
Apply anyway? [n]
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file payment/uc_payment/uc_payment.admin.inc.rej
patching file payment/uc_payment/uc_payment.api.php
patching file payment/uc_payment/uc_payment.module
Hunk #5 FAILED at 500.
1 out of 7 hunks FAILED -- saving rejects to file payment/uc_payment/uc_payment.module.rej
patching file payment/uc_payment_pack/uc_payment_pack.module
patching file payment/uc_paypal/uc_paypal.module
patching file payment/uc_paypal/uc_paypal.pages.inc
patching file shipping/uc_flatrate/uc_flatrate.module
patching file shipping/uc_quote/uc_quote.admin.inc
patching file shipping/uc_quote/uc_quote.api.php
patching file shipping/uc_quote/uc_quote.module
patching file shipping/uc_quote/uc_quote.pages.inc
patching file shipping/uc_shipping/uc_shipping.admin.inc
patching file shipping/uc_shipping/uc_shipping.api.php
patching file shipping/uc_shipping/uc_shipping.module
patching file shipping/uc_ups/uc_ups.module
patching file shipping/uc_usps/uc_usps.module
patching file shipping/uc_weightquote/uc_weightquote.module
patching file uc_attribute/uc_attribute.module
patching file uc_cart/uc_cart.api.php
patching file uc_cart/uc_cart.module
patching file uc_cart/uc_cart.pages.inc
patching file uc_cart/uc_cart_checkout_pane.inc
patching file uc_cart_links/uc_cart_links.module
patching file uc_cart_links/uc_cart_links.pages.inc
patching file uc_catalog/uc_catalog.module
patching file uc_file/uc_file.admin.inc
patching file uc_file/uc_file.api.php
patching file uc_file/uc_file.module
patching file uc_file/uc_file.pages.inc
patching file uc_order/uc_order.admin.inc
patching file uc_order/uc_order.api.php
patching file uc_order/uc_order.line_item.inc
patching file uc_order/uc_order.module
patching file uc_order/uc_order.order_pane.inc
patching file uc_product/uc_product.admin.inc
patching file uc_product/uc_product.api.php
patching file uc_product/uc_product.module
patching file uc_product_kit/uc_product_kit.module
patching file uc_roles/uc_roles.module
patching file uc_store/uc_store.admin.inc
patching file uc_store/uc_store.api.php
patching file uc_taxes/uc_taxes.api.php
patching file uc_taxes/uc_taxes.module
Island Usurper’s picture

Status: Needs review » Needs work

Ah, 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.

Island Usurper’s picture

Status: Needs work » Needs review
FileSize
84.57 KB

Rerolled.

TR’s picture

Status: Needs review » Reviewed & tested by the community

Patch works fine. Let's get it committed!

Island Usurper’s picture

Status: Reviewed & tested by the community » Fixed

Wahoo! Committed to CVS.

I really need to get CHANGELOG.txt started, eh?

rfay’s picture

Congratulations and thanks!

Status: Fixed » Closed (fixed)

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