From 4987909f92ca145305bf74c3958d4ad7e63ebaae Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Tue, 25 Jul 2023 15:13:10 -0700 Subject: [PATCH] Civi::url() - Simplify/normalize "fragment" and "relative" handling. Fix some edge-case bugs. --- Civi/Core/Url.php | 49 ++++++++++++++------------ tests/phpunit/Civi/Core/UrlTest.php | 4 +++ tests/phpunit/E2E/Core/PathUrlTest.php | 4 +++ 3 files changed, 34 insertions(+), 23 deletions(-) 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'])) { -- 2.25.1