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

Remove "page=CiviCRM" query string from WordPress front-end (5.26) #17352

Merged
merged 1 commit into from
May 20, 2020

Conversation

christianwach
Copy link
Member

@christianwach christianwach commented May 18, 2020

Overview

Duplicate of #17350 against 5.26 branch. Requires this PR to CiviCRM-WordPress. Solves this issue on Lab.

@civibot
Copy link

civibot bot commented May 18, 2020

(Standard links)

@eileenmcnaughton
Copy link
Contributor

hmm - this was on master but from comments I understand it should be 5.26 -so I tried to change it - but without success

@kcristiano
Copy link
Member

@eileenmcnaughton this is/was on 5.26 and I do think it should be. There seem to be some false commits now. Can we fix?

@christianwach
Copy link
Member Author

WTF? I'm pretty sure this didn't have all those extra commits yesterday. I'll force-push an update shortly.

@kcristiano
Copy link
Member

I've been doing as much r-run as I can on this.

I've been using 5.26.RC as the testing version.

I've tested in the following configurations - all are CiviCRM 5.26RC + #17352 and civicrm/civicrm-wordpress#194

I have gone through the following on ALL of the above.

  • Contribution Pages with shortcode
  • Contribution pages with CiviCRM url
  • membership Pages with shortcode
  • membership pages with CiviCRM url
  • Event list and Register Pages with shortcode
  • Event List and register pages with CiviCRM url
  • CiviMail Traditional - default urls
  • CiviMail Traditional - WP REST URLS
  • User Dashboard
  • As many admin pages as needed to set up and configure the above

I've tested old links in the style of https://wplocal.test/civicrm?page=CiviCRM&q=civicrm%2Fuser&reset=1&id=202&cid=202 and they redirect to https://wplocal.test/civicrm/?q=civicrm%2Fuser&reset=1&id=202&cid=202&civiwp=CiviCRM on WP 5.4.1 and 5.5.alpha

I've upgraded from 5.25 to 5.26 on one of the instances with these PRs

All behavior (so far) is as expected with one exception:

On WP 5.5 with mailing links set to use WP REST API the public view does not work on WP 5.4.1. This may be related to Lab WP 52

I could not reproduce on WP 5.5.alpha - but more testing is needed.

My inclination is that this is much better in than out and having it merged will allow more testing. I still have to go through Profiles, and I am sure I have forgotten something.

@christianwach Any thought?

cc @totten @eileenmcnaughton

@gah242s
Copy link
Contributor

gah242s commented May 19, 2020

Wow! That's impressive. Reading through that list, it looks like it already takes care of extensions like Caldera Forms. Is that a correct assumption or are extensions also going to have to be updated also?

@kcristiano
Copy link
Member

I ignored Caldera Forms CiviCRM (for now) as it does not use CiviCRM URLs. You could have some redirection going on so we will need to tets that, but I want to focus on the core functionality first.

@gah242s
Copy link
Contributor

gah242s commented May 19, 2020

Sounds reasonable! Thanks to you and the others for buckling down so tightly on this.

@seamuslee001
Copy link
Contributor

@kcristiano So would you think that we should merge this noting the outstanding issue with lab WP 52?

@kcristiano
Copy link
Member

@seamuslee001 I do - I'd like to see if @christianwach has an opinion, but without this we are risking breaking ALL of CiviCRM. With this PR we've found one issue that only affects mailings that use the new (5.25) WP REST API filter for mailing endpoints.

Merging also gives us time to test between now and June 3rd

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

I've added merge-ready based o @kcristiano's comments. I feel like absent feedback against from @christianwach in the next 24 hours we should merge & start testing with this merged

@christianwach
Copy link
Member Author

I made a small change to the linked PR in response the testing done by @kcristiano and it seems to solve the only outstanding issue.

@kcristiano
Copy link
Member

see civicrm/civicrm-wordpress#194 (comment)

Based on that commit and further r-run this is good to merge

@colemanw colemanw merged commit 193f6e0 into civicrm:5.26 May 20, 2020
@christianwach christianwach deleted the lab-wp-49-civi5-26 branch March 22, 2022 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.26 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.

6 participants