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

Allow for custom base page in AJAX requests #23420

Merged
merged 2 commits into from
May 13, 2022

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented May 10, 2022

Overview

See #20702. This is a simpler, safer fix for the same issue.

Variants of this issue come up a lot with anything CiviCRM that uses AJAX/API requests on the frontend with a custom base page on WordPress. Eg. Stripe - https://lab.civicrm.org/extensions/stripe/-/issues/370

Before

Custom base page does not work for AJAX requests from "frontend".

After

AJAX requests can be used with any "base page".

Technical Details

On WordPress it is possible to define a custom "base page" for frontend CiviCRM URLs. Eg. if you specify custom base page as crm then the URL civicrm/ajax/rest would need to be rewritten as crm/ajax/rest.

CiviCRM uses a placeholder URL civicrm/placeholder-url-path in crm.ajax.js and l10n.js scripts which is replaced with the actual URL using a match/replace. But this fails if you have the custom basepage because the placeholder URL gets set to eg. crm/placeholder-url-path in l10n.js and then does not match the find/replace in crm.ajax.js.

We fix that by matching on the part after civicrm so we are not dependent on the part that can change. The second commit in this PR changes civicrm/placeholder-url-path to civicrm/civicrm-placeholder-url-path so that the second part is more unique (in case we had another plugin/software on the frontend that also decided to use placeholder-url-path).

Comments

You can test this by eg:

  1. Open a contribution page on WordPress frontend.
  2. Run CRM.url('civicrm/ajax/rest') on the browser console and seeing what it returns.
  3. Now do the same again but configure CiviCRM/WordPress to use a custom base page.

Be careful that you clear CiviCRM caches between tests (l10n.js is processed using assetbuilder and is only updated when caches are cleared).

@civibot
Copy link

civibot bot commented May 10, 2022

(Standard links)

@civibot civibot bot added the master label May 10, 2022
@eileenmcnaughton
Copy link
Contributor

@christianwach @kcristiano input?

@kcristiano
Copy link
Member

@eileenmcnaughton The fix makes sense and I do want to test in a number of situations.

@kurund
Copy link
Contributor

kurund commented May 11, 2022

@mattwire

Thanks, I have tested and the fix works for me.

@christianwach
Copy link
Member

@eileenmcnaughton I'm happy with this.

Great catch @mattwire :)

@demeritcowboy
Copy link
Contributor

This seems to bork drupal 9 (e.g. the menu disappears). I'm not clear on how this code works so I'm not sure what to suggest yet. When I do the console test CRM.url('civicrm/ajax/rest') it gives /civicrm/ajax/rest before the patch and /civicrm/-placeholder-url-path after.

@mattwire
Copy link
Contributor Author

@demeritcowboy I wonder if this line (https://github.com/civicrm/civicrm-core/pull/23420/files#diff-ca1e48f494632f29fb7b6867e7767b4507d345e1f468e9671f3bc9a102c8b71eR32) is generating a path starting with /civicrm/.. instead of civicrm/.. like on other CMSs?

@demeritcowboy
Copy link
Contributor

Checking...

Before the patch that line comes out as

CRM.url({back: '/civicrm/placeholder-url-path?civicrm-placeholder-url-query=1', front: '/civicrm/placeholder-url-path?civicrm-placeholder-url-query=1'});

After:

CRM.url({back: '/civicrm/-placeholder-url-path?civicrm-placeholder-url-query=1', front: '/civicrm/-placeholder-url-path?civicrm-placeholder-url-query=1'});

The difference seems to be the - for front.

@mattwire
Copy link
Contributor Author

@demeritcowboy What happens if you don't apply the second commit - ie. "Use a more unique placeholder url path for CiviCRM AJAX requests"

That commit is not required for the fix but seemed sensible.

@demeritcowboy
Copy link
Contributor

Cool the menu comes back.

@demeritcowboy
Copy link
Contributor

demeritcowboy commented May 13, 2022

It seems that any relative url that looks like civicrm/civicrm-X gets converted into a "drupal route" civicrm.civicrm and the path part has the leading civicrm removed and it becomes -X. (It happens in the getUrlIfValidWithoutAccessCheck() part https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Url.php/function/Url%3A%3AfromInternalUri/9.3.x)

If I use, say, crmajax- as the uniqifier prefix instead of civicrm- then it seems ok. That should work for everyone right?

diff --git a/js/crm.ajax.js b/js/crm.ajax.js
index 39f7c85ef9..4cb20cd03e 100644
--- a/js/crm.ajax.js
+++ b/js/crm.ajax.js
@@ -28,11 +28,13 @@
       path = path.split('#')[0];
     }
     frag = path.split('?');
+    // Remove basepage as it can be changed on some CMS eg. WordPress frontend.
+    frag[0] = frag[0].replace('civicrm/', '/');
     // Encode url path only if slashes in placeholder were also encoded
-    if (tplURL[mode].indexOf('civicrm/placeholder-url-path') >= 0) {
-      url = tplURL[mode].replace('civicrm/placeholder-url-path', frag[0]);
+    if (tplURL[mode].indexOf('/crmajax-placeholder-url-path') >= 0) {
+      url = tplURL[mode].replace('/crmajax-placeholder-url-path', frag[0]);
     } else {
-      url = tplURL[mode].replace('civicrm%2Fplaceholder-url-path', encodeURIComponent(frag[0]));
+      url = tplURL[mode].replace('%2Fcrmajax-placeholder-url-path', encodeURIComponent(frag[0]));
     }

     if (_.isEmpty(query)) {
diff --git a/templates/CRM/common/l10n.js.tpl b/templates/CRM/common/l10n.js.tpl
index 7e804f5568..67f69ada05 100644
--- a/templates/CRM/common/l10n.js.tpl
+++ b/templates/CRM/common/l10n.js.tpl
@@ -29,7 +29,7 @@
   CRM.config.entityRef = $.extend({ldelim}{rdelim}, {$entityRef|@json_encode}, CRM.config.entityRef || {ldelim}{rdelim});

   // Initialize CRM.url and CRM.formatMoney
-  CRM.url({ldelim}back: '{crmURL p="civicrm/placeholder-url-path" q="civicrm-placeholder-url-query=1" h=0 fb=1}', front: '{crmURL p="civicrm/placeholder-url-path" q="civicrm-placeholder-url-query=1" h=0 fe=1}'{rdelim});
+  CRM.url({ldelim}back: '{crmURL p="civicrm/crmajax-placeholder-url-path" q="civicrm-placeholder-url-query=1" h=0 fb=1}', front: '{crmURL p="civicrm/crmajax-placeholder-url-path" q="civicrm-placeholder-url-query=1" h=0 fe=1}'{rdelim});
   CRM.formatMoney('init', false, {$moneyFormat|@json_encode});

   // Localize select2

@mattwire
Copy link
Contributor Author

@demeritcowboy crmajax sounds sensible. I've updated the PR

@demeritcowboy
Copy link
Contributor

Thanks. Works for me.

@demeritcowboy demeritcowboy merged commit fa1346e into civicrm:master May 13, 2022
@mattwire mattwire deleted the custombasepage branch May 14, 2022 17:35
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.

6 participants