CRM-21788 Clean URLs in WordPress

[christianwach]

Overview

This PR accompanies - and is dependent on - this PR on the CiviCRM WordPress repo to implement clean URLs in WordPress.

Before

URLs in WordPress are of the form:
https://civicrm.latest/civicrm/?page=CiviCRM&q=civicrm/contribute/transact&reset=1&id=1

After

URLs in WordPress have the same structure as those in Drupal, e.g.:
https://civicrm.standalone.latest/civicrm/contribute/transact/?reset=1&id=1

Technical Details

I have kept these commits separate because each probably merits discussion in its own right. I'll leave the discussion to the comment thread.

Comments

Of particular note may be cea37e0 since it was only by fixing this that I got hit by (and solved) the dreaded "Could not find valid value for id” error page. This is one of the most common errors reported on Stack Exchange:

https://civicrm.stackexchange.com/search?q=Could+not+find+valid+value+for+id

It occurs when URLs are malformed like so:

https://civicrm.latest/civicrm/event/register/?&_qf_ThankYou_display=true&qfKey=a3f542cba021a05df18f477f691f8c97_7747

Note the ?& which is the result of the perform() method incorrectly handling (perfectly valid) URLs that end with an ampersand. I wonder if this is the root cause of this slippery issue?

[kcristiano]

@christianwach Fantastic work. I've built test sites with these patches and they work as expected.

I've run upgrades from 4.7.27 to latest master as well as fresh installs.

The code looks good and results are as expected.

We plan on further testing as this will need a good going through to find any issues. I'd recommend merging this PR and civicrm/civicrm-wordpress#123 early to get in the next RC.

[totten]

Question regarding r-tech: If someone has old-style links in their bookmarks, in a custom navbar, in another plugin, etal... what happens if they click the old-style link?

[kcristiano]

@totten Based on my testing: Old style links still work with the query string still work, they don't convert. Pages with shortcodes also work.

[eileenmcnaughton]

@kcristiano There have been some changes in civicrm/civicrm-wordpress#123 since you recommended merging - with @mlutfy & @christianwach working on it - do you still think we should be merging this at this stage?

@christianwach are you able to squash the commits on both of these?

[christianwach]

@eileenmcnaughton Yes, eventually. The commits are left separate because I think we need clarity of purpose during the testing phase. Personally, I think we're still in the testing phase on this one - I recently hit a "session lost" error like the one @mlutfy describes but have yet to discover the cause.

[kcristiano]

I agree that this needs more testing. I am a bit concerned if we added to the 5.3 cycle it might be too soon. I'd rather leave it open and test.

[eileenmcnaughton]

@kcristiano 5.3 rc is branched. Do you want this merged into 5.4 now - which will give it a few weeks before the 5.4 rc + the 5.4 rc period

[kcristiano]

@eileenmcnaughton Thanks for asking. Let me talk to @christianwach I do think we should merge civicrm/civicrm-wordpress#130 and civicrm/civicrm-wordpress#125 now, then rebase the 'cleanurl' PRs.

Those two civicrm-wordpress PRs (above) have been held up a bit and should be in. This one should either go in for testing or be pulled back and resubmitted later - at least that is my current thinking.

[christianwach]

@eileenmcnaughton @kcristiano Please don't merge. The outstanding issues are not resolved.

[eileenmcnaughton]

Thanks @christianwach I'm going to close this for now then. You can re-open it when it's review-ready (and in the meantime it's fine to keep discussing here)

[christianwach]

I have opened cea37e0 and 316bed1 as separate PRs. Although this PR revealed them, they are independent bugs in their own right. Fixing them will simplify the eventual Clean URLs PR.