search

php-parallel-lint / PHP-Parallel-Lint

This tool check syntax of PHP files faster than serial check with fancier output.
Other
287 stars 21 forks source link

Fix the php executable to handle folder name with spaces #171

Open dingo-d opened 6 months ago

dingo-d commented 6 months ago

As reported in https://github.com/beyondcode/herd-community/issues/631.

jrfnl commented 6 months ago

@dingo-d Thanks for this PR. I haven't looked at the code yet, but there have been two other PRs - #147 and #149 - not too long ago which attempted to do the same, but not always correctly. Could you please check that your PR avoids the problems those PRs had ?

dingo-d commented 6 months ago

@jrfnl I don't think this PR has the same problems as those two PRs. In one, the author was adding escaping around executable, and in another there was a whole new method for escaping the binary path.

In this PR the only change is wrapping the executable in quotes so that the spaces in the paths are correctly handled.

I don't have a windows based dev environment to test if this solution works there tho.

dingo-d commented 5 months ago

@jrfnl I fixed the failed phpcs check, additional tests are passing, so I think this should be good to go.

jrfnl commented 5 months ago

I fixed the failed phpcs check, additional tests are passing, so I think this should be good to go.

@dingo-d Thanks for that! I've just looked this over and these are my findings:

  1. While I agree the test you added has value, it doesn't actually test your change.
  2. When looking at that test, I wonder if some additional test cases should be added with Windows paths ? (forward slashes and paths quoted on the command line) Outside the scope for this PR, but would be good to safeguard that more for the future.

Back to the actual proposed change.

This change is about the path to the PHP executable containing spaces. I'm not actually sure if this can be tested via the test suite easily. Then again, you might be able to test it by making a copy of the executable file and then adding a space in the file path or something. Or maybe by creating a symbolic link where the symbolic link has a space in the path ?

Having said that, I think we may first need to have a good reproduction case anyhow as I haven't been able to make this fail on Windows in the first place (using the develop branch).

How I tested:

  1. Rename the directory on my system which contains the PHP executable I'm going to use for the tests to /php 8.3.7/ (note the space in the path)
  2. Updated the Windows Path environment variable to include this new path and no other path to PHP.
  3. Added a var_dump($executable) on the line above the change to verify that the path to PHP being used is the one with the space in it.
  4. Ran php ./parallel-lint ./src/errors/ from the root of this repo - this should use the system default PHP version, which should now be the PHP executable in the /php 8.3.7/ path. All okay.
  5. Ran "C:\wamp\bin\php\php 8.3.7\php.exe" ./parallel-lint ./src/errors/ - this makes the path to PHP explicit. Same thing, all okay.
  6. Did the same, but without quoting the path to PHP - got an (expected) Windows error about not being able to execute the command as it doesn't read past the space.

image

So, on develop I wasn't able to reproduce the issue, which means I can't verify the fix. Having said that, I do believe the fix is correct, though I wonder if any protection is needed against "double quoting", i.e. quoting a path which is already quoted ?

Maybe some people who are having this problem can outline a reproduction scenario ? /cc @ElRochito @azatakmyradov @piqusy

dingo-d commented 5 months ago

Having said that, I think we may first need to have a good reproduction case anyhow as I haven't been able to make this fail on Windows in the first place (using the develop branch).

I agree, but the failure affected macOS specifically so Windows not failing is a good sign. And with the proposed change, I tested that it's working on macOS.

I've renamed the path to my PHP to include a space:

image

Then ran the tests on the branch with the fix and on the develop branch, and I get the error in the develop branch, but not this one with the fix:

image
jrfnl commented 5 months ago

Rebased without changes to get a passing build after #174.

jrfnl commented 5 months ago

@dingo-d Thank you for sharing your test results. I didn't realize it was about MacOS, not Windows.

As the unit test doesn't really belong with this PR, do you want to move that contribution to a separate PR ?

Note: it's not a blocker, but it will prevent confusion about what the test is testing if someone does a git blame in the future.

dingo-d commented 5 months ago

@jrfnl I've removed the tests (I made a separate branch for those tests so I'll make a separate PR for that), and rebased to one commit. Hope it's ok now 👍🏼