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

wp-rest - Adjust extern URL. Be more defensive about 'query' part. #192

Merged
merged 1 commit into from
May 21, 2020

Conversation

totten
Copy link
Member

@totten totten commented May 14, 2020

Overview

hook_civicrm_alterExternUrl allows for modifications to certain callback URLs (e.g. CiviMail open/click-through URLs). As a hook, there may be multiple parties which weigh-in.

This fixes an interaction that arises in the following conditions:

Before

The tracked link is converted to something like

http://wpmaster.bknix:8001/wp-json/civicrm/v3/url?page=CiviCRM&q=civicrm/mailing/url
  &u=5&qid=67

On the plus side, this includes query parameters u=5&qid=67 - which are important inputs for any extern/url-style end-point.

On the downside, the URL is mixing up artifacts which identify different end-points, ie

  • wp-json/civicrm/v3/url suggests the wp-rest end-point
  • ?page=CiviCRM&q=civicrm/mailing/url suggests the #17312 end-point

After

The tracked link appears as:

http://wpmaster.bknix:8001/wp-json/civicrm/v3/url?u=5&qid=68

Notice that this unambiguously uses the wp-rest end-point. It includes the important u=5&qid=67.

Technical Details

The contract for hook_civicrm_alterExternUrl allows multiple parties to weigh-in on the URL. In this case, we have 3 parties which can generate the URL:

  • One function suggests the original standalone scripts
  • Another function suggests the #17312 routes
  • Another function suggests the wp-rest routes

This is prioritized such that wp-rest will override the URLs suggested by #17312.

Aside: When using alterExternUrl to change the end-point, it seems perhaps most robust to build $url from scratch -- ie don't read anything from $url, read it from the request-values in $path and $query. I've suggested some updates to the examples: civicrm/civicrm-dev-docs#807

Overview
--------

`hook_civicrm_alterExternUrl` allows for modifications to certain callback
URLs (e.g.  CiviMail open/click-through URLs). As a hook, there may be multiple
parties which weigh-in.

This fixes an interaction that arises in the following conditions:

* Use Civi+WordPress
* Set `CIVICRM_WP_REST_REPLACE_MAILING_TRACKING` to `TRUE`
* Use civicrm/civicrm-core#17312
* Send a mailing (or test email) with a tracked link

Before
------

The tracked link is converted to something like

```
http://wpmaster.bknix:8001/wp-json/civicrm/v3/url?page=CiviCRM&q=civicrm/mailing/url&u=5&qid=67
```

On the plus side, this includes query parameters `u=5&qid=67` - which are important inputs for any `extern/url`-style end-point.

On the downside, the URL is mixing up artifacts which identify different end-points, ie

* `wp-json/civicrm/v3/url` suggests the `wp-rest` end-point
* `?page=CiviCRM&q=civicrm/mailing/url` suggests the #17312 end-point

After
-----

The tracked link appears as:

```
http://wpmaster.bknix:8001/wp-json/civicrm/v3/url?u=5&qid=68
```

Notice that this unambiguously uses the `wp-rest` end-point.  It includes
the important `u=5&qid=67` but skips the irrelevant `q=civicrm/mailing/url`.

Technical Details
-----------------

The contract for `hook_civicrm_alterExternUrl` allows multiple parties to
weigh-in on the URL. In this case, we have 3 parties which can generate
the URL:

* One function suggests the original standalone scripts
* Another function suggests the #17312 routes
* Another function suggests the `wp-rest` routes

Notably, all of these parties have the aim of *choosing the end-point*, so they
should construct the full `$url`.
@totten
Copy link
Member Author

totten commented May 14, 2020

jenkins, test this please

@kcristiano
Copy link
Member

@totten Thanks I'll test.

I also want @mecachisenros to chime in on this

@eileenmcnaughton eileenmcnaughton merged commit 545a143 into civicrm:master May 21, 2020
@eileenmcnaughton
Copy link
Contributor

Merged per @kcristiano comment regarding review on civicrm/civicrm-core#17312

@totten totten deleted the master-extern-query branch May 21, 2020 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants