Migrate CiviMail "extern" scripts to conventional routes #17312
Conversation
Ex: In a request for `http://dmaster.bknix:8001/civicrm/mailing/url?u=3&qid=2`, the redirect URL has the value `q=civicrm/mailing/url` incorrectly appended.
For example, suppose your goal is to recognize any CiviMail links going to `woogle.com` and
append code with the mailing ID (`&src=civimail_123`). Do this:
```php
if ($context['for'] === 'civicrm/mailing/url' && preg_match('/woogle\.com$/', $url->getHost())) {
$mailing_id = CRM_Core_DAO::singleValueQuery('
SELECT mj.mailing_id FROM civicrm_mailing_event_queue meq
INNER JOIN civicrm_mailing_job mj ON mj.id = meq.job_id
WHERE meq.id = %1
', [
1 => [$context['queue_id'], 'Int']
]);
$url = $url->withQuery($url->getQuery() . '&src=civimail_' . $mailing_id);
}
```
This toggle allows a sysadmin to choose between the legacy `extern/foo.php` and newer built-in routes.
|
(Standard links)
|
…egotten functionality
| // Ugh | ||
| unset($query_param['page']); | ||
| unset($query_param['noheader']); | ||
| } |
There was a problem hiding this comment.
When I first tested on a wpmaster site, extractPassthroughParameters() was still producing some noisy GET params (e.g. a link to https://www.google.com winds up as https://www.google.com?noheader=1). This extra conditional resolved that.
But it's also ugly. TBH I'd be happier to rip out extractPassthroughParameters() and make a small BC-break:
- The notion in https://issues.civicrm.org/jira/browse/CRM-7103 /
extractPassthroughParameters()is not well-defined across different routing systems. - This PR makes a better API available for the same use-case (
hook_civicrm_alterRedirect). - I skimmed
hook_civicrm_alterMailParamsconsumers inuniverseand couldn't spot any doing URL manipulation on CiviMail content. (The fuziontransactionalext was close, but it skips CiviMail.) - If one is actually relying on
hook_civicrm_alterMailParamsfor URL manipulation, then you can buy some time for a transition -- setdefaultExternUrl=standaloneand you get the old behavior.
There was a problem hiding this comment.
@totten I'm pretty sure that this issue on Lab will have an impact here - because bleeding edge WordPress has changed the way in which the page query var is used and it now assumes that this is the the desired path and redirects accordingly. We're going to have to audit CiviCRM's usage of this query var in a WordPress context and strip out any usage of it on the front-end. This should lead to unset($query_param['page']); being unnecessary.
There was a problem hiding this comment.
OK, well, ?page= won't be missed. :)
Since coordinating the timelines on these changes may be tricky, I guess the implication here is... if #17312 does keep extractPassthroughParameters(), then eventually we can remove the unset($query_param['page'). For the moment, from a WP-integration perspective, there's no harm in keeping the unset().
|
I've done some testing on
(Note: When testing the links, make sure to control for permissions -- e.g. open the links in a separate browser or "private" browser where the user is unauthenticated.) I have not yet tested I also haven't tested with |
|
@totten I've done a quick run through on WP with the PR both with and without WP REST routing. Testing looks good. Still need to test with Mosaico/flexmailer. I do want to review what will need to change due to: I'll also add this to the RC testing and look at Joomla as well. Update: Testing with Mosaico and Flexmailer on WP: Mailing Links with PR - http://wpcvmaster.test/wp-admin/admin.php?page=CiviCRM&q=civicrm/mailing/url&u=13&qid=16 works Mailing links with PR and WP REST API routing: http://wpcvmaster.test/wp-json/civicrm/v3/url?page=CiviCRM&q=civicrm/mailing/url&u=11&qid=12 Fails Mailing Links on Master (No PR) and Mosaico/Flexmailer: http://wpcvmaster.test/wp-json/civicrm/v3/url?u=15&qid=21 works |
|
test this please |
|
I think transitional support fo the old extern urls is good - but I have some concern about the open endedness. I'd rather see something pointing out that the setting will be removed and that if a site needs to resort to setting it they should create a gitlab issue. I would make sure the name of the setting, not just the help, made that clear |
| ], | ||
| 'default' => 'router', | ||
| 'add' => '5.27', | ||
| 'title' => ts('Extern URL Style'), |
There was a problem hiding this comment.
Transitional Extern URL style ...
There was a problem hiding this comment.
Oh, I just noticed your bit about gitlab. Here's another variation:
There was a problem hiding this comment.
@totten @eileenmcnaughton I'm confused -- why would a GitLab issue be necessary?
There was a problem hiding this comment.
@christianwach the theory is that people would only use this setting if they need to because something weird is happening. Once there are no more something weirds then the setting & support for it can go. Probably in a few months we would remove from the form & later on from the code base
There was a problem hiding this comment.
@eileenmcnaughton Sounds reasonable to me 👍
| 'title' => ts('Extern URL Style'), | ||
| 'is_domain' => 1, | ||
| 'is_contact' => 0, | ||
| 'help_text' => NULL, |
There was a problem hiding this comment.
Also - the help text should ideally be in here - although I think that if the settings page is not using the generic tpl it may not work
There was a problem hiding this comment.
That sounds cool :) - I didn't realize that dataflow was now working for some forms.
FWIW, when I grep settings/ for other examples of help_text, they all appear to be a couple sentences long. Since this is actually several paragraphs (which in turn requires markup), perhaps it's still OK in the .hlp file?
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`.
This aims to improve cooperation between `flexmailer`, `civicrm-wordpress:wp-rest/`, and civicrm/civicrm-core#17312 -- so that flexmailer produces compliant URLs. Before ------ The open-tracking URL is constructed as a reference to the standalone script `extern/open.php` using certain `$config` properties. After ----- On Civi v5.23+, it uses `CRM_Utils_System::externUrl()` API to request the full URL of `extern/open`. Flexmailer is no longer responsible for figuring the URL, and other agents (via `hook_civicrm_alterExternUrl`) can do so. On Civi v5.22 and earlier, it continues using the same old formula. This provides drop-in/bug-level-compatibility on older versions. This can be removed in a couple months. Comments -------- I suspect this also fixes [dev/mail#17](https://lab.civicrm.org/dev/mail/issues/17), wherein the open tracker has flawed URLs in certain multilingual configurations. However, I can't confirm for certain because I don't have that configuration. But just to game this out... * If the current patch fixes dev/mail#17, then huzza! * If it doesn't, then the problem is in `civicrm-core`'s `CRM_Utils_System::externUrl()` (not `flexmailer:OpenTracker.php`). Which means: * The problem already affects other use-cases in `civirm-core` in v5.23+. * The fix should go in `civicrm-core`. Either way, addressing dev/mail#17 shouldn't require any further change in `flexmailer`.
|
@kcristiano Great, nice testing. :) I've posted a PR to fix the oddball ( For
|
|
test this please |
|
I am waiting to do more testing on this until we merge #17352 and civicrm/civicrm-wordpress#194 |
|
@kcristiano the wait is over :-) |
|
I've done testing on WP. All on master woith both WP 5.41 and WP 5.5.alpha I tested:
All worked as expected. I did not test on Joomla. But this does look ready to merge along with civicrm/civicrm-wordpress#192 |
|
Thanks @kcristiano & @totten |
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`.
Overview
Update CiviMail's default behavior to replace
extern/open.phpandextern/url.phpwithcivicrm/mailing/openandcivicrm/mailing/url.This is an update/replacement for #17084. It contributes towards multiple issues - e.g. https://lab.civicrm.org/dev/drupal/-/issues/9, https://lab.civicrm.org/dev/drupal/-/issues/77, https://lab.civicrm.org/dev/core/-/issues/1708, and possibly https://lab.civicrm.org/dev/mail/-/issues/17
Before
http://example.com/...codepath.../civicrm/extern/url.php?u=NNN&qid=NNNhttp://example.com/...codepath.../civicrm/extern/open.php?q=NNNAfter
http://example.com/...codepath.../civicrm/extern/url.php?u=NNN&qid=NNN(same)http://example.com/...codepath.../civicrm/extern/open.php?q=NNN(same)http://example.com/civicrm/mailing/url?u=NNN&qid=NNN(new)http://example.com/civicrm/mailing/open?qid=NNN(new; note: 'qbecomesqid`)defaultExternUrl=routerordefaultExternUrl=standalone). The default isrouter.Technical Details
This is an update/replacement for #17084. Some distinguishing characteristics are:
extern/openin addition toextern/url.CRM_Utils_System::externUrl()). This should mean thatwp-civi-restandflexmailercan get decent forward/backward compatibility by using that API.Why allow the option? It's a hedge against some of risks/edges invovled.
settings_location.phpand/or customized boot logic (for multisite/URL dynamism). While these ought to work better out-of-the-box, it's an open-space and hard to verify from my POV.Of course, there are several upsides:
setting_location.php-style customizations).extern/url.phpreplacement supportshook_civicrm_alterRedirect. Compared tohook_civicrm_alterMailParams(which was reportedly used circa v3.3 to customize click-through tracker URLs), this is a simpler and safer way to programmatically supplement click-through URLs.Going forward, when we tackle migrations for some other
externscripts, the settingdefaultExternUrland the functionmigrateExternUrl()may come in handy again.