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

Pass path through rawurlencode when not using CIVICRM_CLEANURL #13043

Merged
merged 1 commit into from
Nov 1, 2018

Conversation

christianwach
Copy link
Member

Overview

Fixes https://lab.civicrm.org/dev/wordpress/issues/12

Before

CiviCRM does not generate valid URLs when CIVICRM_CLEANURL is off. This means URLs of the form:

http://civicrm.local/?page=CiviCRM&q=civicrm/contribute/transact&_qf_ThankYou_display=true&qfKey=KEY

that CiviCRM uses on the front-end.

What happens when a shortcode is used on the home page is that this URL is caught by redirect_canonical on the template_redirect hook and redirected to a rebuilt URL (where where q is urlencoded) that looks like this:

http://civicrm.local/?page=CiviCRM&q=civicrm%2Fcontribute%2Ftransact&_qf_ThankYou_display=true&qfKey=KEY

but, crucially, not before CiviCRM has sent the confirmation email and cleared the session. This causes the next page load to trigger invalidKeyRedirect and thus causes an infinite redirect. This does not happen on other pages because of the logic in redirect_canonical.

After

All relevant URLs are appropriately urlencoded in the form:

http://civicrm.local/?page=CiviCRM&q=civicrm%2Fcontribute%2Ftransact&_qf_ThankYou_display=true&qfKey=KEY

No redirection is made by redirect_canonical.

Technical Details

N/A

Comments

You may notice that this PR makes provision for a future where it is possible to allow Clean URLs in WordPress. The code to enable this is forthcoming in the CiviCRM WordPress repo - but since that code is dependent on #12969 and these changes to url(), it makes sense for this PR to include those changes here.

@civibot
Copy link

civibot bot commented Oct 31, 2018

(Standard links)

@civibot civibot bot added the master label Oct 31, 2018
@alifrumin
Copy link
Contributor

alifrumin commented Oct 31, 2018

I tested this pr by following the documentation in https://lab.civicrm.org/dev/wordpress/issues/12 and was able to view the thank you page for a contribution form shortcode on the homepage!!!!

Thank you @christianwach and @kcristiano

This is ready to be merged from my perspective (not sure if the failing check is related).

@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton
Copy link
Contributor

@christianwach @kcristiano this only touches WP so you should be the arbiters of it. If you feel it is mergeable but are just waiting on tests then add the 'merge on pass' label & someone will merge it when they notice the green tick.

@eileenmcnaughton
Copy link
Contributor

also I should note that I've seen the emails flying by on this issue & it really seems a great example of open source at it's finest - the 3 of you have collaborated with impressive speed & effectiveness on this

@kcristiano
Copy link
Member

kcristiano commented Oct 31, 2018

@eileenmcnaughton I am comfortable with this being merged.

@kcristiano
Copy link
Member

@eileenmcnaughton I can't add labels on this repo or I would set it merge on pass

@eileenmcnaughton
Copy link
Contributor

OK I added it - @totten I thought they were also going to be mergers on this repo (with the understanding they were merging WP code)

@seamuslee001
Copy link
Contributor

merging as per the tag

@seamuslee001 seamuslee001 merged commit 9080e65 into civicrm:master Nov 1, 2018
@kcristiano
Copy link
Member

@christianwach Take a look at https://lab.civicrm.org/dev/rc/issues/1#note_10697 This PR appears to create a regression in creating a new Contribution Page.

@christianwach
Copy link
Member Author

@kcristiano It's definitely not a regression - it has brought an additional bug to the surface. Please see my analysis on https://lab.civicrm.org/dev/wordpress/issues/12#note_10709

@kcristiano
Copy link
Member

thanks @christianwach for the quick analysis

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants