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

dev/core#2436 On WordPress, redirect back to the event registration o… #19718

Merged
merged 1 commit into from
Apr 9, 2021

Conversation

aydun
Copy link
Contributor

@aydun aydun commented Mar 3, 2021

…r contribution page that prompted the user to login.

Mostly copied from CRM_Utils_System_DrupalBase.php

Overview

See https://lab.civicrm.org/dev/core/-/issues/2436

Before

On WP, event registration and contribution page display a message inviting user to login if the page includes a profile that allows or requires a CMS user registration. After logging in, the user is left at the WP profile page.

After

After logging in, the user is redirected to the original event registration or contribution page.

@civibot
Copy link

civibot bot commented Mar 3, 2021

(Standard links)

@civibot civibot bot added the master label Mar 3, 2021
Copy link
Member

@christianwach christianwach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have an issue with the flow, only the usage of wp_login_url().

@@ -927,19 +927,43 @@ public function getUniqueIdentifierFromUserObject($user) {
*/
public function getLoginURL($destination = '') {
$config = CRM_Core_Config::singleton();
$loginURL = wp_login_url();
$loginURL = wp_login_url() . ($destination ? '?redirect_to=' . $destination : '');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't how wp_login_url() works - please use it with params as per the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - please see revised version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now 👍

…r contribution

page that prompted the user to login.

Mostly copied from CRM_Utils_System_DrupalBase.php
@eileenmcnaughton
Copy link
Contributor

@christianwach I assume you want me to merge this?

@christianwach
Copy link
Member

@eileenmcnaughton I haven't done an r-run but I can't fault the code.

@christianwach
Copy link
Member

@eileenmcnaughton Having said that I'd really like to see an r-run on this - particularly with Clean URLs because I can't tell offhand whether this addition to the query params will work as expected.

@aydun Could you provide some sample values of $destination with and without Clean URLs please?

$destination = NULL;
if ($args) {
// append destination so user is returned to form they came from after login
$destination = CRM_Utils_System::url(CRM_Utils_System::currentPath(), 'reset=1' . $args);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be forced to the front end or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH this was copied from https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/System/DrupalBase.php#L488-L505
It works as is for the case described in https://lab.civicrm.org/dev/core/-/issues/2436
I'll remove the other bits and keep it simple. If anyone wants this for PCP's they can add later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drupal doesn't have the same concern about front end v backend as WordPress thoughts @kcristiano @haystack

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do an r-run I am concerned about who it will get the URLs depending on front-end vs back-end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't envisage a situation where this redirect would happen on the back end. If someone can give an example, then yes I'd agree that forcing front end would be necessary.

@eileenmcnaughton
Copy link
Contributor

@kcristiano @christianwach I will merge this & the other one when / if one of you says 'please merge' - so when you are ready please give the order

@kcristiano
Copy link
Member

r-run - works as expected.

@eileenmcnaughton Please merge

@eileenmcnaughton
Copy link
Contributor

thanks @kcristiano

@eileenmcnaughton eileenmcnaughton merged commit c2655a0 into civicrm:master Apr 9, 2021
@aydun
Copy link
Contributor Author

aydun commented Apr 9, 2021

thanks @kcristiano

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

Successfully merging this pull request may close these issues.

5 participants