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-18221 filter relativize with realpath function to properly configure [civicrm.root] and [civicrm.files] on hosts with symlinked file systems #8494

Merged
merged 1 commit into from
Jun 21, 2016

Conversation

kcristiano
Copy link
Member

  • filter relativize with realpath function to properly configure [civicrm.root] and [civicrm.files] on hosts with symlinked file systems
  • style cleanups

- filter relativize with realpath function to properly configure [civicrm.root] and [civicrm.files] on hosts with symlinked file systems
- style cleanups

----------------------------------------
* CRM-18221: CiviCRM Resource URL causing issues on initial install with WP
  https://issues.civicrm.org/jira/browse/CRM-18221
@kcristiano kcristiano changed the title CRM-18221 CRM-18221 filter relativize with realpath function to properly configure [civicrm.root] and [civicrm.files] on hosts with symlinked file systems Jun 5, 2016
@eileenmcnaughton
Copy link
Contributor

How does this patch relate to #8466 ?

@kcristiano
Copy link
Member Author

@eileenmcnaughton #8466 looks to be a just a nudge to users that have upgraded to 4.7 to make use of the new more portable formats.

This patch fixes issues we've encountered with WP on hosts with symlinked directories. I would like to see both merged, but I do not see a dependancy.

@alifrumin
Copy link
Contributor

@kcristiano and I tested this patch on a local wordpress build with sym-links the patch is safe it only fixes a use case on one particular host, when we expanded the use case it doesn't work, @kcristiano is going to look at it some more.

@kcristiano
Copy link
Member Author

kcristiano commented Jun 7, 2016

Thanks @alifrumin The patch was designed for one host in mind WP Engine and it works there. Appreciate the testing as I want this to work on any host that has a symlinked or oddly constructed file system.

https://issues.civicrm.org/jira/browse/CRM-18221?focusedCommentId=90157&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-90157

@kcristiano
Copy link
Member Author

Discussed this Issue/PR at the Ft Collins Sprint with @colemanw and @totten

This issue and the corresponding PR solves the issue at WP Engine. WP Engine is mounting NAS storage in the same tree and the cleanup in the PR allows the install to work correctly and CiviCRM operate as normal.

However, in testing CiviCRM in the various symlink scenarios detailed above, this path is not a solution. For that we will need the override detailed above. At this point I do not think we can detect a symlink during install to enable the setting, so this needs to be a fix in documentation.

My opinion is we should still merge the PR as it does solve the issue present at WP Engine. It also has no effect on any other standard installations.

@JoeMurray
Copy link
Contributor

Heh Kevin as Release Manager this month, I'm trying to recruit people to help pare down the backlog of almost 100 PRs, some going back to last summer. I'm wondering if you would be able to help QA another PR if I got someone to QA this PR and your other ones like #8536 and WordPress #103 ?

@jmilone
Copy link

jmilone commented Jun 14, 2016

I just encountered the issue which this patch fixes on another WordPress host (not WP Engine). They are build on top of Amazon and appear to have a similar architecture. Same problem, same fix works. Just wanted to report.

@kcristiano
Copy link
Member Author

@jmilone Thanks for the report. I am sure that there are other hosts/server configs that need this. WP Engine is the only common thread I had before and I could duplicate it there. I'd like to see this merged for the next release.

@JoeMurray I am glad to help on QA, my only reluctance is i am abroad 6/17 until 6/28.

@eileenmcnaughton
Copy link
Contributor

Kevin - I feel unclear what conclusion you reached with @totten & @colemanw - did they agree 'better out than in' in discussion - or is there still something to resolve here

@kcristiano
Copy link
Member Author

@eileenmcnaughton The end result was better out than in. I don't think we have anything else to resolve here, I just wanted to give them a chance to chime in.

@colemanw
Copy link
Member

Err, did you two mean "better in than out?" - I thought that was the thrust of the conversation.

@eileenmcnaughton
Copy link
Contributor

Well who cares what I meant ... is this better in than out?

@colemanw
Copy link
Member

Based on the comments above I'd say "in" but I don't have in-depth knowledge about the change. Not sure if @totten knows more about it.

@JoeMurray
Copy link
Contributor

The code looks good to me, and the testing is working out. Kevin is gone till June 28 and I think we're getting caught on typos as the rest of his comments suggest in rather than out. Could we merge this?

@eileenmcnaughton
Copy link
Contributor

OK - I agree - I think people did a load of digging & decided it was not the perfect fix but it was an improvement. Merging

@eileenmcnaughton eileenmcnaughton merged commit a46aac2 into civicrm:master Jun 21, 2016
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.

7 participants