Allow most values of $civicrm_paths['XXX']['url'] to be relative #16403
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
Overview -------- The `$civicrm_paths` variable allows a sysadmin to override various path and URL computations. ```php $civicrm_paths['civicrm.packages']['url'] = 'https://example.com/libraries/civicrm/packages'; ``` The variable was originally tested with absolute URLs, and the subsequent examples/docs use absolute URLs (https://docs.civicrm.org/dev/en/latest/framework/filesystem/). These values are used to generate addresses, as in: ```php $abs = Civi::paths()->getUrl('[civicrm.packages]/foo.js', 'absolute'); $rel = Civi::paths()->getUrl('[civicrm.packages]/foo.js', 'relative'); ``` The patch allows more values in `$civicrm_paths` while ensuring that `getUrl()` works as expected. Before ------ The `getUrl()` requests only behave correctly if the override is an absolute URL - not if it's relative. After ----- The `getUrl()` requests behave correctly if the override is either an absolute URL or a relative URL. ```php $civicrm_paths['civicrm.packages']['url'] = 'https://example.com/libraries/civicrm/packages'; $civicrm_paths['civicrm.packages']['url'] = '/libraries/civicrm/packages'; ``` Comments -------- * `toAbsoluteUrl()` needs a base to prepend. I initially used `HTTP_HOST` but switched to `cms.root`, but correctly inferring scheme and host and port and httpd prefixes would be more complex - esp for background/CLI jobs. Using `cms.root` as the base is simpler. * It's tempting to allow recursive variables. But it's not actually needed for my purposes, and it would add complexity/maintenance. If it's really needed, one could update `toAbsoluteUrl()` to quickly check for variables (`$url[0] === '['`) and then evaluate them. But for now... I think the simpler format is fine.
1. The expected value of `$civicrmBaseUrl` should not be blank. The old value was a fiction of the unit-test environment. Be more realistic. 2. The expected values for extension-generated URLs should abide the same `$civicrmBaseUrl` as others.
This will allow E2E matrix to send a signal about whether the default configurations produce valid URLs.
|
(Standard links)
|
|
It's mostly tests! new function is well tested & only used in edge cases - merging |
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
The
$civicrm_pathsvariable allows a sysadmin to override various path and URL computations.These values are used to generate addresses, as in:
The variable was originally tested with absolute URLs, and the subsequent examples/docs use absolute URLs (https://docs.civicrm.org/dev/en/latest/framework/filesystem/).
The patch allows more values in
$civicrm_pathswhile ensuring thatgetUrl()works as expected.Before
The
getUrl()requests only behave correctly if the override is an absolute URL - not if it's relative.After
The
getUrl()requests behave correctly if the override is either an absolute URL or a relative URL.Comments
toAbsoluteUrl()needs a base to prepend. I initially usedHTTP_HOSTbut switched tocms.root. Why? Correctly inferring scheme and host and port and path prefixes would be more complex - esp when you consider background/CLI jobs and firewalls/proxies. Usingcms.rootas the base is simpler. This leaves us with a limitation -- if one were to override$civicrm_packages['cms.root']['url'], then that one would still need to be an absolute URL. However, that's no worse than today and doesn't impede my use-case.