From: Tim Otten <totten@civicrm.org>
Date: Tue, 25 Jul 2023 22:13:10 +0000 (-0700)
Subject: Civi::url() - Simplify/normalize "fragment" and "relative" handling. Fix some edge... 
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=4987909f92ca145305bf74c3958d4ad7e63ebaae;p=civicrm-core.git

Civi::url() - Simplify/normalize "fragment" and "relative" handling. Fix some edge-case bugs.
---

diff --git a/Civi/Core/Url.php b/Civi/Core/Url.php
index 9ae35d2891..70eb99bbee 100644
--- a/Civi/Core/Url.php
+++ b/Civi/Core/Url.php
@@ -523,6 +523,7 @@ final class Url implements \JsonSerializable {
       $scheme = \CRM_Core_Menu::isPublicRoute($this->getPath()) ? 'frontend' : 'backend';
     }
 
+    // Goal: After this switch(), we should have the $scheme, $path, and $query combined.
     switch ($scheme) {
       case 'assetBuilder':
         $assetName = $this->getPath();
@@ -534,11 +535,8 @@ final class Url implements \JsonSerializable {
       case 'asset':
         if (preg_match(';^\[([\w\.]+)\](.*)$;', $this->getPath(), $m)) {
           [, $var, $rest] = $m;
-          $result = rtrim(\Civi::paths()->getVariable($var, 'url'), '/');
-          if ($preferFormat === 'relative') {
-            $result = \CRM_Utils_Url::toRelative($result);
-          }
-          $result .= $rest . $this->composeSuffix();
+          $varValue = rtrim(\Civi::paths()->getVariable($var, 'url'), '/');
+          $result = $varValue . $rest . $this->composeQuery();
         }
         else {
           throw new \RuntimeException("Malformed asset path: {$this->getPath()}");
@@ -547,7 +545,7 @@ final class Url implements \JsonSerializable {
 
       case 'ext':
         $parts = explode('/', $this->getPath(), 2);
-        $result = \Civi::resources()->getUrl($parts[0], $parts[1] ?? NULL, FALSE) . $this->composeSuffix();
+        $result = \Civi::resources()->getUrl($parts[0], $parts[1] ?? NULL, FALSE) . $this->composeQuery();
         break;
 
       // Handle 'frontend', 'backend', 'service', and any extras.
@@ -556,12 +554,6 @@ final class Url implements \JsonSerializable {
         if ($result === NULL) {
           throw new \RuntimeException("Unknown URL scheme: $scheme");
         }
-        if ($preferFormat === 'relative') {
-          $result = \CRM_Utils_Url::toRelative($result);
-        }
-        if ($fragment = $this->composeFragment()) {
-          $result .= '#' . $fragment;
-        }
         break;
     }
 
@@ -569,6 +561,12 @@ final class Url implements \JsonSerializable {
       $result = \Civi::resources()->addCacheCode($result);
     }
 
+    $result .= $this->composeFragment();
+
+    if ($preferFormat === 'relative') {
+      $result = \CRM_Utils_Url::toRelative($result);
+    }
+
     // TODO decide if the current default is good enough for future
     $ssl = $this->getSsl() ?: \CRM_Utils_System::isSSL();
     if ($ssl && str_starts_with($result, 'http:')) {
@@ -603,24 +601,29 @@ final class Url implements \JsonSerializable {
     return $this->__toString();
   }
 
-  private function composeSuffix(): string {
-    $result = '';
+  /**
+   * @return string
+   *   '' or '?foo=bar'
+   */
+  private function composeQuery(): string {
     if ($this->query !== NULL && $this->query !== '') {
-      $result .= '?' . $this->query;
+      return '?' . $this->query;
     }
-    $fragment = $this->composeFragment();
-    if ($fragment !== NULL && $fragment !== '') {
-      $result .= '#' . $fragment;
+    else {
+      return '';
     }
-    return $result;
   }
 
-  private function composeFragment(): ?string {
-    $fragment = $this->fragment;
+  /**
+   * @return string
+   *   '' or '#foobar'
+   */
+  private function composeFragment(): string {
+    $fragment = $this->fragment ?: '';
     if ($this->fragmentQuery !== NULL && $this->fragmentQuery !== '') {
-      $fragment = ($fragment ?: '') . '?' . $this->fragmentQuery;
+      $fragment .= '?' . $this->fragmentQuery;
     }
-    return $fragment;
+    return ($fragment === '') ? '' : "#$fragment";
   }
 
   private static function detectFormat(): string {
diff --git a/tests/phpunit/Civi/Core/UrlTest.php b/tests/phpunit/Civi/Core/UrlTest.php
index c12933c13a..4bbbb8812f 100644
--- a/tests/phpunit/Civi/Core/UrlTest.php
+++ b/tests/phpunit/Civi/Core/UrlTest.php
@@ -34,11 +34,15 @@ class UrlTest extends \CiviUnitTestCase {
     $absolutes = [];
     $absolutes['flag'] = Civi::url('backend://civicrm/admin', 'a');
     $absolutes['method'] = Civi::url('backend://civicrm/admin')->setPreferFormat('absolute');
+    $absolutes['ext'] = Civi::url('ext://org.civicrm.search_kit/js/foobar.js', 'a');
+    $absolutes['asset'] = Civi::url('asset://[civicrm.packages]/js/foobar.js', 'a');
 
     $relatives = [];
     $relatives['default'] = Civi::url('backend://civicrm/admin');
     $relatives['flag'] = Civi::url('backend://civicrm/admin', 'r');
     $relatives['method'] = Civi::url('backend://civicrm/admin')->setPreferFormat('relative');
+    $relatives['ext'] = Civi::url('ext://org.civicrm.search_kit/js/foobar.js', 'r');
+    $relatives['asset'] = Civi::url('asset://[civicrm.packages]/js/foobar.js', 'r');
 
     foreach ($absolutes as $key => $url) {
       $this->assertRegExp(';^https?://;', (string) $url, "absolutes[$key] should be absolute URL");
diff --git a/tests/phpunit/E2E/Core/PathUrlTest.php b/tests/phpunit/E2E/Core/PathUrlTest.php
index 98a316fa96..c08fb5650f 100644
--- a/tests/phpunit/E2E/Core/PathUrlTest.php
+++ b/tests/phpunit/E2E/Core/PathUrlTest.php
@@ -107,6 +107,10 @@ class PathUrlTest extends \CiviEndToEndTestCase {
 
     $urlPats[] = [';^https?://.*civicrm;', \Civi::url('frontend://civicrm/event/info?reset=1', 'a')];
     $urlPats[] = [';^https://.*civicrm;', \Civi::url('frontend://civicrm/event/info?reset=1', 'as')];
+    $urlPats[] = [';civicrm(/|%2F)a(/|%2F).*#/mailing/new\?angularDebug=1;', \Civi::url('backend://civicrm/a/#/mailing/new?angularDebug=1')];
+    $urlPats[] = [';/jquery.timeentry.js\?r=.*#foo;', \Civi::url('asset://[civicrm.packages]/jquery/plugins/jquery.timeentry.js', 'c')->addFragment('foo')];
+    $urlPats[] = [';/stuff.js\?r=.*#foo;', \Civi::url('ext://org.civicrm.search_kit/stuff.js', 'c')->addFragment('foo')];
+    $urlPats[] = [';#foo;', \Civi::url('assetBuilder://crm-l10n.js?locale=en_US')->addFragment('foo')];
 
     // Some test-harnesses have HTTP_HOST. Some don't. It's pre-req for truly relative URLs.
     if (!empty($_SERVER['HTTP_HOST'])) {