Form functions are called at multiple places in the form lifecycle (create through to validation/submit), have the function invoke the hook_order function is just plan wrong.

// In the file: uc_paypal.module

function uc_paypal_wps_form($form_state, $order) {

  ...

  foreach (module_implements('order') as $module) {
    $result = module_invoke($module, 'order', 'submit', $order, NULL);
    $msg_type = 'status';
    if ($result[0]['pass'] === FALSE) {
      $error = TRUE;
      $msg_type = 'error';
    }
    if (!empty($result[0]['message'])) {
      drupal_set_message($result[0]['message'], $msg_type);
    }
    if ($error) {
      $_SESSION['do_review'] = TRUE;
      drupal_goto('cart/checkout/review');
    }
  }

  return $form;
}

The second problem is that if there is if an error occurs then that code will go back to the 'cart/checkout/review' page, but we have already called hook_order with submit, meaning you are going to then invoke that hook again. Please remove that code in that foreach loop above or put it somewhere where its not going to invoked multiple times.

I did try to warn against this addition here: #700270: Remove hook_uc_order('submit') as it is never called for redirected payment methods

CommentFileSizeAuthor
#9 898050.paypal_wps_snafu.patch797 bytesrszrama

Comments

geerlingguy’s picture

geerlingguy commented

I can confirm that this code is causing problems on my site:

  • Firstly, the drupal_goto() sends me to the review page, but gets caught in an infinite redirect, which errors out the browser. Changing the goto() value to cart/checkout at least allows me to see the error which is thrown.
  • Second, removing this block of code allows uc_recurring to work correctly. So, for the time being, I'm going to run a patched version of Ubercart, removing the code from #700270: Remove hook_uc_order('submit') as it is never called for redirected payment methods, on archstl.org.
ezra-g’s picture

ezra-g commented
Title: module_invoke in a form function is wrong » Avoid infinite redirect errors with uc_recuring.

If you want to make changes to Ubercart core, please suggest a patch for others to review. Otherwise, working on a fix at #781010: Patch to Ubercart might break recurring Paypal WPS seems more likely to actually get committed somewhere.

Changing the title of this issue to reflect the problem that's being caused here. Please see Island Usurper's thoughts on using a form function this way at http://drupal.org/node/700270#comment-2891624 .

univate’s picture

univate commented
Title: Avoid infinite redirect errors with uc_recuring. » hook_order('submit', ...) invoked by paypal module before order actually is submitted
Priority: Normal » Major

This is not just a uc_recurring problem - its just shown itself in a big way in uc_recurring, because uc_recurring is using the hook_order('submit', ...) to setup recurring fees when an order gets submitted.

The problem is both a error in logic, since the hook being invoked before it actually should be, the order hasn't been submitted yet when this gets invoked. The other problem I mentioned above is that the purpose of a form functions is to generate an array to build a form. You have to expect that a form function will be called multiple times by drupal in the forms lifecycle, meaning this code could get called multiple times. Fortunately there is 'yet' another hack in paypal that prevents this from happening - assuming you are not trying to use this form elsewhere yourself?

I am not apposed to having this hook invoked, I just think we should be able to assume in uc_recurring (and other modules) that when hook_order('submit', ...) is invoked the user has actually submitted the order.

joyseeker’s picture

joyseeker commented

I'm not on your level of coding -- I'm just a poor Drupal user that wants to get my Recurring Payment working.

Is it safe to take out the foreach loop above to have the recurring payment work?

I also checked out the #781010 thread linked to above, but it seems to be talking about the dev version and not the current stable 2.4 version.

univate’s picture

univate commented

@joyseeker, if you use ubercart 2.2 with uc_recurring alpha4 (or current dev) recurring payments using Paypal WPS should work - although what will happen in the future I'm not sure.

Taking out that foreach look will essentially take that file back to how it looked in ubercart 2.2 - so that would also potentially work.

joyseeker’s picture

joyseeker commented

thanks

joyseeker’s picture

joyseeker commented

univate,

Again I have one product and one recurring product. When I take out the code, the review order shows both the product and the recurring product, but at Paypal there's only the recurring product being charged, not both.

torgospizza’s picture

torgospizza
he/him
Primary language English
Location Portland, OR
commented

#7: I think that is a separate issue. I'm unable to reproduce it.

Back to the issue at hand. Speaking with Ryan, we both agree that this is not a UC Recurring issue but is an inherent flaw with trying to call hook_order('submit') when a PayPal order is submitted. The only option that seems like a good workaround, IMO, is to require any 3rd party modules, like the one that was the original requirement for the patch at #700270: Remove hook_uc_order('submit') as it is never called for redirected payment methods, to not check for errors at the submit op. I'm not sure the best alternative, perhaps 'save', but either way since things need to happen at 'submit' (such as the uc_recurring handlers) this is a fatal flaw that appears, by asking around, to potentially break more things than it fixes.

I think for the UC Recurring issue we should revert the patch at #700270: Remove hook_uc_order('submit') as it is never called for redirected payment methods and attempt to refactor the portion of the module that invokes the recurring fee handler to not rely on the hook_order('submit') op. This is going to be my main focus - if anyone has better suggestions, please chime in. We need to get this working and I don't feel like having to re-patch UC core every time there is an update.

rszrama’s picture

rszrama commented
Status: Reviewed & tested by the community » Active
StatusFileSize
new 898050.paypal_wps_snafu.patch797 bytes

Chris is exactly right - this sort of logic does not belong in a form builder function. Validating orders and performing redirects is never a part of constructing a form, so form builder functions should never be doing that. Someone might want to use this form to collect payment outside of the normal checkout context and now couldn't. If a redirect needs to happen, it should happen earlier (i.e. a wrapper page callback function for the form), and a surface level examination of the patch indicates that this form being built on cart/checkout/review is going to cause an infinite loop when there's an error. Basically, #700270: Remove hook_uc_order('submit') as it is never called for redirected payment methods should never have been committed, and here's why...

We all know hook_order() has problems, and one of its biggest is the submit $op. That $op is there to give modules a chance to react to an order when the checkout review form is completed prior to redirection to the checkout completion page. Modules can say "Wait a minute! Something happened that prevents this order from being completed!" and keep the order on the review page with a tidy error message. The only core module that does this is the Credit Card module when it tries to process credit cards during checkout. If a charge fails, it keeps you on the review page and tells you to review your data and try again. Other modules can use this hook and $op as well if they need to actually halt the checkout completion process for some reason, but they should be aware that they're playing with fire - because the Credit Card module is using this the way it does, it means a card could have already been charged once, your module made checkout fail, and now the customer could be charged again... except if their CC data is now truncated, they'll try again, get a failure message for some reason on the CC data they thought they entered and have to go back to the checkout page and start over.

In other words - this $op is broken. What is it really trying to do? Well, it's a validate handler and submit handler wrapped into one for the checkout review form. That's part of the problem... it's a validate handler that only fails when some other action falls through based on the order details, but because you can have multiple implementations of the hook / $op, you can never tell if some other module has already done something that it's now in danger of repeating. You can't go undo those actions after the fact. The CC module fails here.

So then you think, well, if this is a validate / submit thing for the review form, why not add #validate and #submit handlers to the review form array? The answer as the original issue located tangentially is that the $op has to accommodate redirected payment methods. What the patch did, then, is say hey, this isn't being called for these orders... and since this $op is normally called in the context of the review order form, why don't I just call it in advance before heading off to payment? The problem is you're going to halt someone from ever getting to the point of submitting payment and trigger other options that might depend on payment having already been attempted or expect to happen concurrently. (The original hook doesn't do this but rather would even let modules after payment send the customer back to review.) Furthermore, the purpose of this $op isn't to halt someone from attempting payment - it's to keep someone from thinking their order is complete when it isn't.

Changing the $op to appear here for PayPal WPS doesn't make sense given the above, especially because it doesn't solve the same issue for any other redirected payment method. In fact, hook_order($op = 'submit') now works one way for PayPal WPS and a different way for other redirected payment services, and a third way for every other payment method. It would be much better for there to be a single function every redirected payment method can call after payment has been submitted, and even better if modules using this hook would find some other solution unless the module really needs to prevent order completion for some reason. The way UC Recurring 1.x (and perhaps still 2.x) deals with this is it gives you the option of still allowing checkout to complete but displaying an error message indicating some further action still needs to be taken on the order. This is the larger "how do we fix it" question that wasn't even addressed in the other issue because of PayPal WPS being the red herring. (Kinda makes me wonder if PayPal EC is still broken, too.)

I see Chris has brought up the issue of a form builder function being the wrong place for this kind of logic, and in my mind that's the only reason required for this patch to be reverted. It's even more pressing given the infinite redirect mentioned above. Not having a better solution isn't a good enough reason to commit anyways and break the form / other modules.

So, do I have a solution? Not really... I don't have a test environment to do this development at the moment. I had to totally change the way redirected payment services work with the Commerce checkout form to avoid these kinds of problems, and you can bet there's nothing like hook_order($op = 'submit') in there. : P

My strong suggestion is there doesn't need to be a fix - not in Ubercart 2.x. We're talking a major reconsideration of that hook_order() $op and the core module that uses it (CC) and then a major reconsideration of checkout in general to accommodate redirected payment services a lot better. It would be much easier for modules using this hook to operate elsewhere (doubly so since this behavior is still inconsistent for other redirected payment methods). If you want to press for a solution, then it needs to be generic enough to accommodate every redirected payment method (not just PayPal WPS) and shouldn't junk up any form builder functions to get it done.

That said, I will attach a patch for review, as it's the first step toward any solution. Because a full solution would be so long in coming, I also strongly suggest you commit this patch in the meantime and then find a robust fix.

torgospizza’s picture

torgospizza
he/him
Primary language English
Location Portland, OR
commented

What Ryan Said+++

rszrama’s picture

rszrama commented

Hmm, and I should add that I'm happy to help particular modules find a workable solution instead of relying on hook_order($op = 'submit'). I caused this mess in the first place after all...

(Also, so I don't appear to be sniping, I also have a vested interest in this situation being resolved as I just implemented recurring for a recent client that's still on UC 2.2 who is simply waiting for their recurring stuff to deploy to update to 2.4. i.e. I'm not being stodgy... I really need this resolved, too.)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Active » Needs review

Marking as "needs review", since there's a patch in here.

And holy good heavens, that's the most detailed and informative reply I've read on any issue ever in quite some time. :) NICE!

elliotttf’s picture

elliotttf commented

Subscribing. Thanks for the patch, uc_recurring was definitely completely fubar without it.

elliotttf’s picture

elliotttf commented
Status: Needs review » Reviewed & tested by the community

And I'm not sure if this will be the ultimate solution in this issue, but I can confirm that ubercart with uc_recurring behaves much better with the patch applied.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Agreeing with Ryan that hook_order really needs to be fixed, UC2.x is not the right place to do it but please consider this for UC3.x. The abuse of hook_order op 'submit' bit me when using uc_multi_stock, see #919744: Module weight issue when using uc_credit - cards were getting charged by uc_credit, then uc_multi_stock reported items were out of stock so the customer didn't realise they had been charged.

torgospizza’s picture

torgospizza
he/him
Primary language English
Location Portland, OR
commented

@longwave: I didn't realize it affected things that way too. That's a pretty insane flaw to introduce.

I think the eventual solution will be a rewrite of the order API, plain and simple.

Island Usurper’s picture

Island Usurper commented
Status: Active » Fixed

OK. #700270 has been reverted. Chalk it up as something I wish I hadn't done.

torgospizza’s picture

torgospizza
he/him
Primary language English
Location Portland, OR
commented

Thanks, Lyle!

geerlingguy’s picture

geerlingguy commented

Thank you!

pixelsweatshop’s picture

pixelsweatshop commented

So I am getting this error now. How do I roll this back now?

rszrama’s picture

rszrama commented

heh, like I said above, we should really chalk it up to something I wish I hadn't done. Or at least accommodating redirected payment gateways is a problem I wish I'd had in mind when writing the checkout system back then.

torgospizza’s picture

torgospizza
he/him
Primary language English
Location Portland, OR
commented

#20 (nigel.waters): You can apply the patch in #9. If you don't know how to do that, go here: http://drupal.org/patch/apply

Status: Fixed » Closed (fixed)

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