Generic/Syntax: add support for inspecting code passed via STDIN

[rodrigoprimo]

Description

This PR improves the Generic.PHP.Syntax sniff to make it work when code is passed via STDIN. Before, any code passed this way would cause a false negative as the sniff was not taking into account that $phpcsFile->getFilename() might return STDIN instead of an actual file name.

Refer to #915 for additional context and testing instructions.

Suggested changelog entry

Fixed: the Generic.PHP.Syntax sniff now handles code passed via STDIN correctly

Related issues/external references

Fixes #915

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 have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.

[jrfnl]

@rodrigoprimo I believe there is still something wrong in the command as the tests do not pass on Windows.

Aside from that, could you please rebase this branch on the current master ?

[rodrigoprimo]

Aside from that, could you please rebase this branch on the current master ?

I just rebased without changes, and now remark is passing.

@rodrigoprimo I believe there is still something wrong in the command as the tests do not pass on Windows.

I will take a look to understand why the tests are failing on Windows. When I was creating this PR, I checked that the Quicktest workflow was passing, but I completely forgot that the full test suite is not executed on Windows in this workflow.

[jrfnl]

@rodrigoprimo Yes, you will probably want to add @group Windows to the test you added.

[rodrigoprimo]

@jrfnl, I added the @group Windows to the new test and fixed the issue on Windows.

As far as I could research, it is not straightforward to reliably pipe commands and escape characters when echoing on Windows, so I opted to create a temporary file instead when on this OS, and the file content is passed via STDIN. What do you think of this approach?

[jrfnl]

I added the @group Windows to the new test and fixed the issue on Windows.

As far as I could research, it is not straightforward to reliably pipe commands and escape characters when echoing on Windows, so I opted to create a temporary file instead when on this OS, and the file content is passed via STDIN. What do you think of this approach?

I've been thinking this over and while I appreciate all the work you've put into this, I have a feeling the Windows vs STDIN issue is theoretical only.

PHPCS doesn't currently support STDIN on Windows:

PHP_CodeSniffer/src/Config.php

Lines 432 to 436 in 368817d

	if (defined('STDIN') === false
	|| stripos(PHP_OS, 'WIN') === 0
	) {
	return;
	}

Now, whether that is something which should be fixed or not is outside the scope of this issue.

But knowing that STDIN is not supported for Windows, the sniff will never be called with STDIN when running on Windows, so the issue with the STDIN test not passing on Windows should probably be fixed by a @requires OS ^(?!WIN).* instead of changing the sniff code to accommodate Windows.

What do you think ?

[rodrigoprimo]

Good point, I failed to consider that PHPCS currently doesn't support STDIN on Windows. I went ahead, kept the original commit, dropped the two that I had added to make the sniff work when handling STDIN on Windows, and added a new one to skip the STDIN test on this OS.

[jrfnl]

@rodrigoprimo Thanks for updating this PR. Looks good to me now. I've only got a few small nitpicks, but other that, this is good to go.

[rodrigoprimo]

Thanks for the final remarks, @jrfnl. They are addressed now.

[jrfnl]

Thanks for these updates @rodrigoprimo ! LGTM. Will merge once the build has passed.