Remove hook_uc_order('submit') as it is never called for redirected payment methods

subscribing

at first glance this patch scratches my particular itch, the issue from node #540298, mentioned by Ezra above.

i'm not sure of the broader implications for Ubercart, but thanks for taking this issue into consideration.

Thanks, this patch works for me. A real life saver!

subscribing

I am having an issue where the user pays by credit card (using paypal WPS gateway) and receives points successfully BUT the admin has to login to the paypal account to press ACCEPT for the payment manually.

I don't want to give away points until the payment is confirmed with paypal.
Does this patch help fix this issue? Can anyone help?

subscribing

subscribing

Is there a way to implement this for WPP Direct Payment and Express Checkout?

Is this patch needed in the 1.5.2.24 version of the paypal module?

Thanks for the patch, but unfortunately it didn't work for me. I'm using UC signup with Website Payments Standard. I can successfully sign up users when not going through Ubercart. But even though the payments get marked as Complete in Ubercart, I don't get any signups after users have completed checkout. I have the 'Change temporary signups to paid signups upon checkout when a payment clears the order balance.' predicate enabled.

Did some more testing and got it to work with the patch (doesn't without it). The issue was that I had 'Attendee Signup Information' disabled in the Checkout panes. When I enabled it, the signup got logged.

So, make sure you have the 'Attendee Signup Information' pane enabled.

My clients don't want that area to show up, since they're likely to have an older audience. They think it might confuse them and make them think the list of classes they're signed up for already are items in their cart. I'll can use CSS to hide that section, since I don't want it to show up on the checkout. I'll leave it up to you, ezra whether you think that's an issue that should be opened for UC Signup.

I really don't like this change...

This is now likely to break uc_recurring Paypal WPS support - we had to put in various work-arounds/hacks to make uc_recurring work with Paypal in the first place largely due to this issue of hook_order('submit', ...) not being called for the Paypal WPS gateways.

The real problem here is that the implementation of the Paypal gateway is a hack to start with. Adding more hacks into that gateway implementation just makes the implementation even worse and will likely cause more problems then it fixes. You just have to look at how it uses a form_alter to change the entire behavior of the checkout form as an example of why the gateway implementation needs a re-think. That hack is the reason why hook_order('submit',...) is not called in the first place.

I think the solution to this problem needs to be solved at the payment method level, where it should actually support a gateways redirecting off to another site like Paypal wants to do while still handling all the hooks and other functionality that needs to happen on checkout - this way 3rd party module developers can be confident that all gateways are going to work the same and not require lots of dirty hacks like this patch implements.

The real problem here is that the implementation of the Paypal gateway is a hack to start with. Adding more hacks into that gateway implementation just makes the implementation even worse and will likely cause more problems then it fixes. You just have to look at how it uses a form_alter to change the entire behavior of the checkout form as an example of why the gateway implementation needs a re-think.

I tend to agree with univate. However, it seems there is a shortage of people willing to work on improving the uc_paypal module, so a number of these PayPal related issues are languishing.

This change actually makes Paypal WPS *more* API compliant by invoking the hook at almost the same stage of checkout

Not really, as for every other gateway hook_order('submit', ...) happens independently of the gateway meaning you can as an example set the weight of your module to have your code occur before or after the payment module implementation of this hook. For paypal it will now always occur before payment - meaning the behavior of that hook could be different for paypal payment.

The other problem I just noticed while reviewing this code, is putting this code in a form function is probably not a good idea, form functions are designed to return an array of form elements. Form functions are not designed for functionality and may be called by drupal in multiple places, for example they get called again when the form is submitted - I don't think thats happening here because the submit function is replaced on that form. But it does highlight my point that this module is a hack upon hack.

This change is definitely going to effect uc_recurring - although I am yet to work out how bad or what it will require to work around.

univate, I agree that putting that code in the form building function isn't a good place for it, but I couldn't think of a *better* place. Using JavaScript to redirect to the PayPal page after the form is submitted feels like a hack to me, and I didn't want to take the time to figure out how to implement that when Ezra brought this issue to me.

I think it would be good to hold off on a 2.3 release until uc_recurring can handle this change. However, we shouldn't wait too long for this because there are a lot of bug fixes that have gone into the dev branch.

FYI: Opened a new issue that's a consequence of this change at #841862: Revisiting the checkout review form multiple times creates a new order each time.

Judging by my experience, this patch definitely breaks uc_recurring - see #781010: Patch to Ubercart might break recurring Paypal WPS . Is there no other way to tie into hook_order? The big problem, which univate points out, is that the form should, in essence, be submitted to PayPal WPS... ugh.

The problem is this patch is plain wrong as it breaks how form functions are normally expected to work.

I have created a new issue to explore how we can remove the code this patch introduced into a form function and put it somewhere where is doesn't cause more problems then it fixes:
#898050: hook_order('submit', ...) invoked by paypal module before order actually is submitted

Also this was never a bug it was a new feature as it change the functionality of the paypal gateway and should have been more closely examined before rushing to commit it.

Indeed, it's a problem. I was having a problem with redirect loops on recurring orders in Ubercart 2.4, so I reverted the patch by commenting out the block of code that this patch adds. Now not having the problem anymore.

Was it a security risk to do this?

Changing the status to indicate that this patch completely breaks PayPal recurring payments.

I really think this patch should be reverted, but I went ahead and used the other issue to state the case:

http://drupal.org/node/898050#comment-3569890

This issue surely affects more than PayPal WPS, as many other payment modules also redirect offsite by replacing the "submit order" button on the review screen - 2Checkout does this, along with a number of contributed modules I've seen.

Other than uc_credit using it to process payments, what modules implement hook_order op 'submit' and why? Maybe this op should be deprecated entirely, or moved so it fires when moving from checkout to submission? What do modules want to do in this hook that can't be achieved with a new submit handler on the checkout form, Conditional Actions, hook_uc_payment_entered or hook_uc_checkout_complete?

I can't see a way of guaranteeing this hook is only ever fired once immediately before payment occurs, as the user could use the back button, cancel payment or close their browser when they get to (e.g.) PayPal. You also can't easily fire this after payment occurs with PayPal at least - the user can close their browser after making a PayPal payment and not return to the site, and IPN is not 100% reliable - or when IPNs do arrive, the user's session context isn't available to use. It sounds like whatever the solution is to this should also solve #644538: Duplicate order notification e-mail, and duplicate stock decrement at the same time.

The 'order-submit' op for payment method callbacks gets called, though in Ubercart, that only seems to apply to CODs. uc_quote uses it to unset() some session data, so that could probably be moved to hook_uc_checkout_complete().

Perhaps we could redirect folks to an intermediate page once the order has been submitted, before having them post off-site.

I thought the reason for hijacking the submit button on the checkout form was that Paypal accepted the variables via POST, which makes a redirect unsuitable?

NB: #946580: PXAccess should invoke hook_order case submit (backport from #700270) is a related issue with UC PaymentExpress (PXAccess). That issue will be postponed until a solution is found here and we can apply the same fix there (unless someone engages it separately that queue).

I wonder if we could move this hook invocation to uc_cart_complete_sale() as part of #1192018: Duplicate order notification e-mail, and duplicate stock decrement as that patch should guarantee it is called only once after each successful order. There seems no other way to handle the case where the PayPal IPN (or another offsite payment method runs a callback) is invoked but the user never returns to the site.

And should we remove this entirely in 7.x-3.x?

Alternatively I proposed a patch #781720: Refactor the Paypal WPS IPN code which adds a hook to expose the IPN message to other modules which would be an easy way to do stuff on various IPN events.

hook_uc_checkout_complete() should be much more reliable now #1192018: Duplicate order notification e-mail, and duplicate stock decrement has been committed. I think this is a suitable replacement for hook_order('submit') and is guaranteed to be called exactly once when either the IPN is received or the user returns from PayPal.

As there has been no activity here for almost two years, it's unlikely this is going to change in current stable releases, especially now hook_uc_checkout_complete() is more reliable and #781720: Refactor the Paypal WPS IPN code would go even further for the PayPal case.

Instead, let's write off hook_uc_order('submit') as a bad idea and consider removing it in a future version.

Ideally in 8.x-4.x we can remove hook_uc_order() entirely and rely on the existing Entity API hooks instead.

See #2293939: Replace the payment API with Payment 8.x-2.x.

Will be fixed in #2647840: Remove hook_uc_order('submit')