Skip to content
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

CRM_Utils_Check - Suggest using [cms.root], etal #8466

Merged
merged 1 commit into from
Oct 14, 2016

Conversation

totten
Copy link
Member

@totten totten commented May 28, 2016

In "Admin => Resource URLs" and "Admin => Directories", sites may have a
confusing mix of absolute and relative paths, especially if they originated
in <=v4.6. This complicates migrations (e.g. moving to a new server or
syncing with a dev/staging system). In 4.7+, we can use more portable path
variables.

This is a soft suggestion (NOTICE) and not a validation error or warning.
There are obscure scenarios where one would want to use absolute addresses;
but for most sites, it's an extraneous maintenance cost.

In "Admin => Resource URLs" and "Admin => Directories", sites may have a
confusing mix of absolute and relative paths, especially if they originated
in <=v4.6.  This complicates migrations (e.g.  moving to a new server or
syncing with a dev/staging system).  In 4.7+, we can use more portable path
variables.

This is a soft suggestion (NOTICE) and not a validation error or warning.
There are obscure scenarios where one would want to use absolute addresses;
but for most sites, it's an extraneous maintenance cost.
@totten
Copy link
Member Author

totten commented May 28, 2016

IMHO, the main thing that needs review is the policy & verbiage -- e.g. how best to explain this to downstream admins. Someone like @PalanteJon or @agh1 might have good insight.

@agh1
Copy link
Contributor

agh1 commented May 28, 2016

The message in general is good, but you'll need to assure the admin that [civicrm.files] will actually resolve to the files directory they're using. Because your message will know their path and be able to resolve [civicrm.files], you could be more detailed and suggest the exact text they might want to enter. Likewise, you could check that the directory could actually be replaced safely--i.e. that [civicrm.files]/whatever would actually be the same as /whole/path/to/whatever--and issue a different message in that case.

@eileenmcnaughton
Copy link
Contributor

@agh1 Can you corner @totten & get this to the point we can merge it?

@colemanw
Copy link
Member

@totten can you please merge or close this?

@eileenmcnaughton
Copy link
Contributor

I'm going to merge this - I understand the messaging could be improved & encourage anyone who would like to take a stab (@agh1) to submit a follow up PR

@eileenmcnaughton eileenmcnaughton merged commit 0d418e2 into civicrm:master Oct 14, 2016
@totten totten deleted the master-path-warning branch January 3, 2017 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants