dev/core#1637 - Multiple fixes for Civi/Core/Paths.php

[totten]

Overview

This fixes multiple edge-case issues in the evaluation of Civi::paths()->getUrl($expr) and Civi::paths()->getPath($expr).

Before + After

Describing the before+after is a bit daunting because we're in bouncy-bug territory - i.e. 5.22.x had an issue fixed in 5.23.0, but 5.23.0 caused a regression/new issue, so there's a pending revert in the unreleased HEADs of 5.24.beta1/5.23.1 which fixes the latter, but it rebreaks the former. Thus, we have multiple issues and multiple revisions to consider in "Before"+"After".

Instead of a simple Before+After, think of this as a grid. The columns of the grid are different git-branches/git-tags, and the rows are the use-cases for Civi::paths(). Each row has an example of an $expr that you could pass to Civi::paths()->getUrl($expr) and Civi::paths()->getPath($expr).

Row	Expression	5.22.1	5.23.0	5.24 (HEAD)	5.24 (This PR)
A	[myvar]/myfile	BAD: Sometimes ignores / from /myfile	GOOD: Keeps / from /myfile	BAD: Sometimes ignores / from /myfile	GOOD: Keeps / from /myfile
B	[myvar]/.	BAD: Ending varies	BAD: Ending always has trailing /	BAD: Ending varies	GOOD: Ending never has trailing /
C	[wp.backend]/.	GOOD: Ends with admin.php	BAD: Ends with admin.php/	GOOD: Ends with admin.php	GOOD: Ends with admin.php
D	[myvar]/0	BAD: Returns false	BAD: Returns false	BAD: Returns false	GOOD: Returns configured value of [myvar] plus /0
E	[myvar]/	BAD: Returns false	BAD: Returns false	BAD: Returns false	GOOD: Returns the value of [myvar] with one trailing slash
F	[myvar]/http-1.1-rfc2616.txt	BAD: Returns malformed URL (http-1.1-rfc2616.txt)	BAD: Returns malformed URL (http-1.1-rfc2616.txt)	BAD: Returns malformed URL (http-1.1-rfc2616.txt)	GOOD: Returns configured value of [myvar] plus /http-1.1-rfc2616.txt

This patch generally un-reverts #16404/#16713 to provide a less bouncy patch - i.e. the revert fixed (C) but rebroke (A) - and this patch fixes both (A) and (C). Strictly speaking, I can see the argument that buggy (A) is preferable over buggy (C) (because (A) bug is older), but I think it's preferrable to fix both.

Technical Details

(1) IMHO, the best way to think about this functionality is to work through the list of examples in Civi\Core\PathsTest. I can give prose to discuss specific situations if needed, but using prose for all of these edge-cases would be laborious for both reader and writer, so let's first try the table+code and then raise comments as needed.

(2) It may help to note some history of Civi::paths()->getUrl($expr) and getPath($expr). Originally, an $expr was simply the name of a file (e.g. getPath('README')), and you had the option to use a prefix or variable (e.g. getPath('[civicrm.root]/README')). That means each $expr would have two parts (a prefix-part and a file-part). But over time, more prefixes/variables/use-cases arose, and now we have cases where there is only a prefix-part (e.g. [wp.backend]/. or [civicrm.l10n]/.) and no substantive file-part. There's been an undocumented trick to coerce it to use a trivial file-part (i.e. ., the dot-file, an alias for the self-path).

(3) My general feeling is that the behavior of getUrl() when there's no substantive file-part has been inappropriate for a long time.

• For 16404, I took a lazy route and tried to just lock-in the existing behavior of dot-file trick on the most plausible use-case I saw (assuming that all other active users were similar) - but the WP regression shows the problem was harder. The dot-file behavior was previously inconsistent. Witness:WordPress.php expected that [wp.backend]/. would not return a trailing slash, and I18n.php expected that [civicrm.l10n]/. would return a slash. Witness: tutorial.php and Resources.php were already defensive/mistrusting.

• With inconsistent behavior, it's not enough to just read the output from an example and lock it into a test. So this fix pulls out heavier guns. Paragraph #4 asks "What behavior makes more sense as user of getUrl/getPath?", and #5 demonstrates by exhaustion that the chosen behavior is compatible with various use-cases..

(4) If you call getUrl($expr) and getPath($expr) with a non-substantive file-part, what would the caller expect as the result?

• [myvar]/ => You would expect this to yield myvar with one trailing slash (so that you can append subdirs/files). The patch makes these varying examples behave interchangeably as you would intuit.

  // Similar/interchangeable definitions of $myDir
  $myDir = '/var/www/foo/';
  $myDir = "{$wwwVar}/foo/";
  $myDir = Civi::paths()->getUrl('[myvar]/');
  
  // Similar/interchangeable definitions of $readmeFile
  $readmeFile = $myDir . 'README';
  $readmeFile = "{$myDir}README";

• [myvar]/. => You would expect this to yield myvar in a form that needs another /. The patch makes these varying examples behave interchangeably as you would intuit.

  // Similar/interchangeable definitions of $myDir
  $myDir = '/var/www/foo';
  $myDir = '/var/www/foo/.';
  $myDir = "{$wwwVar}/foo";
  $myDir = "{$wwwVar}/foo/.";
  $myDir = Civi::paths()->getUrl('[myvar]/.');
  
  // Similar/interchangeable definitions of $readmeFile
  $readmeFile = $myDir . '/README';
  $readmeFile = "{$myDir}/README";

• [myvar] => This still does not behave as you'd expect, and arguably is unreasonable, but it does behave exactly as it has in previous releases. Not on critical path of current bugs.

(5) IMHO, the most substantive change/flipflop is in the handling of the . dotfile.

• In 5.22, [myvar]/. was already wishywashy - it may or may not produce a trailing / - depending on the specific variable/configuration.
• For Make $civicrm_paths less sensitive to trailing slashes. Add tests. #16404, I didn't really care if [myvar]/. ended with / -- I just wanted it to be consistent / defined / unit-tested.
• In dev/core#1637, it does matter if it ends in / (and, specifically, it's better without the trailing /).
• Since we're really looking at this now, I did some grepping on universe to find all code which relied on the [myvar]/. notation. These are the things which came up:

  org.civicrm.tutorial/tutorial.php                       <== already defensive; conditionally appends trailing "/"
  civicrm-wordpress/civicrm.php                           <== part of the issue under investigation; prefers no trailing "/"; not defensively written
  civicrm-core/CRM/Utils/System/WordPress.php             <== part of the issue under investigation; prefers no trailing "/"; not defensively written
  civicrm-core/CRM/Core/Resources.php                     <== already defensive; conditionally appends trailing "/"
  civicrm-core/CRM/Core/I18n.php                          <== new use-case in 5.23; updated in this PR to be more defensive
  civicrm-core/tests/phpunit/Civi/Core/PathsTest.php      <== new unit-test in 5.23; updated in PR
  civicrm-core/tests/phpunit/E2E/Extern/CliRunnerTest.php <== unit-test; updated to be more defensive
  

• With the list, we can demonstrate (by exhaustion) that all use-cases are either defensively written (tutorial.php, Resources.php, I18n.php, CliRunnerTest.php) or compliant with the newer contract (WordPress.php, civicrm.php, PathsTest.php).

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[totten]

(Edited description)

[totten]

jenkins, test this please

[kcristiano]

I did r-run on WP and this fixes the issue as described.

I also tested on Joomla and it did not change the issue in dev/financial/120

Drupal 7 had no issues with the patch applied