Patch to Ubercart might break recurring Paypal WPS

I can confirm that the patch in #700270: Remove hook_uc_order('submit') as it is never called for redirected payment methods does indeed break uc_recurring. I am currently working down through the various function calls to see if I can get my head around what exactly is going wrong. But a very obvious point is this:

At the end of the loop in the patch which calls all the hook_order() modules there is a test to see if there was an error:

if ($error) {
$_SESSION['do_review'] = TRUE;
drupal_goto('cart/checkout/review');
}

This surely cannot be right? Since this patch invokes hook_order() for the order review page when using WPS, if any of the order hooks fail as the review page is being built then drupal_goto('cart/checkout/review') is called, putting the whole process into an infinite loop. I am actually wondering if this should read drupal_goto('cart/checkout'), to send the page back to the checkout page if the review page fails.

Subscribe.

subscribe...

Hopefully it helps to mention that this happened to me using 6.x-2.0-alpha4 also. If this issue existed in older version also, it may help shed some light. See my notes in another post, if this is of interest (http://drupal.org/node/779820#comment-3281626). Scroll down to "TO POTENTIALLY HELP OTHERS"...

Thanks,

The Brandon

puravida,

I noticed you awarded this project on freelancer? Has any progress been made?

There does appear to have been some progress but we have not been able to test it as yet. The developer has mentioned finding a couple of "bugs" with the uc_recurring paypal support and will be submitting some patches once he gets a more stable update.

So far, the developer has run a test order that appears to have set up separate multiple recurring items from a single checkout using the Paypal "account payment" method, so it is promising.

Once completed, the project we posted should offer the following upgrades to the uc_recurring support:

1. Allow for a Paypal "account payment" method that doesn't suffer from certain multiple recurring item limitations http://drupal.org/node/779820 *

2. Should avoid the issue in this current thread

3. Provide a better framework for updating/canceling individual pieces of recurring orders **

*Although, we are not sure yet how this will be implemented. If it is done with Paypal Express Checkout only, then it will be limited to 10 recurring items per order, according to the developer. If he is able to also integrate WPP (or WPS) recurring support fully for Paypal "account payments", then there would be no theoretical limit on multiple recurring items per order.

**This will be phase two of the project and we will also be sure to give those updates back to the community

Just wanted to cross-post with #805454: Make Paypal WPS work with uc_recurring_order, as that issue seems highly relevant here.

I'm going to try the -dev release today and try to make some progress on this issue.

[Edit: As I really didn't have time to test -dev right now, and am going to have to postpone work on that area of the website, I basically reverted the patch to ubercart core that was applied in #700270: Remove hook_uc_order('submit') as it is never called for redirected payment methods. I commented out lines 1009-1023 of uc_paypal.module (the part of function uc_paypal_wps_form() that allows other modules to throw errors during checkout).

Hopefully I'll find time soon to dive back in and get a real fix in place. I was tracking this bug internally here: Fix 'redirect loop' when confirming order details.]

Just to reiterate what Univate said in #805454: Make Paypal WPS work with uc_recurring_order, and to clarify my own thought process, this bug is in relation only to using PayPal WPS to rebill an entire Order, correct? I've been testing PayPal WPS with uc_recurring and it works great for us, but only for products instead of orders. (Since we're allowing users to buy other products with their subscriptions, duplicating an entire order is not a possibility for us.)

Just wanted to put this out there, as a reminder for myself and anyone else who might get confused with this issue.

Hey Univate,

Good point - No, we're still on 2.2. I saw some other issues that gave me pause so I haven't upgraded yet. Is the issue only with Ubercart 2.4?

Just FYI I have the Recurring Order module disabled, Ubercart 2.2 and the Hosted Recurring module enabled for PayPal WPS, and that has worked for me with no issue. If you'd like I could see about upgrading to 2.4 (still a little bit weary of it, but hopefully it'll go smoothly) and try to help with this issue, if it is in fact one that affects uc_recurring in general.

I can confirm that it affects both 2.3 and 2.4.

Also, when you upgrade to 2.3, you have to be careful to test first, because if you have any custom admin/customer invoice templates, you have to go through a lot of work to get them working properly again!

So to test for this error, one must submit the checkout form with something that would cause an error to fire? That seems to be what the OP mentions, that it's a patch within the error-checking condition.

If someone can list the steps needed to replicate this bug, that would be helpful. Thanks!

Steps to reproduce:

1. Enable both UC 2.4 and uc_recurring.
2. Add a new product, and add a 'Recurring' feature to the product (can also add attributes for this).
3. Order the product with this recurring feature.
4. Complete checkout. At the end of the checkout process, when you'd normally be sent to PayPal, you'll be sent to the cart, with an error message (or you'll be stuck in a redirect loop, if you didn't change the drupal_goto() path mentioned in Comment #1 above).

Ah, thanks. That seems strange - does changing the path as discussed in #1 seem to solve the issue? Does it have any side-effects with regard to other modules? Can anyone think of why this change would go into UC in the first place? Is there a reason why changing the path to remove the 'review' path would have an adverse effect on other gateways? I'm trying to wrap my head around this, and after reading #700270: Remove hook_uc_order('submit') as it is never called for redirected payment methods no one there has pointed out this part of the patch causing an issue.

If the best solution is to simply change that path, is it worthwhile to make this an Ubercart issue (not uc_recurring) and submit a patch that changes the path as described? Or is that only a band-aid solution, and a more complete solution should be worked on? Sorry for the questions, I'm coming into this late, but want to help :)

Changing the path corrects the redirect loop, but uc_recurring WPS integration is still broken. An error message is displayed and the user is returned to his cart. The form is never submitted to PayPal.

That's kind of what I was afraid of. Alright, I'll see if I can work on upgrading our UC install and taking a look at this. Has anyone done a diff on the Ubercart versions to see what could be breaking WPS integration with uc_recurring? The patch in the thread mentioned in #18 seems pretty small, and I don't think it would cause this by itself... there seems to be a piece missing here and I'm having a hard time finding it. Will try to dig into it this week and see if I can reach something conclusive.

EDIT: I read through that thread one more time, and I see the issue now, having to do with the patch indeed and the invocation of the 'submit' $op in hook_order. TR and Univate's suggestions seem to allude to a discussion of rewriting some of the Payment API. Seems to me that this is actually a larger undertaking than I had hoped... but I'm still going to take a look and see if I can fix this. We need this functionality pretty badly, and I'm willing to devote some time to it. Probably not a "rewrite things completely" approach but to see if I can at least start on a viable solution.

Suggestions and help greatly appreciated at this point, of course. Will post back with thoughts and progress as I get there.

#7 (puravida):

Do you have any patches / code you can share? This is something that's at the top of my hot list, and anything you've already got working would be awesome to look at, especially with the regard to how your developer approached things. Don't want to reinvent the wheel if you have something that works (or is almost there).

So it sounds like we should actually start at #781720: Refactor the Paypal WPS IPN code and see where that leads us. I'll take a look and start to chip away at this on a dev site, unless puravida comes back with something similar that's already been fleshed out.

EDIT: I see the patch looks like it's RTBC, unless others come back and say that it's broken. I'd love to test this on Dev and see about helping it get rolled into core. I'll try to bug some of the UC guys and see about having them check it out and possibly roll it in.

Also, the comment at #781720-8: Refactor the Paypal WPS IPN code seems to suggest there need to be full hooks for this process. Do you agree? Or do you think your patch covers it and really just needs to be rolled into core to get this issue fixed for good? (I figure there is always room to improve, but without a total payment hooks rewrite, it's good to try not to bite off more than one can chew.)

Hello All,

I don't have time to catch up on this thread (which has blossomed since I last visited), but our developer did overcome some issues and identified 2 "bugs"... What we have now is the ability to accept Paypal Express Checkout for recurring orders (max 10 recurring per checkout) and also WPP for credit cards.

The updates needed were across a few modules, including the vanilla ubercart paypal module itself. So it's best to let the developer handle the patch submission (especially b/c I am not familiar with how to provide patches).

We are starting a 2nd project with this developer tomorrow and part of that project will be to post patches here for inclusion.

Thanks,

Brandon

Brandon -

Thanks for the update. Looking forward to seeing what your developer has so far.

erik

subscribe

subscribe

@puravida,

Any update? Would love to have your developer in on the discussion here, as well as these issues which are related:
#898050: hook_order('submit', ...) invoked by paypal module before order actually is submitted
#781720: Refactor the Paypal WPS IPN code
#805454: Make Paypal WPS work with uc_recurring_order

Sorry for the extended delay. Was involved in a lot of pressing personal issues for the past month and now out of the country for the next month. Just got situated and trying to catch up.

Our developer has opened two new threads that encompass the updates he has made:

http://drupal.org/node/904074
It sounds like univate is waiting on anarchid to submit patches and I will follow up to see where Anarchid is on that piece.

http://drupal.org/node/904056
univate signaled that this may be fixed in current dev. Please confirm.

Best regards,

Brandon

Brandon, that's awesome news.

Here are patches based on the new module. Keep in mind the uc_recurring_hosted patch is based on the current stable, not the current dev, which has some refactoring in it that may alter the way this patch applies, so the changes here will need to be merged with latest dev if they're accepted.

I haven't tested them yet but I will. I'm putting them here so others can at least review the changes.

EDIT: I realized I should've posted them at the other thread: #904074: UC Recurring Paypal Express Checkout integration / recurring fee handler - I'll cross-post now.

EDIT 2: See my post below this one with regard to the uc_recurring.module patch.

Scrap that uc_recurring_hosted.patch file, this one is the correct one. That was the one based on the current -dev of uc_recurring, I think. (Version number is newer.)

Attached is the one based on the current stable (alpha4).

Ah, bummer. I hadnt gotten to look at it yet... will wait to see if the dev comes back with more.

Subscribing...

Marking that there's a patch here (although it apparently needs work).

Okay, so recurring fees with WPS break because when "Review order" is pressed, uc_recurring_hosted_paypal_wps_process() is fired. Unfortunately this is looking for $_POST['subscr_id'] coming back from PayPal. I'm wondering if the solution is to move this to be handled when the order is submitted? I didn't know that the subscriber ID was setup when the order gets reviewed - that seems odd, since a user could hit "Back" and revise their cart, add some stuff to it, etc.

Would it make sense to refactor this and move that code to a new step in the process? Is that even possible?

I talked to Ryan and he thinks something like this should be moved to either Conditional Actions, or the IPN handler. Likewise, if it's worth taking the time to think of a better solution than the patch in #700270: Remove hook_uc_order('submit') as it is never called for redirected payment methods then we can consider that too.

subscribing.

Nigel: The patch in question has been reverted. Look at the patch here: #898050: hook_order('submit', ...) invoked by paypal module before order actually is submitted (Patch is at comment #9). Applying that patch should solve the issue with PayPal WPS and UC Recurring not working together.

If I just install the latest dev version, is this patch already applied?

I believe so. http://drupal.org/cvs?commit=438610

This patch has been reverted as of the Ubercart 6.x-2.x-dev release from Oct. 20th. I think we can set this to fixed now.

reply to #38
is it still valid for UC 2.4 and uc_recurring 2.0-alpha5?
can the patch make them work together? is there another way?

Yes, if those are the versions you're using, the patch will work for UC 2.4. All it does is comment some code out of being used.

Sub

@Anticosti, this issue is now closed.

I would like to know if uc_recurring-alpha6 and ubercart-2.4 + paypal WPS are working.

If they are, please update the uc_recurring project page to reflect this.

subscribe - any updates on PayPal WPS?

Still exactly the same as #48 states - you need to either patch 2.3/2.4, use 2.2, or use 2.x-dev. Doing that, along with alpha6 of this module, will work fine with WPS.

Is it the patch in #31 that needs to be applied?

As I said in #44, http://drupal.org/node/781010#comment-3938892

Yes, if those are the versions you're using, the patch will work for UC 2.4. All it does is comment some code out of being used.

In other words, if you're using Ubercart 2.4, you need to apply the patch. If you're using Ubercart 6.x-2.x (dev) then you don't need the patch, as the patch is already committed to the dev branch.

Subscribing.

maybe mention on the project page some more specific details about the -dev that is required, i.e. anything later than October 30th 2010.

Can we upgrade to 6.x-2.6 and not experience any problems?

What does that question mean? Problems with what?

If you want uc_recurring to work with PayPal WPS, it sounds like you need the 2.x-dev version, as 2.6 is not recent enough.

You've asked this same question several times, and the answer is the same: use the -dev version for now, as the changes are not yet rolled into the stable Ubercart release.

You have misunderstood the question: Can I upgrade Ubercart to 6.x-2.6 without PayPal WPS recurring payments having problems? My understanding is the problem WPS faced was with Ubercart 2.3 and 2.4, so I wanted to see if I could upgrade to 2.6 instead of 2.3 or 2.4. This is the first time I have asked this particular problem and have not received a response, hence the double post. I'm sorry you felt the need to become agitated, but I'm simply a layman attempting to get support for a module I utilize.

Oh, I see what you mean. I believe 2.6 did not address the issue with recurring payments, so if you are going to upgrade, I would upgrade to -dev and not to 2.6. But that is just my hunch.

Thank you for the response; I was not sure the commits to the dev would automatically translate over to the releases, thanks.

Thanks for the clarification, Univate!

Everything appears to be okay on this end after a 2.6 ubercart upgrade.

Can you update the front page then so that it states 2.6 is OK.

Also there has been a lot of work going on re-structuring the conditional actions in D6 regarding sales, which has been committed to -dev.
#1192018: Duplicate order notification e-mail, and duplicate stock decrement

This might break recurring billing. I'm not 100% sure. Either way, univate should probably take a look.

Here's the patch in question:
http://drupal.org/node/1192018#comment-4963666
http://drupal.org/node/1192018#comment-4932644

Changes are also moving forward with re-organizing the rules in D7
http://drupal.org/node/1192018#comment-4974522