Skip to content
This repository has been archived by the owner on Feb 6, 2023. It is now read-only.

Enable tracking of urls with tokens (Issue #30) #46

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

artfulrobot
Copy link

This PR does the following:

  • If there's no token in the URL it does nothing different.
  • If there's a token in the domain or path (pretty unusal use case), it does nothing.
  • If there are tokenised parameters in the query, they are extracted, the rest of the URL is translated to a tracked one and the params (and fragment) are appended.
  • If there are tokens in the fragment, the rest of the URL is tracked and the fragment appended.

Notes on the code changes:

  • Includes phpunit tests to test the above.
  • Nb. disables case Allow installing on Drupal 8.4 #6 of CiviMail's test that Civi\FlexMailer\FlexMailerSystemTest::testUrlTracking inherits from. This is beacuase that case tests for the problem that this PR solves!
  • Moved this new code to a base class that Text and Html ones inherit from.
  • Allow dependency injection to assist unit testing.

@civibot civibot bot added the master label Sep 18, 2019
@mattwire
Copy link
Contributor

@artfulrobot Can you update the copyright header to the current civi standard one? Also does this relate to #51 at all?

@artfulrobot
Copy link
Author

artfulrobot commented Feb 14, 2020

@mattwire sure.

Interesting question about #fragments. On the one hand it's common to include a link twice and use different fragments, on the other hand there's more and more javascript apps running pages, e.g. civicrm's angularjs urls which do have fragments which could need tracking.

Anyway, I believe that this PR does NOT include fragments in the tracked part of the link, and is therefore an alternative to #51

@artfulrobot
Copy link
Author

Shortened license, rebased on master

@artfulrobot
Copy link
Author

Rebased this to master, added a commit to fix a test that was failing.

@totten
Copy link
Member

totten commented Jun 11, 2020

I haven't tried this, but the concept is great, and I love that there are tests.

Somewhat related: this relies on the extern/url.php behavior where it passes-through extra query-parameters. As part of civicrm/civicrm-core#17312, I questioned whether the pass-through behavior makes sense with router-based listeners. (e.g. docblock in https://github.com/civicrm/civicrm-core/blob/5.27/CRM/Mailing/Page/Url.php#L68).

But this is a good use-case for pass-through.

I do wonder how far we are from making a safer form of pass-through behavior. The main risk there was ... naming-conflicts between CMS-params and external/target URL params, e.g.

## Contact record
id=1000
custom_123=microtargetted

## Original URL
http://outside.com/?q={contact.custom_123}

## Tracked URL, straight pass-through, given to recipient
http://mysite.org/civicrm/mailing/url?u=1&qid=2&q=microtargetted

## Eeep, conflict. `?q=` has reserved meaning on Drupal.

I do wonder how hard it would be to nest the pass-through params, e.g.

## URL encoding "q=microtargetted"
q%3Dmicrotargetted

## Tracked URL, nested pass-through, given to recipient
http://mysite.org/civicrm/mailing/url?u=1&qid=2&passthru=q%3Dmicrotargetted

I suppose it's not a hard block for this PR -- since the existing extern/url.php contract is what it is, and adding this doesn't break anything per se. (It's just that the new functionality has some edge-case bugs which depend on the specific environment+data.)

But, again, overall, this is a valid use-case for a pass-through mechanism.

@artfulrobot
Copy link
Author

@totten thanks, and good points.

There are limitations in this for sure. However I don't think url-encoded passthrough will work since we don't have the token values. e.g. {contact.checksum} outputs a pair (which should just about work still); other data may also cause problems, like {contact.display_name} if it evaluates to Rich & Tim.

I think we'd need that to be a separate PRs:

  1. one in core to unpack passthru params in
  2. one in core to url encode token values if used in a URL context - that'll be fun.
  3. another one here to create the passthru params (that's the easy bit)

Maybe relevant: my use of this patch has been to include the mailing ID as a source param to a form that uses a page on the Civi site. Our site generated donation pages (bespoke, non-civi pages, but served by the same CMS), and we want to know which CiviMail mailing generates donations, so we inject tokens to identify the mailing, but we may also inject contact checksums so users don't have to enter their name and address.

So the limitations of this are:

  1. The tokens are only allowed in the query
  2. The tokens must evaluate to a URL-safe (or already URL-encoded) value.
  3. You can't use any params reserved by your CMS (so don't use q if you use Drupal, don't use half the English lanuguage if you use WordPress...)

I think these limitations are still worth it.

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

Successfully merging this pull request may close these issues.

3 participants