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

Ensure path returned by drush_server_home() does not have trailing slashes #858

Merged
merged 2 commits into from
Sep 30, 2014

Conversation

alexbarrett
Copy link

Fixes #848.

@greg-1-anderson
Copy link
Member

This looks pretty good to me. Do you think we should strip HOMEDRIVE as well, just to be defensive, or is this environment variable pretty well guaranteed to be in the form 'X:'? (i.e. if someone mistakenly sets it to 'X:', do other components in Windows break?)

@alexbarrett
Copy link
Author

Looking around it seems usual to concatenate those two environment variables directly so I'd say it's better to trust that they are valid.

greg-1-anderson added a commit that referenced this pull request Sep 30, 2014
Ensure path returned by drush_server_home() does not have trailing slashes
@greg-1-anderson greg-1-anderson merged commit 803ef26 into drush-ops:master Sep 30, 2014
@greg-1-anderson
Copy link
Member

Committed to master. I don't think we should backport to Drush 6, though; if anyone has mistakenly configured their environment such that drush_server_home() returns a trailing slash, this is usually innocuous, but their config files could break if they incorrectly used drush_server_home() . 'filename'; instead of drush_server_home() . '/filename';. We'll let this break in a major Drush upgrade, but not in a minor release.

@alexbarrett
Copy link
Author

What would be the recommended fix for Drush 6 users in my situation (homedrive+path mapped to a root directory)? It's a work environment so I can't modify these variables. I can of course keep patching Drush manually but it seems likely that more people are in a similar environment to myself than there are those using an API call incorrectly.

Options I can think of:

  • Add a default parameter to the function, e.g. drush_server_home($strip_slashes = FALSE), that Drush uses internally (there aren't many calls to it).
  • Add support for a DRUSH_HOME environment variable that we can set manually.
  • Backport the fix with a minor break in backwards compatibility for those using the call incorrectly.
  • Leave it. We will manage :)

@greg-1-anderson
Copy link
Member

I think that any of those options could be viable; #3 is my least favorite, but we do sometimes break people when necessary for bug fixes. I think #2 is best if we want to support DRUSH_HOME perpetually (indifferent about that); otherwise, #1 would be best.

Make a PR for Drush 6, make your best argument for the option you chose, and ping @weitzman for his opinion.

@greg-1-anderson
Copy link
Member

I thought of a fifth option. Make a new PR for Drush 6 that is just like this one, except leave out the part that trims the HOME environment variable. Thus, only Windows users are affected; Linux users will continue to get the same behavior. Only Windows users should need this fix, and probably only a small subset of them will be negatively affected.

@alexbarrett
Copy link
Author

Thanks Greg, that makes complete sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

drush_server_home() should strip trailing slashes from its returned path
2 participants