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

Fix #11289: Handle paths with spaces in Silverstripe's sake script #11290

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

digitall-it
Copy link
Contributor

@digitall-it digitall-it commented Jun 22, 2024

Description

Expected Behaviour: vendor/bin/sake should function correctly regardless of where the PHP binary is located, even if the path includes spaces.

Observed Behaviour: If the path to the PHP interpreter contains spaces, the script fails with an error because the path isn’t wrapped in quotes, leading to incorrect parsing of the command.

This fix ensures that all paths in the sake script are properly quoted to handle spaces and other special characters that might appear in directory names.

Manual Testing Steps

  1. Place your PHP binary in a path with spaces and set up the environment variable or path accordingly.
  2. Run vendor/bin/sake build to ensure it picks up the PHP binary and executes without errors.
  3. Check other commands and scenarios where sake is used to ensure no other side effects are introduced by this change.

Issues

Pull Request Checklist

Additional Notes

Please review the changes and provide feedback or merge if everything is in order.

Thank you!

Fix bug in sake that happens when the PHP interpreter is a directory with spaces in it.
@digitall-it
Copy link
Contributor Author

I suggest revisiting the script to ensure that the use of the new syntax "${@}" instead of the old ${*} does not introduce unintended side effects.

The ${@} syntax, when quoted, ensures each argument is preserved in its entirety, which is crucial for handling paths or options that include spaces. It's important to verify that this change maintains the desired behavior throughout the script.

Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

Tested it locally and it seems sensible.

I don't know that there's a reasonable way of unit testing this. So I'm happy to merge as is.

The ${@} syntax, when quoted, ensures each argument is preserved in its entirety, which is crucial for handling paths or options that include spaces. It's important to verify that this change maintains the desired behavior throughout the script.

I'm trying to think of a scenario where this could break. Can't think of one off the top of my head.

The fact that this is targeted at the next minor, means we would have a couple months of playing with this to potentially find weird edge cases where $@ would produce a different outcome from $*.

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM. Tested with both php path and project path with spaces, and both work without problems.

@GuySartorelli GuySartorelli merged commit bdd27b9 into silverstripe:5 Jun 24, 2024
16 checks passed
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