Merge pull request #16744 from civicrm/5.24

5.24
civicrm · Mar 11, 2020 · c879543 · c879543
2 parents 79f3672 + 96269ae
commit c879543
Show file tree

Hide file tree

Showing 6 changed files with 163 additions and 18 deletions.
diff --git a/CRM/Core/I18n.php b/CRM/Core/I18n.php
@@ -282,7 +282,7 @@ public function strarg($str) {
    * @return string
    */
   public static function getResourceDir() {
-    return \Civi::paths()->getPath('[civicrm.l10n]/.');
+    return CRM_Utils_File::addTrailingSlash(\Civi::paths()->getPath('[civicrm.l10n]/.'));
   }
 
   /**

diff --git a/CRM/Event/BAO/Event.php b/CRM/Event/BAO/Event.php
@@ -2351,7 +2351,7 @@ public static function buildOptions($fieldName, $context = NULL, $props = []) {
         // @fixme - this is going to ignore context, better to get conditions, add params, and call PseudoConstant::get
         // @fixme - https://lab.civicrm.org/dev/core/issues/547 if CiviContribute not enabled this causes an invalid query
         //   because $relationTypeId is not set in CRM_Financial_BAO_FinancialType::getIncomeFinancialType()
-        if (array_key_exists('CiviEvent', CRM_Core_Component::getEnabledComponents())) {
+        if (array_key_exists('CiviContribute', CRM_Core_Component::getEnabledComponents())) {
           return CRM_Financial_BAO_FinancialType::getIncomeFinancialType();
         }
         return [];

diff --git a/Civi/Core/Paths.php b/Civi/Core/Paths.php
@@ -210,52 +210,65 @@ public function hasVariable($name) {
    * @return mixed|string
    */
   public function getPath($value) {
+    if ($value === NULL || $value === FALSE || $value === '') {
+      return FALSE;
+    }
+
     $defaultContainer = self::DEFAULT_PATH;
     if ($value && $value{0} == '[' && preg_match(';^\[([a-zA-Z0-9\._]+)\]/(.*);', $value, $matches)) {
       $defaultContainer = $matches[1];
       $value = $matches[2];
     }
-    if (empty($value)) {
-      return FALSE;
-    }
-    if ($value === '.') {
+
+    $isDot = $value === '.';
+    if ($isDot) {
       $value = '';
     }
-    return \CRM_Utils_File::absoluteDirectory($value, $this->getVariable($defaultContainer, 'path'));
+
+    $result = \CRM_Utils_File::absoluteDirectory($value, $this->getVariable($defaultContainer, 'path'));
+    return $isDot ? rtrim($result, '/' . DIRECTORY_SEPARATOR) : $result;
   }
 
   /**
    * Determine the URL to a file.
    *
    * @param string $value
    *   The file path. The path may begin with a variable, e.g. "[civicrm.files]/upload".
+   *
+   *   This function was designed for locating files under a given tree, and the
+   *   the result for a straight variable expressions ("[foo.bar]") was not
+   *   originally defined. You may wish to use one of these:
+   *
+   *   - getVariable('foo.bar', 'url') => Lookup variable by itself
+   *   - getUrl('[foo.bar]/') => Get the variable (normalized with a trailing "/").
+   *   - getUrl('[foo.bar]/.') => Get the variable (normalized without a trailing "/").
    * @param string $preferFormat
    *   The preferred format ('absolute', 'relative').
    *   The result data may not meet the preference -- if the setting
    *   refers to an external domain, then the result will be
    *   absolute (regardless of preference).
    * @param bool|NULL $ssl
    *   NULL to autodetect. TRUE to force to SSL.
-   * @return mixed|string
+   * @return FALSE|string
+   *   The URL for $value (string), or FALSE if the $value is not specified.
    */
   public function getUrl($value, $preferFormat = 'relative', $ssl = NULL) {
+    if ($value === NULL || $value === FALSE || $value === '') {
+      return FALSE;
+    }
+
     $defaultContainer = self::DEFAULT_URL;
     if ($value && $value{0} == '[' && preg_match(';^\[([a-zA-Z0-9\._]+)\](/(.*))$;', $value, $matches)) {
       $defaultContainer = $matches[1];
-      $value = empty($matches[3]) ? '.' : $matches[3];
+      $value = $matches[3];
     }
 
-    if (empty($value)) {
-      return FALSE;
-    }
-    if ($value === '.') {
-      $value = '';
-    }
-    if (substr($value, 0, 4) == 'http') {
+    $isDot = $value === '.';
+    if (substr($value, 0, 5) === 'http:' || substr($value, 0, 6) === 'https:') {
       return $value;
     }
 
-    $value = $this->getVariable($defaultContainer, 'url') . $value;
+    $value = rtrim($this->getVariable($defaultContainer, 'url'), '/') . ($isDot ? '' : "/$value");
 
     if ($preferFormat === 'relative') {
       $parsed = parse_url($value);

diff --git a/tests/phpunit/Civi/Core/PathsTest.php b/tests/phpunit/Civi/Core/PathsTest.php
@@ -0,0 +1,104 @@
+<?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
+
+    $exs['ap1'] = ['te.st', 'path', '/var/www/files/', '[te.st]/foo/bar', '/var/www/files/foo/bar'];
+    $exs['ap2'] = ['te.st', 'path', '/var/www/files/', '[te.st]/foo/', '/var/www/files/foo/'];
+    $exs['ap3'] = ['te.st', 'path', '/var/www/files/', '[te.st]/foo', '/var/www/files/foo'];
+    $exs['ap4'] = ['te.st', 'path', '/var/www/files/', '[te.st]/.', '/var/www/files'];
+    $exs['ap5'] = ['te.st', 'path', '/var/www/files/', '[te.st]/0', '/var/www/files/0'];
+    $exs['ap6'] = ['te.st', 'path', '/var/www/files/', '[te.st]/', '/var/www/files/'];
+
+    $exs['au1'] = ['te.st', 'url', 'http://example.com/files/', '[te.st]/foo/bar', 'http://example.com/files/foo/bar'];
+    $exs['au2'] = ['te.st', 'url', 'http://example.com/files/', '[te.st]/foo/', 'http://example.com/files/foo/'];
+    $exs['au3'] = ['te.st', 'url', 'http://example.com/files/', '[te.st]/foo', 'http://example.com/files/foo'];
+    $exs['au4'] = ['te.st', 'url', 'http://example.com/files/', '[te.st]/.', 'http://example.com/files'];
+    $exs['au5'] = ['te.st', 'url', 'http://example.com/files/', '[te.st]/0', 'http://example.com/files/0'];
+    $exs['au6'] = ['te.st', 'url', 'http://example.com/files/', '[te.st]/', 'http://example.com/files/'];
+
+    $exs['au18'] = ['te.st', 'url', 'http://example.com:8080/', '[te.st]/foo/bar', 'http://example.com:8080/foo/bar'];
+    $exs['au28'] = ['te.st', 'url', 'http://example.com:8080/', '[te.st]/foo/', 'http://example.com:8080/foo/'];
+    $exs['au38'] = ['te.st', 'url', 'http://example.com:8080/', '[te.st]/foo', 'http://example.com:8080/foo'];
+    $exs['au48'] = ['te.st', 'url', 'http://example.com:8080/', '[te.st]/.', 'http://example.com:8080'];
+    $exs['au58'] = ['te.st', 'url', 'http://example.com:8080/', '[te.st]/0', 'http://example.com:8080/0'];
+    $exs['au68'] = ['te.st', 'url', 'http://example.com:8080/', '[te.st]/', 'http://example.com:8080/'];
+
+    // Trimmed-slash configurations
+
+    $exs['bp1'] = ['te.st', 'path', '/var/www/files', '[te.st]/foo/bar', '/var/www/files/foo/bar'];
+    $exs['bp2'] = ['te.st', 'path', '/var/www/files', '[te.st]/foo/', '/var/www/files/foo/'];
+    $exs['bp3'] = ['te.st', 'path', '/var/www/files', '[te.st]/foo', '/var/www/files/foo'];
+    $exs['bp4'] = ['te.st', 'path', '/var/www/files', '[te.st]/.', '/var/www/files'];
+    $exs['bp5'] = ['te.st', 'path', '/var/www/files', '[te.st]/0', '/var/www/files/0'];
+    $exs['bp6'] = ['te.st', 'path', '/var/www/files', '[te.st]/', '/var/www/files/'];
+
+    $exs['bu1'] = ['te.st', 'url', 'http://example.com/files', '[te.st]/foo/bar', 'http://example.com/files/foo/bar'];
+    $exs['bu2'] = ['te.st', 'url', 'http://example.com/files', '[te.st]/foo/', 'http://example.com/files/foo/'];
+    $exs['bu3'] = ['te.st', 'url', 'http://example.com/files', '[te.st]/foo', 'http://example.com/files/foo'];
+    $exs['bu4'] = ['te.st', 'url', 'http://example.com/files', '[te.st]/.', 'http://example.com/files'];
+    $exs['bu5'] = ['te.st', 'url', 'http://example.com/files', '[te.st]/0', 'http://example.com/files/0'];
+    $exs['bu6'] = ['te.st', 'url', 'http://example.com/files', '[te.st]/', 'http://example.com/files/'];
+
+    $exs['bu18'] = ['te.st', 'url', 'http://example.com:8080', '[te.st]/foo/bar', 'http://example.com:8080/foo/bar'];
+    $exs['bu28'] = ['te.st', 'url', 'http://example.com:8080', '[te.st]/foo/', 'http://example.com:8080/foo/'];
+    $exs['bu38'] = ['te.st', 'url', 'http://example.com:8080', '[te.st]/foo', 'http://example.com:8080/foo'];
+    $exs['bu48'] = ['te.st', 'url', 'http://example.com:8080', '[te.st]/.', 'http://example.com:8080'];
+    $exs['bu58'] = ['te.st', 'url', 'http://example.com:8080', '[te.st]/0', 'http://example.com:8080/0'];
+    $exs['bu68'] = ['te.st', 'url', 'http://example.com:8080', '[te.st]/', 'http://example.com:8080/'];
+
+    // Oddballs
+    $exs['wp1'] = ['wp.ex1', 'url', 'http://example.com/wp-admin/admin.php', '[wp.ex1]/.', 'http://example.com/wp-admin/admin.php'];
+    $exs['http'] = ['te.st', 'url', 'http://example.com/files', '[te.st]/httpIsBetterThanGopher', 'http://example.com/files/httpIsBetterThanGopher'];
+
+    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, "Evaluate $func(\"$inputExpr\") given ([$varName] = \"$varValue\")");
+
+    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'));
+  }
+
+}
diff --git a/tests/phpunit/E2E/Core/PathUrlTest.php b/tests/phpunit/E2E/Core/PathUrlTest.php
@@ -88,4 +88,32 @@ private function assertFileContentRegex($expectContentRegex, $file) {
     $this->assertRegexp($expectContentRegex, $content);
   }
 
+  /**
+   * @link https://lab.civicrm.org/dev/core/issues/1637
+   */
+  public function testGetUrl_WpAdmin() {
+    $config = \CRM_Core_Config::singleton();
+    if ($config->userFramework !== 'WordPress') {
+      $this->markTestSkipped('This test only applies to WP sites.');
+    }
+
+    // NOTE: For backend admin forms (eg `civicrm/contribute`) on WP, it doesn't matter
+    // if cleanURL's are enabled. Those are always be dirty URLs.
+
+    // WORKAROUND: There's some issue where the URL gets a diff value in WP E2E env
+    // than in normal WP env. The `cv url` command seems to behave more
+    // representatively, though this technique is harder to inspect with xdebug.
+    $url = cv('url civicrm/contribute?reset=1');
+    // $url = \CRM_Utils_System::url('civicrm/contribute', 'reset=1', TRUE, NULL, FALSE);
+
+    $parts = parse_url($url);
+    parse_str($parts['query'], $queryParts);
+    $this->assertEquals('CiviCRM', $queryParts['page']);
+    $this->assertEquals('civicrm/contribute', $queryParts['q']);
+    $this->assertEquals('1', $queryParts['reset']);
+
+    // As an E2E test for wp-demo, this assertion is specifically valid for wp-demo.
+    $this->assertEquals('/wp-admin/admin.php', $parts['path']);
+  }
+
 }
diff --git a/tests/phpunit/E2E/Extern/CliRunnerTest.php b/tests/phpunit/E2E/Extern/CliRunnerTest.php
@@ -101,7 +101,7 @@ public function testBasicPathUrl($name, $r) {
 
     $ufrUrl = $this->callRunnerJson($r, 'CRM_Core_Config::singleton()->userFrameworkResourceURL');
     $crmUrl = $this->callRunnerJson($r, 'Civi::paths()->getUrl("[civicrm.root]/.")');
-    $this->assertEquals($crmUrl, $ufrUrl);
+    $this->assertEquals(rtrim($crmUrl, '/'), rtrim($ufrUrl, '/'));
   }
 
   /**