dev/core#973 Fix legacy IPN endpoint for Drupal

[artfulrobot]

Overview

The old extern/ipn.php script was failing.

Before

Calls failed because Drupal was not bootstrapped but was assumed to have been.

After

Works as expected.

Technical Details

I added a bootstrap call.

Comments

Note that the bootstrap does not load a user (which is the default behaviour). I don't think a CMS user is required for this anyway, but pointing it out.

https://lab.civicrm.org/dev/core/issues/973

[civicrm-builder]

Can one of the admins verify this patch?

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[jaapjansma]

I do think this is a good patch.

[laryn]

Should we put an "or backdrop" condition in with it?

[mattwire]

What is the reason for still using the deprecated extern/ipn.php script instead of the newer civicrm/payment/ipn/XX endpoint? A better fix might be to "encourage" people to switch to the newer endpoints as they have been available for quite a long time now..

[artfulrobot]

@mattwire because you physically cannot change the IPN for PayPal. When the payment is submitted it is bound to a fixed URL(!) So if you have been running CiviCRM for years you'll always need that extern script.

[artfulrobot]

@laryn re Backdrop: Probably. Would need someone with Backdrop to test. For all I know this might also be broken in WordPress as well though. I don't know.

[mattwire]

@artfulrobot Can you add in some commenting to this file (as part of this PR) that explains why we still need the extern/ipn.php for paypal and we can't "just get rid of it". And also modify the commit message so it's a bit clearer without having to read the gitlab first? Then I would feel ok to merge.

[artfulrobot]

How's that @mattwire ?

[mattwire]

@artfulrobot Comment is great but now the actual patch has gone away...

[artfulrobot]

@mattwire huh? weird. Try reloading github page? I forced an update to the branch to clean it up. (also spotted I'd accidentally joined three lines in a comment so corrected that too).

[laryn]

@artfulrobot I think he's referring to this: https://github.com/civicrm/civicrm-core/pull/14272/files

[mattwire]

@artfulrobot Yes, this was your original patch (cb70683) - now that bit's not included in the latest PR!

[artfulrobot]

*blush* !

Sorry! Try this now! Also I swapped out the if elseif for a switch as I figured it would be clearer/nicer to add in case 'Backdrop: if that turns out to be a good idea.

[laryn]

I think it would be quite safe to add case 'Backdrop': immediately after case 'Drupal': since it should be identical but I don't have a way to test such an IPN from Paypal currently.

Also, maybe it would be good to move the //CRM-18245 comment to the Joomla switch to reference the issue which seems to be Joomla specific.
https://issues.civicrm.org/jira/browse/CRM-18245

Or, perhaps you've already thought of this and discarded the idea, but might it be better to lose the switch altogether? What's the functional difference between CRM_Utils_System::loadBootStrap(); and CRM_Utils_System::loadBootStrap([], FALSE);?

[artfulrobot]

Backdrop, agreed seems sensible, I'll do that.

The difference between the 2 calls is whether the bootstrap should load a CMS user or not. It doesn't make sense to load a user to me, and sure enough it doesn't work doing so in Drupal but I didn't want to change someone else's code without testing that my change works in joomla.

[mfb]

As a long-term solution, could CiviCRM create an endpoint named extern/ipn.php and remove the file?

[eileenmcnaughton]

add to whitelist

[eileenmcnaughton]

@mfb it's a full path - /sites/all/modues/civicrm/extern/ipn.php - not sure thatis do-able

[mfb]

You can create an endpoint @ any path, so sites/all/modues/civicrm/extern/ipn.php would work, but it looks like CiviCRM supports setups where this isn't doable: no clean URLs.

[artfulrobot]

OK, included @laryn's Backdrop suggestion and put the Joomla comment by the joomla bit (thanks).

@mfb: yeah can't rely on clean paths. But still be good if CiviCRM scripts had a single way to bootstrap the whole system (CMS+Civi).

[seamuslee001]

Jenkins doesn't seem to like your style @artfulrobot https://test.civicrm.org/job/CiviCRM-Core-PR/26258/checkstyleResult/new/

[xurizaemon]

We know that PayPal respects HTTP 307 (Jon's SE answer, chat.civicrm.org with @artfulrobot).

Are these fixes really worthwhile, to keep maintaining extern/ipn.php?

🔥 🔥 🔥 rm -rf extern/ 🔥 🔥 🔥

/opinion 😁

be good if CiviCRM scripts had a single way to bootstrap the whole system (CMS+Civi)

dev/cloud-native#16

To be fair to @artfulrobot's MR, the change is probably not going to make things worse, but in your position I'd try to change your existing redirect to the modern IPN and be done.

[artfulrobot]

@xurizaemon what I'm unclear on is how the 307 can help redirect from

 extern/ipn.php?contributionID=123&contributionRecurID=456&contactID=1234&reset=1&...

to

civicrm/payment/ipn/100

Where you don't know the payment processor ID on the incoming request.

[artfulrobot]

@seamuslee001 think I've calmed that perfectionist Jenkins :-)

[xurizaemon]

Where you don't know the payment processor ID on the incoming request.

Uggh, fair.

Do you really not know, though? Does the site in question really have multiple PayPal processors configured? If not, I'd 307 to new-IPN URL with a fixed value; if so, I'd look at the proxy / relay approach mentioned in same chat, esp since the PP ID is derivable from the contributionID which you do have.

Anyway ... good luck with it :)

[artfulrobot]

@xurizaemon Actually they do have several PayPal accounts (although probably these old IPN requests are all going to one of those two), however we're talking about core CiviCRM here so we can't make assumptions if we want it to be a stable platform. I think we just need a way to keep this extern script as minimal to maintain as poss (I appreciate that's what your linked issue was in part about).

[eileenmcnaughton]

I guess this is safe since we were already doing it for Joomla! & you have tested for drupal - there seems to be adequate discussion ./ buy in. Merging

[eileenmcnaughton]

turns out this was a regression caused by #13439

[artfulrobot]

Thanks @eileenmcnaughton!

[eileenmcnaughton]

@artfulrobot do you have any thoughts about the comments here https://lab.civicrm.org/dev/drupal/issues/66#note_18471