From d0cda11f761d4babb3fc515517dbbc0a80743338 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Mon, 23 Mar 2020 22:56:36 -0700 Subject: [PATCH 1/2] dev/joomla#26 - Fix path derivation when CMS is rooted in a subdir Overview -------- CiviCRM is deployed inside a CMS. The CMS is usually deployed at the HTTP root (`http://example.org`), but it is sometimes deployed in a subdirectory (`http://example.org/my-cms`). Some asset URLs are computed using the variables `[civicrm.bower]`, `[civicrm.packages]`, and `[civicrm.vendor]`, which are derived from the value of `[civicrm.root]`. However, if the site is deployed in a subdirectory, and if using v5.23, then the computation of `[civicrm.bower]` (etc) can misbehave. Before ------ When the URL for `[civicrm.bower]` (or similar) is derived, it goes through multiple filters - first, from absolute to relative, and then later from relative back to absolute. In the process, the base is inadvertently changed. After ----- When the URL is derived, it is computed in absolute format - and simply kept that way. Comments -------- Regarding test coverage, there are two relevant unit-tests. This PR only updates one. * `E2E\Core\PathUrlTest`: This is a more concrete smoke test which demonstrates functional problems with variables like `[civicrm.bower]`. It should already catch problems like dev/joomla#26... but only if you run the E2E suite on a system where the CMS was deployed to a subdirectory. `civibuild` doesn't currently include such a build-type. * `Civi\Core\PathsTest`: This is a more abstract, headless test to examine edge-cases in `Civi\Core\Paths`. It does not specifically check for `[civicrm.bower]` or similar variables (b/c the URL routing is ill-defined in a headless context). However, I've updated it to compare/contrast the working and non-working ways to derive variables. --- Civi/Core/Paths.php | 6 ++-- tests/phpunit/Civi/Core/PathsTest.php | 48 +++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/Civi/Core/Paths.php b/Civi/Core/Paths.php index 9099c3cff95a..d0d15814b367 100644 --- a/Civi/Core/Paths.php +++ b/Civi/Core/Paths.php @@ -38,19 +38,19 @@ public function __construct() { ->register('civicrm.packages', function () { return [ 'path' => \Civi::paths()->getPath('[civicrm.root]/packages/'), - 'url' => \Civi::paths()->getUrl('[civicrm.root]/packages/'), + 'url' => \Civi::paths()->getUrl('[civicrm.root]/packages/', 'absolute'), ]; }) ->register('civicrm.vendor', function () { return [ 'path' => \Civi::paths()->getPath('[civicrm.root]/vendor/'), - 'url' => \Civi::paths()->getUrl('[civicrm.root]/vendor/'), + 'url' => \Civi::paths()->getUrl('[civicrm.root]/vendor/', 'absolute'), ]; }) ->register('civicrm.bower', function () { return [ 'path' => \Civi::paths()->getPath('[civicrm.root]/bower_components/'), - 'url' => \Civi::paths()->getUrl('[civicrm.root]/bower_components/'), + 'url' => \Civi::paths()->getUrl('[civicrm.root]/bower_components/', 'absolute'), ]; }) ->register('civicrm.files', function () { diff --git a/tests/phpunit/Civi/Core/PathsTest.php b/tests/phpunit/Civi/Core/PathsTest.php index ee56ca85d885..7df04dbaa483 100644 --- a/tests/phpunit/Civi/Core/PathsTest.php +++ b/tests/phpunit/Civi/Core/PathsTest.php @@ -101,4 +101,52 @@ public function testGetUrl_ImplicitBase() { $this->assertEquals("$cmsRoot/foo", $p->getUrl('foo')); } + /** + * This test demonstrates how to (and how not to) compute a derivative path variable. + */ + public function testAbsoluteRelativeConversions() { + $gstack = \CRM_Utils_GlobalStack::singleton(); + $gstack->push(['_SERVER' => ['HTTP_HOST' => 'example.com']]); + $cleanup = \CRM_Utils_AutoClean::with([$gstack, 'pop']); + + $paths = new Paths(); + $paths->register('test.base', function () { + return [ + 'path' => '/var/foo/', + 'url' => 'http://example.com/foo/', + ]; + }); + $paths->register('test.goodsub', function () use ($paths) { + return [ + 'path' => $paths->getPath('[test.base]/good/'), + 'url' => $paths->getUrl('[test.base]/good/', 'absolute'), + ]; + }); + $paths->register('test.badsub', function () use ($paths) { + return [ + 'path' => $paths->getPath('[test.base]/bad/'), + // This *looks* OK, but `getUrl()` by default uses `$preferFormat==relative`. + // The problem is that `$civicrm_paths['...']['url']` is interpreted as relative to CMS root, + // and `getUrl(..., 'relative')` is interpreted as relative to HTTP root. + // So this definition misbehaves on sites where the CMS root and HTTP root are different. + 'url' => $paths->getUrl('[test.base]/bad/'), + ]; + }); + + // The test.base works as explicitly defined... + $this->assertEquals('/var/foo', $paths->getPath('[test.base]/.')); + $this->assertEquals('http://example.com/foo', $paths->getUrl('[test.base]/.', 'absolute')); + $this->assertEquals('/foo', $paths->getUrl('[test.base]/.', 'relative')); + + // The test.goodsub works as expected... + $this->assertEquals('/var/foo/good', $paths->getPath('[test.goodsub]/.')); + $this->assertEquals('http://example.com/foo/good', $paths->getUrl('[test.goodsub]/.', 'absolute')); + $this->assertEquals('/foo/good', $paths->getUrl('[test.goodsub]/.', 'relative')); + + // The test.badsub doesn't work as expected. + $this->assertEquals('/var/foo/bad', $paths->getPath('[test.badsub]/.')); + $this->assertNotEquals('http://example.com/foo/bad', $paths->getUrl('[test.badsub]/.', 'absolute')); + $this->assertNotEquals('/foo/bad', $paths->getUrl('[test.badsub]/.', 'relative')); + } + } From 961b20062fceeb4ffe585d0cdc172da4598bebf9 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Tue, 24 Mar 2020 14:46:10 -0700 Subject: [PATCH 2/2] (NFC) Update comments in PathsTest.php --- tests/phpunit/Civi/Core/PathsTest.php | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/phpunit/Civi/Core/PathsTest.php b/tests/phpunit/Civi/Core/PathsTest.php index 7df04dbaa483..7415442386b6 100644 --- a/tests/phpunit/Civi/Core/PathsTest.php +++ b/tests/phpunit/Civi/Core/PathsTest.php @@ -117,18 +117,21 @@ public function testAbsoluteRelativeConversions() { ]; }); $paths->register('test.goodsub', function () use ($paths) { + // This is a stand-in for how [civicrm.bower], [civicrm.packages], [civicrm.vendor] currently work. return [ 'path' => $paths->getPath('[test.base]/good/'), 'url' => $paths->getUrl('[test.base]/good/', 'absolute'), ]; }); $paths->register('test.badsub', function () use ($paths) { + // This is a stand-in for how [civicrm.bower], [civicrm.packages], [civicrm.vendor] used to work (incorrectly). return [ 'path' => $paths->getPath('[test.base]/bad/'), - // This *looks* OK, but `getUrl()` by default uses `$preferFormat==relative`. - // The problem is that `$civicrm_paths['...']['url']` is interpreted as relative to CMS root, - // and `getUrl(..., 'relative')` is interpreted as relative to HTTP root. - // So this definition misbehaves on sites where the CMS root and HTTP root are different. + // The following *looks* OK, but it's not. Note that `getUrl()` by default uses `$preferFormat==relative`. + // Both registered URLs (`register()`, `$civicrm_paths`) and outputted URLs (`getUrl()`) + // can be in relative form. However, they are relative to different bases: registrations are + // relative to CMS root, and outputted URLs are relative to HTTP root. They are often the same, but... + // on deployments where they differ, this example will misbehave. 'url' => $paths->getUrl('[test.base]/bad/'), ]; });