Description

The --parallel option spawns processes with pcntl_fork and expects the result of pcntl_waitpid to be one of its spawned processes. It looks up the temporary output file of a process in the given $childProcs array without checking if the returned process ID is actually managed, which under some circumstances caused by external factors may result in an undefined array index error.

Suggested changelog entry

N/A, this change likely impacts a very small subset of users.

Related issues/external references

Fixes #1112

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
  • I looked for existing tests or infrastructure to integrate this change with (with the sample breaking command mentioned in the linked issue), but was unable to find any. I'm open to adding this command as a test in the appropriate place given some pointers.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.

I was able to reproduce the problem with the command that you provided in #1112 and confirm that this PR fixes it.

Here is the version of the command that I used:

bash -c 'bash --init-file <(echo "cd /path/to/phpcs/repo") -i -t -c "bin/phpcs --parallel=8 -p"'

@NanoSector Thank you for this PR and for your willingness to contribute to PHP_CodeSniffer!

While the bug being solved here is very much an edge-case and related to external factors, I also agree that a fix as straight-forward as this one should not be blocked because of that, so happy to merge this.

I looked for existing tests or infrastructure to integrate this change with (with the sample breaking command mentioned in the linked issue), but was unable to find any. I'm open to adding this command as a test in the appropriate place given some pointers.

This particular issue feels like a perfect example of the type of tests which could be run via BashUnit (Docs). Adding a BashUnit test set up to test the CLI commands has been on my "need to look into this" list for a while 😉

Would you be interested in having a look to see if you can create an initial test setup for PHPCS with BashUnit ?

Please note: there's no obligation to say "yes" as I also realize that creating an initial BashUnit set up may be more than you bargained for with this PR.
However, I definitely would like to safeguard this fix via automated tests, so if those won't be included in this PR, I'd appreciate it if you could open an issue as a reminder that a test for this particular issue still needs to be added.

@jrfnl Thanks for your feedback!

BashUnit is something I hadn't heard of before but indeed sounds like a perfect fit to handle cases like these & to application test this package more globally.

I'd definitely be interested in looking into this and it seems straightforward enough, I might be able to take a look later today. This issue isn't blocking for me so I'll append this PR.

@NanoSector That's be amazing! If it helps, feel free to DM me on Mastodon if you want to talk things through in a call.

Looks like something in the sequence for the unhappy flow test trips up the tests, disabling that fixes the pipeline - it works on my local machine however, but looking into it. Sorry for the spam you're probably getting.

@jrfnl I have implemented your feedback & implemented some basic bashunit tests. Few things of note:

1. It uses an in-repository bashunit file, which seemed cleaner than relying on an external installer script.
2. The unhappy flow tests use a temporary file in the current directory; I'd rather make them use a construction withmktemp as this would reduce the amount of code needed, but ran into issues where phpcbf would prepend the current working directory to the absolute path so it would fail trying to find the files.
3. The tests for this specific bug are a bit messy... I ran into issues with macOS and its outdated bash version, updating it through Homebrew yielded more process control issues. I opted for a platform-dependent solution and have verified that both commands reproduce the error.
4. I replaced the call to phpcs in the test.yml workflow file, it seemed to me that bashunit makes this obsolete.

Looking forward to your feedback!

@NanoSector Thank you for setting this up, amazing work!

I've looked the commit over. I really like that you've chosen the path of using test scripts. That will help with maintainability and gives contributors the ability to run these tests locally. 👍

I do have some feedback/questions.

1. It uses an in-repository bashunit file, which seemed cleaner than relying on an external installer script.

I'm not so sure about that and I'm not a fan of having code from other projects committed into this repo. The risk of someone making changes to the script and breaking our upgrade path are non-negligable (as this is plain code and not something like a closed PHAR file).

So, I'd rather it be installed in the GH Actions workflow at a specific version and that a section be added to the CONTRIBUTING file on what these tests are/when to add tests to the BashUnit setup, and how to install (link to the BashUnit manual + example command) and run the tooling locally.

2. The unhappy flow tests use a temporary file in the current directory; I'd rather make them use a construction with mktemp as this would reduce the amount of code needed, but ran into issues where phpcbf would prepend the current working directory to the absolute path so it would fail trying to find the files.

Are you talking about the copying of the Runner.php file and the you apply to that via tests/EndToEnd/es/Runner.php_inject_style_error. ?

In my opinion, using real PHPCS files for the tests make the tests quite fragile.
Think: someone submits a PR which updates the Runner.php or the Ruleset.php file and the contains a CS error. Now suddenly the BashUnit tests would also be failing as the output expectations no longer match.
This could be quite confusing for contributors and can lead to time wasted debugging something which doesn't need debugging.

Would it be possible to run these tests on some simple test fixtures instead ? I believe that would also side-step the issue with the current working directory ?

So, instead of having a es subdirectory, there would be a [Ff]ixtures subdirectory with some files which can be used in the tests.
Heads-up: You can find some example fixtures like that in the 4.x branch: https://.com/PHPCSStandards/PHP_CodeSniffer/tree/4.x/tests/Core/Util/ExitCode/Fixtures/ExitCodeTest

And if you then run phpcbf in the tests with the --suffix=.fixed flag, the original test fixtures would not be changed by the test run (new files with a .fixed extension would be created instead and those can be .gitignored for that specific fixtures directory).

That should also remove the need for the set_up() method and the tear_down() method would then only need to remove the *.fixed files from the fixture directory (with the .gitignore being a fallback in case the tear_down() didn't run for whatever reason).

3. The tests for this specific bug are a bit messy... I ran into issues with macOS and its outdated bash version, updating it through Homebrew yielded more process control issues. I opted for a platform-dependent solution and have verified that both commands reproduce the error.

🙌🏻

4. I replaced the call to phpcs in the test.yml workflow file, it seemed to me that bashunit makes this obsolete.

For now, I'd like to keep that test step in place, if you don't mind. I don't mind some QA duplication. Better that, than the tests failing to find an issue before the code gets released.

Other than that, it looks like the tests aren't running/working on the Windows builds (example). The Linux jobs all show the testdox output, the Windows jobs don't show any output at all.

I'd expect the step to fail with a non-zero exit code if the tests couldn't run on Windows, preferably with some feedback on why it is failing.
This will probably need some debugging to figure out why the tests aren't running on Windows.
First thing I'd suggest to try, would be adding shell: bash to the step and see if that makes a difference.

Then, depending on whether the failure to run on Windows is related to PowerShell or due to an issue with BashUnit itself, you may also want to open an issue in the BashUnit repo about this.

If needs be, I'm okay with skipping these tests on Windows initially and we can look into fixing that at a later point in time.
Making these kind of tests compatible cross-OS will probably not be that straight-forward in a lot of cases, especially for the setup/teardown, so running these tests on Windows may make life a lot more difficult for maintaining these tests, though time will tell.

(Skipping on Windows would mean adding a skip condition to the step in GH Actions + opening an issue about this to be addressed later.)

Let me know what you think.

[jrfnl]

Suggested change

	* @param array<non-negative-int, non-empty-string> $childProcs An array of child processes to wait for.
	* @param array<int, string> $childProcs An array of child processes to wait for.

These annotation should not be tooling specific and non-negative-int and non-empty-string are PHPStan specific annotations.

@jrfnl Thanks again for your feedback 🙌

I'm not so sure about that and I'm not a fan of having code from other projects committed into this repo. The risk of someone making changes to the script and breaking our upgrade path are non-negligable (as this is plain code and not something like a closed PHAR file).

While I understand this, a counter-point here would be bashunit making spontaneous breaking changes to their install script. You can lock the version using this install script but you'd still be relying on external shell code.
If you still prefer this then I'll change it, it's just not necessarily something I agree on.

Would it be possible to run these tests on some simple test fixtures instead ? I believe that would also side-step the issue with the current working directory ?

Yes, it would, I wasn't aware of the --suffix option (I was looking for a --dry-run flag instead) 👍

Other than that, it looks like the tests aren't running/working on the Windows builds
...
First thing I'd suggest to try, would be adding shell: bash to the step and see if that makes a difference.

Good catch, I assumed the tests succeeded since the pipeline didn't report errors. I'll give this a try; I don't have a Windows instance to debug on so if this doesn't work I'd prefer disabling the tests for now and opening a separate issue.

While I understand this, a counter-point here would be bashunit making spontaneous breaking changes to their install script. You can lock the version using this install script but you'd still be relying on external shell code. If you still prefer this then I'll change it, it's just not necessarily something I agree on.

I get that and I appreciate your thoughtfulness about this, but I'm willing to take that risk.
If the BashUnit team would break their install script at some point in the future, we can re-evaluate, but for now, there is no reason to expect they will (break the script).
And if they would, we report it back and help another open source project to do better (and I expect we wouldn't be the only ones reporting either) 😉

As an aside, I will be watching releases for the package to keep an eye on things.

Would it be possible to run these tests on some simple test fixtures instead ? I believe that would also side-step the issue with the current working directory ?

Yes, it would, I wasn't aware of the --suffix option (I was looking for a --dry-run flag instead) 👍

Figured as much. Glad I mentioned it.

Other than that, it looks like the tests aren't running/working on the Windows builds
...
First thing I'd suggest to try, would be adding shell: bash to the step and see if that makes a difference.

Good catch, I assumed the tests succeeded since the pipeline didn't report errors. I'll give this a try; I don't have a Windows instance to debug on so if this doesn't work I'd prefer disabling the tests for now and opening a separate issue.

If it helps, I'm on Windows, so we could do a screen share over video if you want me to test some things.