Do not add tracking to internal anchor URLs #20115
Conversation
|
(Standard links)
|
|
For purposes of streamlining testing, I could put this related proposed change in here too. |
| if (preg_match('/\.(png|jpg|jpeg|gif|css)[\'"]?$/i', $url) | ||
| or substr_count($url, 'civicrm/extern/') | ||
| or substr_count($url, 'civicrm/mailing/') | ||
| or ($url[0] === '#') |
There was a problem hiding this comment.
@larssandergreen would be worth it to add here another condition as ?:
or substr_count($url, 'mailto:')
There was a problem hiding this comment.
Is there a good reason not to track mailto: links? I'm in favour of keeping them in the clickthrough report, both in terms of counting them as a click for the overall rate and in the individual link statistics.
I guess for people who use a non-browser based client, there's a roundtrip to the browser and back for the clicked link, but is that a big problem?
There was a problem hiding this comment.
hmm ok, we don't see the value of that tracking those, specially when sending and email through a SMTP provider that tracks its own stats, so it's a triple jump to get at the end to compose an email. But seems fair to follow the discussion in a different issue/PR, than this one focused on local anchors
There was a problem hiding this comment.
FWIW, it looks like there was a previous endeavor track mailto:s when use BAO delivery -- eg @JKingsnorth's #7950.
I guess when considering whether to track a URL, there will be a few buckets like:
- Cases where everyone agrees tracking is appropriate. (Outbound
http(s)URLs) - Cases where everyone agrees tracking is not appropriate. (Page-internal anchors)
- Cases where people may reasonably have different opinions (eg
mailto:).
I'd be a soft +1 for some kind of setting or hook to address the latter.
There was a problem hiding this comment.
+1 to @totten 's +1 to a setting or hook to address mailto: (or even whatsapp: etc.) =]
There was a problem hiding this comment.
That sounds good to me too. We should cover at least mailto: and tel:, I'd think. But that's another PR.
|
I've also added a similar PR to only track <a> URLs, instead of all URLs inside a tag (as above). |
|
@sluc23 @JKingsnorth @totten - I've read through the discussion on this PR and my take is that people think the change in this PR should be merged but there is an extension that is not in it and not agreed. On that basis I'm giving the PR 'merge-ready' - please speak now if for any reason you do not think this code and our codebase shall be wed together from this day forth until a PR do them part |
eileenmcnaughton
added
the
merge ready
|
Merging based on tag |
Before
Tracking is added to internal anchor links, breaking them.
After
Internal anchor links do not have tracking added and so they work in emails (where the email client supports them).
Tested and works in both traditional and Mosaico mailings. Based on @totten's suggestion here.