Closed shivapoudel closed 1 year ago
Additionally this ticket have some research https://github.com/woocommerce/woocommerce/issues/36850
I'm not sure I understand the issue, exactly.
From the linked ticket:
Eventually I discovered that the issue is that you have to run the command from the directory where the composer.json file is located (plugins/woocommerce)
This should not be necessary. phpcs-changed
can be run from anywhere. You don't need to use composer exec
, you can just specify the path to the binary.
but the phpcs-changed command assumes that you are in the root of the Git repository, so it can't find the files that you're specifying when it's getting the linting results from the base branch. Thus it assumes that all of the changed files are brand new and shows you every linting error.
That doesn't sound right. phpcs-changed
is designed to operate correctly outside of the git root directory, but maybe there's a bug with that feature. Can you run the command that's not working as you expect with --debug
and copy the output here so I can see what is going wrong?
I found the issue as reported by the woocommerce repo. You can read the details in https://github.com/woocommerce/woocommerce/issues/36850#issuecomment-1483841136
Basically, some git commands (eg: git show
) are using the full file path when referring to the file by using $(git ls-files --full-name 'file.php')
, but others (eg: git cat-file -e
) are not. They should all be using that path.
The command should be fixed by https://github.com/sirbrillig/phpcs-changed/pull/67
@sirbrillig I can share this so you can debug thoroughly:
I use lint-staged and husky to add a pre-commit hook and run phpcs-change command. Here I get this error while committing:
I tried to output some info with --debug
:
In package.json file lint-staged is configure like this for PHP:
Above used PHPCS Changed is setup like this in phpcs-pre-commit
:
Still same error after the latest version of phpcs-changed.
Finally, this one is the latest with --debug
:
Please lemme know if needed anything.
@shivapoudel Thanks! it seems that inside whatever environment composer run-script
creates (or possibly the environment created by husky?), it cannot find the git
executable (it's not available in $PATH
). ... 😅 which I guess is what you reported in the first place, but I got distracted by the issue in the woocommerce repo.
You can explicitly tell phpcs-changed
where to find git
by setting the $GIT
environment variable. Maybe try changing the command to GIT=$(which git) composer run-script phpcs-pre-commit
or export GIT=$(which git) && composer run-script phpcs-pre-commit
?
also
Ah, interesting! This must be an issue with how PHP on Windows executes programs. 🤔 Let me do some research...
When phpcs-changed
tries to do things with the native OS like run git
, it uses a class I wrote called UnixShell. However, I wrote that module to be very Unix-specific (Mac OS and Linux), and it's likely that on Windows at least some shell commands need to be executed in a different way. In this case the command that's failing is the function validateExecutableExists. It could be something as simple as quoting, using is_executable()
instead of type
to determine existence, or something more complex.
Fortunately, switching out the class for a different OS is very simple as it follows an interface and is instantiated at runtime. It would be easy to detect a Windows OS and use a different class, but I need to figure out how to properly write the functions in ShellOperator
for Windows and for that I need access to a Windows machine. I have some friends that may be able to help. I'll update this thread with my progress.
Hi @shivapoudel! Thanks for your patience. I tried using phpcs-changed
in bash on a new Windows 11 account with Windows Subsystem for Linux (WSL) installed and it worked without a problem (once I installed php, git, and phpcs).
What's curious to me is that in your screenshots, the which git
command returned C:/Program Files/Git/mingw64/bin/git
which is clearly a windows file system path except with forward slashes, but when I tried the same thing I got a Linux filesystem path /usr/local/bin/git
.
I don't know a ton about using unix scripts on Windows but something must be different about your environment than what I was using. Can you tell me more about your system? What version of Windows are you using, and what kind of shell are you using? Do you have WSL installed?
@sirbrillig Thanks for you effort and regarding the scenario of Git installation here are my answers:
What's curious to me is that in your screenshots, the
which git
command returnedC:/Program Files/Git/mingw64/bin/git
which is clearly a windows file system path except with forward slashes, but when I tried the same thing I got a Linux filesystem path/usr/local/bin/git
.
I am using Windows 11 but not using WSL and can be downloaded from here: https://git-scm.com/download/win
The type
command used in the exec()
function to check for the existence of the executable may not work on Windows operating systems. Instead, you can use the where
command to achieve the same result.
Here's an updated version of the function that should work on Windows 11:
public function validateExecutableExists(string $name, string $command): void {
exec(sprintf("where %s > nul 2>&1", escapeshellarg($command)), $ignore, $returnVal);
if ($returnVal != 0) {
throw new \Exception("Cannot find executable for {$name}, currently set to '{$command}'.");
}
}
In this version, the where
command is used to check for the existence of the executable. The output of the command is redirected to nul
to discard any output, and the return value is checked as before. This should allow the function to work correctly on Windows 11.
Can you please verify if these changes work on your end? This was the answer from Chat GPT, my friend :)
Thanks! That does seem to work (TIL where
).
exec(sprintf("where %s > nul 2>&1", escapeshellarg($command)), $ignore, $returnVal);
In this example you wrote > nul
to replace > /dev/null
. Was that intentional or a typo?
@sirbrillig That code was generated by chat GPT, not me. Might be typo or maybe not :)
Based on the screenshots in #68, it looks like the bash subshell syntax first-command $(second-command)
might not work in Windows. I have a feeling this is going to require more work than just changing the command that detects the executable. We may need to re-work the commands themselves to work in a Windows environment. 😞 We probably need to separate the subshell commands into just plain commands and perform the input and pipe operations manually inside PHP.
I'll try to get a Windows VM so I can test these things out myself but it will probably be about a month as I'm going on vacation 😓 . If you need this to work in the meantime, I suggest trying Windows Subsystem for Linux if that's an option that you can use.
Another idea while I work on this is to try the manual mode of phpcs-changed
. I have seen folks use manual mode to create a script that performs the git operations they need and pass the results as input.
Manual Mode:
In manual mode, only one file can be scanned and three arguments are required
to collect all the information needed for that file:
--diff <FILE> A file containing a unified diff of the changes.
--phpcs-unmodified <FILE> A file containing the JSON output of phpcs on the unchanged file.
--phpcs-modified <FILE> A file containing the JSON output of phpcs on the changed file.
Closing this in favor of https://github.com/sirbrillig/phpcs-changed/issues/73
I have a git repo and installed this package as devDependencies but it states git is not found in PATH.
Please help me with the solution.