-
-
Notifications
You must be signed in to change notification settings - Fork 814
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dev/core#1637 - Multiple fixes for Civi/Core/Paths.php #16735
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…s. Add tests."" This reverts commit cbcbfd6.
…URLs without trailing /
When browsing the list of outputs from this test class, each of the test cases was identified by its numerical position in the list of `$exs`. This makes it hard to keep track of the failures. 1. Add a symbolic name to each case (that's easier to search on) 2. Add more verbose output for failed assertions
The rationale will be discussed more via PR description.
For a full write-up, see PR
(Standard links)
|
The interpretation of `/.` is evolving per civicrm#16735: * When this code was first written, it was unspecified/variable whether the value `[foo]/.` would end in `/` * During most of the testing of 5.23.beta1, this was defined to always return `/` * During a regression fix in 5.23.1, we're flipping it back the other way so that `[foo]/.` never ends in `/`.
(Edited description) |
Per 16735, the interpretation of `getPath('[foo]/.`)` changed slightly - from: * 5.22: Inconsistent/undefined tail end (may or may not have trailing `/`) * 5.23.0: Defined to always end with `/` * civicrm#16735: Defined to never end with `/`
jenkins, test this please |
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 |
totten
added a commit
to totten/civicrm-core
that referenced
this pull request
Mar 11, 2020
The interpretation of `/.` is evolving per civicrm#16735: * When this code was first written, it was unspecified/variable whether the value `[foo]/.` would end in `/` * During most of the testing of 5.23.beta1, this was defined to always return `/` * During a regression fix in 5.23.1, we're flipping it back the other way so that `[foo]/.` never ends in `/`.
totten
added a commit
to totten/civicrm-core
that referenced
this pull request
Mar 11, 2020
Per 16735, the interpretation of `getPath('[foo]/.`)` changed slightly - from: * 5.22: Inconsistent/undefined tail end (may or may not have trailing `/`) * 5.23.0: Defined to always end with `/` * civicrm#16735: Defined to never end with `/`
totten
added a commit
to totten/civicrm-core
that referenced
this pull request
Mar 16, 2020
These use-cases had been tested during PR dev for 5.23.alpha, but they regressed in 5.23.1. In 5.23.1's civicrm#16735, note item (5) and the flip-flop on `/.` Item (5) references some greps to find references `/.` For obscure reasons, the file `l10n.js.tpl` didn't match the greps.
totten
added a commit
to totten/civicrm-core
that referenced
this pull request
Mar 16, 2020
These use-cases had been tested during PR dev for 5.23.alpha, but they regressed in 5.23.1. In 5.23.1's civicrm#16735, note item (5) and the flip-flop on `/.` Item (5) references some greps to find references `/.` For obscure reasons, the file `l10n.js.tpl` didn't match the greps.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This fixes multiple edge-case issues in the evaluation of
Civi::paths()->getUrl($expr)
andCivi::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 toCivi::paths()->getUrl($expr)
andCivi::paths()->getPath($expr)
.[myvar]/myfile
/
from/myfile
/
from/myfile
/
from/myfile
/
from/myfile
[myvar]/.
/
/
[wp.backend]/.
admin.php
admin.php/
admin.php
admin.php
[myvar]/0
[myvar]
plus/0
[myvar]/
[myvar]
with one trailing slash[myvar]/http-1.1-rfc2616.txt
http-1.1-rfc2616.txt
)http-1.1-rfc2616.txt
)http-1.1-rfc2616.txt
)[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)
andgetPath($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, andI18n.php
expected that[civicrm.l10n]/.
would return a slash. Witness:tutorial.php
andResources.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 ofgetUrl
/getPath
?", and#5
demonstrates by exhaustion that the chosen behavior is compatible with various use-cases..(4) If you call
getUrl($expr)
andgetPath($expr)
with a non-substantive file-part, what would the caller expect as the result?[myvar]/
=> You would expect this to yieldmyvar
with one trailing slash (so that you can append subdirs/files). The patch makes these varying examples behave interchangeably as you would intuit.[myvar]/.
=> You would expect this to yieldmyvar
in a form that needs another/
. The patch makes these varying examples behave interchangeably as you would intuit.[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.[myvar]/.
was already wishywashy - it may or may not produce a trailing/
- depending on the specific variable/configuration.[myvar]/.
ended with/
-- I just wanted it to be consistent / defined / unit-tested./
(and, specifically, it's better without the trailing/
).universe
to find all code which relied on the[myvar]/.
notation. These are the things which came up:tutorial.php
,Resources.php
,I18n.php
,CliRunnerTest.php
) or compliant with the newer contract (WordPress.php
,civicrm.php
,PathsTest.php
).