From d0cda11f761d4babb3fc515517dbbc0a80743338 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Mon, 23 Mar 2020 22:56:36 -0700 Subject: [PATCH] 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')); + } + }