Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix PayPal IPN URL and WordPress URLs when Permalinks are set to "Plain" #20063

Merged
merged 1 commit into from
May 3, 2021

Conversation

christianwach
Copy link
Member

@christianwach christianwach commented Apr 14, 2021

Overview

This was meant to be simply a fix for this issue on Lab, but in the process of creating that fix, it became evident that a deeper dive into WordPress URLs was necessary to untangle what was needed.

Before

  1. PayPal IPN URLs always pointed back to the Page/Post where the form was embedded
  2. CiviCRM URLs were broken when Permalinks are set to "Plain"

After

  1. PayPal IPN URLs always point to the WordPress Base Page
  2. CiviCRM URLs work as expected when Permalinks are set to "Plain"

Technical Details

This PR does a few things to achieve its ends:

  1. Introduces a getNotifyUrl() family of methods to the CRM_Utils_System* classes so that the PayPal IPN URL always targets the WordPress Base Page.
  2. Introduces CRM_Utils_System_WordPress::getBasePageUrl() in order to build URLs that target the WordPress Base Page when in a Shortcode context.
  3. Simplifies and clarifies CRM_Utils_System_WordPress::url() because WordPress URLs were broken when Permalinks are set to "Plain". Since CRM_Utils_System_WordPress::getNotifyUrl() is a stripped-down version of CRM_Utils_System_WordPress::url() that needs to respect both (a) Permalink settings and (b) whether Clean URLs are used, it became necessary to audit CRM_Utils_System_WordPress::url() in order to strip it down!
  4. Makes CRM_Utils_System_WordPress::getBaseUrl() a public method so that the duplicate method in the CiviCRM-WordPress plugin can be removed.

Comments

I'm opening this PR against master so that there's plenty of time to evaluate the changes to CRM_Utils_System_WordPress::url() in particular.

Many thanks to Agileware who part-funded this PR.

@civibot
Copy link

civibot bot commented Apr 14, 2021

(Standard links)

@civibot civibot bot added the master label Apr 14, 2021
@eileenmcnaughton
Copy link
Contributor

Thanks @christianwach - I glazed over when I got to the wordpress bit but the approach in terms of how the url functions are called looks good

@christianwach
Copy link
Member Author

@eileenmcnaughton Thanks for the feedback on the general approach. I can see that the diff on url() looks a bit messy, but I think the final code is much clearer and ahem actually works. (Testing and all the usual disclaimers notwithstanding, of course.) Being able to look at it in isolation from the Clean URLs build process has been helpful because I could concentrate on its context and inputs more than its output.

@eileenmcnaughton
Copy link
Contributor

Thanks @christianwach - I expect @jusfreeman will be keen to review this

@agileware-justin
Copy link
Contributor

Morning / Afternoon / Evening @christianwach

I've tested this using PayPal Express as that's the PayPal method used for our customers. Using CiviCRM 5.36.1.

When the payment is processed, the return page in CiviCRM is the confirmation page. Despite the option "Use a confirmation page?" being disabled. Once you click on the "Make contribution" button, payment is then confirmed. However this should not be required at all.

Any ideas?

Screenshot_2021-04-27 Help Support CiviCRM – WordPress CiviCRM Sandbox

Agileware Ref: CIVICRM-1668

@christianwach
Copy link
Member Author

@agileware-justin I can't reproduce this with the patch on 5.35.1. Works as expected without the Confirmation Page, i.e. redirect to PayPal happens, then back to the Private Page with the Thank You content. IPN completes successfully. Perhaps your issue is unrelated?

@christianwach
Copy link
Member Author

christianwach commented Apr 27, 2021

@agileware-justin Also tested with patch on vanilla WordPress 5.7.1 & CiviCRM 5.36.1. Works as expected without the Confirmation Page. Suspect your issue does not relate to this PR. Can you replicate with a vanilla install?

FWIW, this PR doesn't touch any of the code related to the page flow.

@kcristiano
Copy link
Member

@christianwach "Private" Page: Can you define? is that a WP Private page (only visible to site admins and editors), password protected or a page restricted by some other method (Plugin restricted content such as groups, members, etc)

I am trying to determine how best to test this.

@christianwach
Copy link
Member Author

@kcristiano You can test by setting the Post Status of the Page to "Private". Doesn't really matter TBH, the key is that wherever the Shortcode is the IPN URL always points to the Base Page.

@agileware-fj
Copy link
Contributor

agileware-fj commented May 3, 2021

Confirming use cases:

  • Paypal Standard – IPN goes back to the CiviCRM public URL, user is returned to the private page URL with the Thank You page embedded. Payment status is Completed.
  • Paypal Express – Come straight back to the private page, but doesn't appear to need a separate IPN, so payment status is Completed in CiviCRM. User is left with the Confirmation page embedded, which doesn't make any sense for the Paypal Express workflow, but that's a different issue.

@eileenmcnaughton
Copy link
Contributor

@christianwach @kcristiano I think the review by @agileware-fj green lights this for merge (thanks all!) - just checking that there is nothing outstanding from your end

@kcristiano
Copy link
Member

@eileenmcnaughton I am OK with it. I spoke to @christianwach last week and the improvement of having the IPN always go to the base page is a great improvement. I think it'd merge-ready based on @agileware-fj 's review

@kcristiano kcristiano added the merge ready PR will be merged after a few days if there are no objections label May 3, 2021
@eileenmcnaughton
Copy link
Contributor

OK - if you 3 think it's good then I'm happy!

@eileenmcnaughton eileenmcnaughton merged commit da3e08e into civicrm:master May 3, 2021
@christianwach christianwach deleted the lab-wp-93 branch March 22, 2022 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants