From: Tim Otten <totten@civicrm.org>
Date: Thu, 14 Jan 2021 10:29:49 +0000 (-0800)
Subject: Simplify tracking of URLs
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=6ad859f1b83295522d94e5392dd1cb00d5cd3775;p=civicrm-core.git

Simplify tracking of URLs

In the prior commit, it needed to conceptually change the behavior of
`getTrackerURL($oldURL): $trackedURL`, but it danced around it (adding
more classes and similar-sounding-methods, but distributed elsewhere).

This re-consolidates to keep a cleaner separation of responsibilities
between `{Html,Text}ClickTracker.php` (*parsing an HTML or text message for URLS*)
and `TrackableURL.php` (*translating the URL*).
---

diff --git a/CRM/Mailing/BAO/TrackableURL.php b/CRM/Mailing/BAO/TrackableURL.php
index 0113e53bb9..ea6d032db4 100644
--- a/CRM/Mailing/BAO/TrackableURL.php
+++ b/CRM/Mailing/BAO/TrackableURL.php
@@ -38,7 +38,15 @@ class CRM_Mailing_BAO_TrackableURL extends CRM_Mailing_DAO_TrackableURL {
    *   The redirect/tracking url
    */
   public static function getTrackerURL($url, $mailing_id, $queue_id) {
+    if (strpos($url, '{') !== FALSE) {
+      return self::getTrackerURLForUrlWithTokens($url, $mailing_id, $queue_id);
+    }
+    else {
+      return self::getBasicTrackerURL($url, $mailing_id, $queue_id);
+    }
+  }
 
+  private static function getBasicTrackerURL($url, $mailing_id, $queue_id) {
     static $urlCache = [];
 
     if (array_key_exists($mailing_id . $url, $urlCache)) {
@@ -87,6 +95,66 @@ class CRM_Mailing_BAO_TrackableURL extends CRM_Mailing_DAO_TrackableURL {
     return $returnUrl;
   }
 
+  /**
+   * Create a trackable URL for a URL with tokens.
+   *
+   * @param string $url
+   * @param int $mailing_id
+   * @param int|string $queue_id
+   *
+   * @return string
+   */
+  private static function getTrackerURLForUrlWithTokens($url, $mailing_id, $queue_id) {
+
+    // Parse the URL.
+    // (not using parse_url because it's messy to reassemble)
+    if (!preg_match('/^([^?#]+)([?][^#]*)?(#.*)?$/', $url, $parsed)) {
+      // Failed to parse it, give up and don't track it.
+      return $url;
+    }
+
+    // If we have a token in the URL + path section, we can't tokenise.
+    if (strpos($parsed[1], '{') !== FALSE) {
+      return $url;
+    }
+
+    $trackable_url = $parsed[1];
+
+    // Process the query parameters, if there are any.
+    $tokenised_params = [];
+    $static_params = [];
+    if (!empty($parsed[2])) {
+      $query_key_value_pairs = explode('&', substr($parsed[2], 1));
+
+      // Separate the tokenised from the static parts.
+      foreach ($query_key_value_pairs as $_) {
+        if (strpos($_, '{') === FALSE) {
+          $static_params[] = $_;
+        }
+        else {
+          $tokenised_params[] = $_;
+        }
+      }
+      // Add the static params to the trackable part.
+      if ($static_params) {
+        $trackable_url .= '?' . implode('&', $static_params);
+      }
+    }
+
+    // Get trackable URL.
+    $data = self::getBasicTrackerURL($trackable_url, $mailing_id, $queue_id);
+
+    // Append the tokenised bits and the fragment.
+    if ($tokenised_params) {
+      // We know the URL will already have the '?'
+      $data .= '&' . implode('&', $tokenised_params);
+    }
+    if (!empty($parsed[3])) {
+      $data .= $parsed[3];
+    }
+    return $data;
+  }
+
   /**
    * @param $url
    * @param $mailing_id
diff --git a/ext/flexmailer/src/ClickTracker/BaseClickTracker.php b/ext/flexmailer/src/ClickTracker/BaseClickTracker.php
deleted file mode 100644
index 970b52fbd0..0000000000
--- a/ext/flexmailer/src/ClickTracker/BaseClickTracker.php
+++ /dev/null
@@ -1,82 +0,0 @@
-<?php
-/**
- *
- * +--------------------------------------------------------------------+
- * | Copyright CiviCRM LLC. All rights reserved.                        |
- * |                                                                    |
- * | This work is published under the GNU AGPLv3 license with some      |
- * | permitted exceptions and without any warranty. For full license    |
- * | and copyright information, see https://civicrm.org/licensing       |
- * +--------------------------------------------------------------------+
- *
- */
-
-
-namespace Civi\FlexMailer\ClickTracker;
-
-class BaseClickTracker {
-
-  public static $getTrackerURL = ['CRM_Mailing_BAO_TrackableURL', 'getTrackerURL'];
-
-  /**
-   * Create a trackable URL for a URL with tokens.
-   *
-   * @param string $url
-   * @param int $mailing_id
-   * @param int|string $queue_id
-   *
-   * @return string
-   */
-  public static function getTrackerURLForUrlWithTokens($url, $mailing_id, $queue_id) {
-
-    // Parse the URL.
-    // (not using parse_url because it's messy to reassemble)
-    if (!preg_match('/^([^?#]+)([?][^#]*)?(#.*)?$/', $url, $parsed)) {
-      // Failed to parse it, give up and don't track it.
-      return $url;
-    }
-
-    // If we have a token in the URL + path section, we can't tokenise.
-    if (strpos($parsed[1], '{') !== FALSE) {
-      return $url;
-    }
-
-    $trackable_url = $parsed[1];
-
-    // Process the query parameters, if there are any.
-    $tokenised_params = [];
-    $static_params = [];
-    if (!empty($parsed[2])) {
-      $query_key_value_pairs = explode('&', substr($parsed[2], 1));
-
-      // Separate the tokenised from the static parts.
-      foreach ($query_key_value_pairs as $_) {
-        if (strpos($_, '{') === FALSE) {
-          $static_params[] = $_;
-        }
-        else {
-          $tokenised_params[] = $_;
-        }
-      }
-      // Add the static params to the trackable part.
-      if ($static_params) {
-        $trackable_url .= '?' . implode('&', $static_params);
-      }
-    }
-
-    // Get trackable URL.
-    $getTrackerURL = static::$getTrackerURL;
-    $data = $getTrackerURL($trackable_url, $mailing_id, $queue_id);
-
-    // Append the tokenised bits and the fragment.
-    if ($tokenised_params) {
-      // We know the URL will already have the '?'
-      $data .= '&' . implode('&', $tokenised_params);
-    }
-    if (!empty($parsed[3])) {
-      $data .= $parsed[3];
-    }
-    return $data;
-  }
-
-}
diff --git a/ext/flexmailer/src/ClickTracker/HtmlClickTracker.php b/ext/flexmailer/src/ClickTracker/HtmlClickTracker.php
index c9f7226db5..5e8955d516 100644
--- a/ext/flexmailer/src/ClickTracker/HtmlClickTracker.php
+++ b/ext/flexmailer/src/ClickTracker/HtmlClickTracker.php
@@ -10,25 +10,13 @@
  */
 namespace Civi\FlexMailer\ClickTracker;
 
-class HtmlClickTracker extends BaseClickTracker implements ClickTrackerInterface {
+class HtmlClickTracker implements ClickTrackerInterface {
 
   public function filterContent($msg, $mailing_id, $queue_id) {
-
-    $getTrackerURL = BaseClickTracker::$getTrackerURL;
-
     return self::replaceHrefUrls($msg,
-      function ($url) use ($mailing_id, $queue_id, $getTrackerURL) {
-        if (strpos($url, '{') !== FALSE) {
-          // If there are tokens in the URL use special treatment.
-
-          // Since we're dealing with HTML let's strip out the entities in the URL
-          // so that we can add them back in later.
-          $originalUrlDecoded = html_entity_decode($url);
-          $data = BaseClickTracker::getTrackerURLForUrlWithTokens($originalUrlDecoded, $mailing_id, $queue_id);
-        }
-        else {
-          $data = $getTrackerURL($url, $mailing_id, $queue_id);
-        }
+      function ($url) use ($mailing_id, $queue_id) {
+        $data = \CRM_Mailing_BAO_TrackableURL::getTrackerURL(
+          html_entity_decode($url), $mailing_id, $queue_id);
         $data = htmlentities($data, ENT_NOQUOTES);
         return $data;
       }
diff --git a/ext/flexmailer/src/ClickTracker/TextClickTracker.php b/ext/flexmailer/src/ClickTracker/TextClickTracker.php
index 4d92397298..31cdd90c2c 100644
--- a/ext/flexmailer/src/ClickTracker/TextClickTracker.php
+++ b/ext/flexmailer/src/ClickTracker/TextClickTracker.php
@@ -10,21 +10,13 @@
  */
 namespace Civi\FlexMailer\ClickTracker;
 
-class TextClickTracker extends BaseClickTracker implements ClickTrackerInterface {
+class TextClickTracker implements ClickTrackerInterface {
 
   public function filterContent($msg, $mailing_id, $queue_id) {
-
-    $getTrackerURL = BaseClickTracker::$getTrackerURL;
-
     return self::replaceTextUrls($msg,
-      function ($url) use ($mailing_id, $queue_id, $getTrackerURL) {
-        if (strpos($url, '{') !== FALSE) {
-          $data = BaseClickTracker::getTrackerURLForUrlWithTokens($url, $mailing_id, $queue_id);
-        }
-        else {
-          $data = $getTrackerURL($url, $mailing_id, $queue_id);
-        }
-        return $data;
+      function ($url) use ($mailing_id, $queue_id) {
+        return \CRM_Mailing_BAO_TrackableURL::getTrackerURL($url, $mailing_id,
+          $queue_id);
       }
     );
   }
diff --git a/ext/flexmailer/tests/phpunit/Civi/FlexMailer/ClickTrackerTest.php b/ext/flexmailer/tests/phpunit/Civi/FlexMailer/ClickTrackerTest.php
index f1e86a7d21..1f723c5557 100644
--- a/ext/flexmailer/tests/phpunit/Civi/FlexMailer/ClickTrackerTest.php
+++ b/ext/flexmailer/tests/phpunit/Civi/FlexMailer/ClickTrackerTest.php
@@ -1,4 +1,5 @@
 <?php
+namespace Civi\FlexMailer;
 
 use Civi\Test\HeadlessInterface;
 use Civi\Test\HookInterface;
@@ -6,14 +7,13 @@ use Civi\Test\TransactionalInterface;
 
 use Civi\FlexMailer\ClickTracker\TextClickTracker;
 use Civi\FlexMailer\ClickTracker\HtmlClickTracker;
-use Civi\FlexMailer\ClickTracker\BaseClickTracker;
 
 /**
  * Tests that URLs are converted to tracked ones if at all possible.
  *
  * @group headless
  */
-class Civi_FlexMailer_ClickTrackerTest extends \PHPUnit\Framework\TestCase implements HeadlessInterface, HookInterface, TransactionalInterface {
+class ClickTrackerTest extends \PHPUnit\Framework\TestCase implements HeadlessInterface, HookInterface, TransactionalInterface {
 
   protected $mailing_id;
 
@@ -27,16 +27,17 @@ class Civi_FlexMailer_ClickTrackerTest extends \PHPUnit\Framework\TestCase imple
 
   public function setUp() {
     // Mock the getTrackerURL call; we don't need to test creating a row in a table.
-    BaseClickTracker::$getTrackerURL = function($a, $b, $c) {
-      return 'http://example.com/extern?u=1&qid=1';
-    };
-
+    // If you want this to work without runkit, then either (a) make the dummy rows or (b) switch this to a hook/event that is runtime-configurable.
+    require_once 'CRM/Mailing/BAO/TrackableURL.php';
+    runkit7_method_rename('\CRM_Mailing_BAO_TrackableURL', 'getBasicTrackerURL', 'orig_getBasicTrackerURL');
+    runkit7_method_add('\CRM_Mailing_BAO_TrackableURL', 'getBasicTrackerURL', '$a, $b, $c', 'return \'http://example.com/extern?u=1&qid=1\';', RUNKIT7_ACC_STATIC | RUNKIT7_ACC_PRIVATE);
     parent::setUp();
   }
 
   public function tearDown() {
     // Reset the class.
-    BaseClickTracker::$getTrackerURL = ['CRM_Mailing_BAO_TrackableURL', 'getTrackerURL'];
+    runkit7_method_remove('\CRM_Mailing_BAO_TrackableURL', 'getBasicTrackerURL');
+    runkit7_method_rename('\CRM_Mailing_BAO_TrackableURL', 'orig_getBasicTrackerURL', 'getBasicTrackerURL');
     parent::tearDown();
   }
 
diff --git a/ext/flexmailer/tests/phpunit/Civi/FlexMailer/FlexMailerSystemTest.php b/ext/flexmailer/tests/phpunit/Civi/FlexMailer/FlexMailerSystemTest.php
index 5e4504ca70..a5b7d5c2b2 100644
--- a/ext/flexmailer/tests/phpunit/Civi/FlexMailer/FlexMailerSystemTest.php
+++ b/ext/flexmailer/tests/phpunit/Civi/FlexMailer/FlexMailerSystemTest.php
@@ -119,7 +119,16 @@ class FlexMailerSystemTest extends \CRM_Mailing_BaseMailingSystemTest {
    */
   public function urlTrackingExamples() {
     $cases = parent::urlTrackingExamples();
-    unset($cases[6]);
+
+    // When it comes to URLs with embedded tokens, support diverges - Flexmailer
+    // can track them, but BAO mailer cannot.
+    $cases[6] = [
+      '<p><a href="http://example.net/?id={contact.contact_id}">Foo</a></p>',
+      ';<p><a href=[\'"].*(extern/url.php|civicrm/mailing/url)(\?|&amp\\;)u=\d+.*&amp\\;id=\d+.*[\'"]>Foo</a></p>;',
+      ';\\[1\\] .*(extern/url.php|civicrm/mailing/url)[\?&]u=\d+.*&id=\d+.*;',
+      ['url_tracking' => 1],
+    ];
+
     return $cases;
   }