From: Tim Otten <totten@civicrm.org>
Date: Wed, 22 Jan 2020 05:14:28 +0000 (-0800)
Subject: Make $civicrm_paths less sensitive to trailing slashes. Add tests.
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=232fdd3dfac4ea09749d7f178818139514e48e38;p=civicrm-core.git

Make $civicrm_paths less sensitive to trailing slashes. Add tests.

Overview
--------

Recall that the default value of `imageUploadURL` is `[civicrm.files]/persist/contribute/`. Now, suppose you have one of these two configurations in `civicrm.settings.php`:

```php
// (1) Trailing slash configuration
$civicrm_paths['civicrm.files']['url'] = 'http://tmpd8-clean.bknix:8001/sites/default/files/civicrm/';
$civicrm_paths['civicrm.files']['path'] = '/Users/totten/bknix/build/tmpd8-clean/web/sites/default/files/civicrm/';

// (2) Trimmed slash configuration
$civicrm_paths['civicrm.files']['url'] = 'http://tmpd8-clean.bknix:8001/sites/default/files/civicrm';
$civicrm_paths['civicrm.files']['path'] = '/Users/totten/bknix/build/tmpd8-clean/web/sites/default/files/civicrm';
```

You could inspect to see if the URL is generated correctly by running these commands:

```
$ cv api setting.get return=imageUploadURL
$ cv url -c imageUploadURL
$ cv url -d "[civicrm.files]/persist/contribute/"
$ cv path -c imageUploadDir
$ cv path -d "[civicrm.files]/persist/contribute/"
```

Before
------

Under either configuration (trailing-slash or trimmed-slash), you'll find that paths are generated properly (`cv path ...` or `Civi::paths()->getPath(...)`).

For generating URLs (`cv url ...` or `Civi::paths()->getUrl(...)`), only the trailing-slash cfg works.  The trimmed-slash cfg leads to a bad URL with a missing slash.

```
[bknix-max:~/bknix/build/tmpd8prj-clean/web] cv api setting.get return=imageUploadURL
{
    "is_error": 0,
    "version": 3,
    "count": 1,
    "id": 1,
    "values": {
        "1": {
            "imageUploadURL": "[civicrm.files]/persist/contribute/"
        }
    }
}

[bknix-max:~/bknix/build/tmpd8prj-clean/web] cv url -c imageUploadURL
"http://tmpd8prj-clean.bknix:8001/sites/default/files/civicrmpersist/contribute"

[bknix-max:~/bknix/build/tmpd8prj-clean/web] cv url -d "[civicrm.files]/persist/contribute/"
"http://tmpd8prj-clean.bknix:8001/sites/default/files/civicrmpersist/contribute/"
```

This is surprising because the path expression (`[civicrm.files]/persist/contribute`) includes a slash...  but it disappears in the final computation.

After
-----

Both configurations work, for paths and for URLs.
---

diff --git a/Civi/Core/Paths.php b/Civi/Core/Paths.php
index a4ab40c220..a5bce6a6e7 100644
--- a/Civi/Core/Paths.php
+++ b/Civi/Core/Paths.php
@@ -220,7 +220,7 @@ class Paths {
       return $value;
     }
 
-    $value = $this->getVariable($defaultContainer, 'url') . $value;
+    $value = rtrim($this->getVariable($defaultContainer, 'url'), '/') . '/' . $value;
 
     if ($preferFormat === 'relative') {
       $parsed = parse_url($value);
diff --git a/tests/phpunit/Civi/Core/PathsTest.php b/tests/phpunit/Civi/Core/PathsTest.php
new file mode 100644
index 0000000000..bd5cbbd07d
--- /dev/null
+++ b/tests/phpunit/Civi/Core/PathsTest.php
@@ -0,0 +1,88 @@
+<?php
+
+namespace Civi\Core;
+
+/**
+ * Class PathsTest
+ * @package Civi\Core
+ * @group headless
+ */
+class PathsTest extends \CiviUnitTestCase {
+
+  public function getExamples() {
+    $exs = [];
+
+    // Ensure that various permutations of `$civicrm_paths`, `Civi::paths()->getPath()`
+    // and `Civi::paths()->getUrl()` work as expected.
+
+    // Trailing-slash configurations -- these all worked before current patch
+
+    $exs[] = ['te.st', 'path', '/var/www/files/', '[te.st]/foo/bar', '/var/www/files/foo/bar'];
+    $exs[] = ['te.st', 'path', '/var/www/files/', '[te.st]/foo/', '/var/www/files/foo/'];
+    $exs[] = ['te.st', 'path', '/var/www/files/', '[te.st]/foo', '/var/www/files/foo'];
+    $exs[] = ['te.st', 'path', '/var/www/files/', '[te.st]/.', '/var/www/files/'];
+
+    $exs[] = ['te.st', 'url', 'http://example.com/files/', '[te.st]/foo/bar', 'http://example.com/files/foo/bar'];
+    $exs[] = ['te.st', 'url', 'http://example.com/files/', '[te.st]/foo/', 'http://example.com/files/foo/'];
+    $exs[] = ['te.st', 'url', 'http://example.com/files/', '[te.st]/foo', 'http://example.com/files/foo'];
+    $exs[] = ['te.st', 'url', 'http://example.com/files/', '[te.st]/.', 'http://example.com/files/'];
+
+    $exs[] = ['te.st', 'url', 'http://example.com:8080/', '[te.st]/foo/bar', 'http://example.com:8080/foo/bar'];
+    $exs[] = ['te.st', 'url', 'http://example.com:8080/', '[te.st]/foo/', 'http://example.com:8080/foo/'];
+    $exs[] = ['te.st', 'url', 'http://example.com:8080/', '[te.st]/foo', 'http://example.com:8080/foo'];
+    $exs[] = ['te.st', 'url', 'http://example.com:8080/', '[te.st]/.', 'http://example.com:8080/'];
+
+    // Trimmed-slash configurations -- some of these worked before, and some misbehaved. Now fixed.
+
+    $exs[] = ['te.st', 'path', '/var/www/files', '[te.st]/foo/bar', '/var/www/files/foo/bar'];
+    $exs[] = ['te.st', 'path', '/var/www/files', '[te.st]/foo/', '/var/www/files/foo/'];
+    $exs[] = ['te.st', 'path', '/var/www/files', '[te.st]/foo', '/var/www/files/foo'];
+    $exs[] = ['te.st', 'path', '/var/www/files', '[te.st]/.', '/var/www/files/'];
+
+    $exs[] = ['te.st', 'url', 'http://example.com/files', '[te.st]/foo/bar', 'http://example.com/files/foo/bar'];
+    $exs[] = ['te.st', 'url', 'http://example.com/files', '[te.st]/foo/', 'http://example.com/files/foo/'];
+    $exs[] = ['te.st', 'url', 'http://example.com/files', '[te.st]/foo', 'http://example.com/files/foo'];
+    $exs[] = ['te.st', 'url', 'http://example.com/files', '[te.st]/.', 'http://example.com/files/'];
+
+    $exs[] = ['te.st', 'url', 'http://example.com:8080', '[te.st]/foo/bar', 'http://example.com:8080/foo/bar'];
+    $exs[] = ['te.st', 'url', 'http://example.com:8080', '[te.st]/foo/', 'http://example.com:8080/foo/'];
+    $exs[] = ['te.st', 'url', 'http://example.com:8080', '[te.st]/foo', 'http://example.com:8080/foo'];
+    $exs[] = ['te.st', 'url', 'http://example.com:8080', '[te.st]/.', 'http://example.com:8080/'];
+
+    return $exs;
+  }
+
+  /**
+   * @param $varName
+   * @param $varType
+   * @param $varValue
+   * @param $inputExpr
+   * @param $expectValue
+   * @dataProvider getExamples
+   */
+  public function testExamples($varName, $varType, $varValue, $inputExpr, $expectValue) {
+    global $civicrm_paths;
+    $civicrm_paths[$varName][$varType] = $varValue;
+    $func = ($varType === 'url') ? 'getUrl' : 'getPath';
+
+    $paths = new Paths();
+    $paths->register($varName, function() {
+      return ['path' => 'FIXME-PATH', 'url' => 'FIXME-URL'];
+    });
+
+    $actualValue = call_user_func([$paths, $func], $inputExpr);
+    $this->assertEquals($expectValue, $actualValue);
+
+    unset($civicrm_paths[$varName][$varType]);
+  }
+
+  public function testGetUrl_ImplicitBase() {
+    $p = \Civi::paths();
+    $cmsRoot = rtrim($p->getVariable('cms.root', 'url'), '/');
+
+    $this->assertEquals("$cmsRoot/foo/bar", $p->getUrl('foo/bar'));
+    $this->assertEquals("$cmsRoot/foo/", $p->getUrl('foo/'));
+    $this->assertEquals("$cmsRoot/foo", $p->getUrl('foo'));
+  }
+
+}