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

Use realpath() to find the parent of a relative path #40

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pjcdawkins
Copy link

I noticed Drush 9 would find my Drupal site with any of these:

$ cd site
$ drush -r ../site/web status
$ drush -r /Users/patrick/site/web status
$ drush -r web/../web status

but curiously not with:

$ cd site
$ drush -r web status

My composer.json lives in site, while autoload.php and core live under web, based on https://github.com/platformsh/template-drupal8

I noticed the shiftPathUp() method does not account for relative paths.

This PR is a small modification to shiftPathUp() which means it can use the realpath() if necessary to normalize a relative path.

If incorporated into Drush 9, this would allow a Platform.sh CLI issue to be resolved quite nicely platformsh/legacy-cli#772

@pjcdawkins
Copy link
Author

Rebasing/updating to add a failing test, and then the fix

Copy link
Collaborator

@weitzman weitzman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I’ve had problems in the past when using real path on windows. Not sure if that applies here. Would be ideal to find a different approach.

@pjcdawkins
Copy link
Author

If realpath() here fails by returning false or the original path, then there is no change in behavior.

Windows only needs to make the path real "enough" to find the parent directory each time, if any, and this method is called until there is no parent directory, so the bug mentioned here does not apply.

realpath() only works on existing files - fortunately in this case existing files are all we care about.

realpath() is used frequently elsewhere in this class, to resolve symlinks.

@pjcdawkins
Copy link
Author

Changes approved. I'd still find this really useful. @webflo would you mind reviewing?

@weitzman
Copy link
Collaborator

Any thoughts @webflo?

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.

3 participants