php errors should go to stderr, too

[arekm]

Describe the bug PHP errors should also go to stderr not stdout.

Current behaviour breaks external tools calling phpcs - these expect json and not mix of json with errors

Code sample + To reproduce

Current behaviour::

% echo test | /usr/local/bin/phpcs --report=json -q --encoding=UTF-8 --error-severity=5 --warning-severity=5 --stdin-path=/somewhere.php
PHP Deprecated:  Return type of PHP_CodeSniffer\Files\FileList::current() should either be compatible with Iterator::current(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2

Deprecated: Return type of PHP_CodeSniffer\Files\FileList::current() should either be compatible with Iterator::current(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2
PHP Deprecated:  Return type of PHP_CodeSniffer\Files\FileList::next() should either be compatible with Iterator::next(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2

Deprecated: Return type of PHP_CodeSniffer\Files\FileList::next() should either be compatible with Iterator::next(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2
PHP Deprecated:  Return type of PHP_CodeSniffer\Files\FileList::key() should either be compatible with Iterator::key(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2

Deprecated: Return type of PHP_CodeSniffer\Files\FileList::key() should either be compatible with Iterator::key(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2
PHP Deprecated:  Return type of PHP_CodeSniffer\Files\FileList::valid() should either be compatible with Iterator::valid(): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2

Deprecated: Return type of PHP_CodeSniffer\Files\FileList::valid() should either be compatible with Iterator::valid(): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2
PHP Deprecated:  Return type of PHP_CodeSniffer\Files\FileList::rewind() should either be compatible with Iterator::rewind(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2

Deprecated: Return type of PHP_CodeSniffer\Files\FileList::rewind() should either be compatible with Iterator::rewind(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2
PHP Deprecated:  Return type of PHP_CodeSniffer\Files\FileList::count() should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2

Deprecated: Return type of PHP_CodeSniffer\Files\FileList::count() should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2
PHP Deprecated:  Return type of PHP_CodeSniffer\Filters\Filter::getChildren() should either be compatible with RecursiveFilterIterator::getChildren(): ?RecursiveFilterIterator, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Filters/Filter.php on line 2

Deprecated: Return type of PHP_CodeSniffer\Filters\Filter::getChildren() should either be compatible with RecursiveFilterIterator::getChildren(): ?RecursiveFilterIterator, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Filters/Filter.php on line 2
PHP Deprecated:  Return type of PHP_CodeSniffer\Filters\Filter::accept() should either be compatible with FilterIterator::accept(): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Filters/Filter.php on line 2

Deprecated: Return type of PHP_CodeSniffer\Filters\Filter::accept() should either be compatible with FilterIterator::accept(): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Filters/Filter.php on line 2
{"totals":{"errors":0,"warnings":1,"fixable":0},"files":{"\/somewhere.php":{"errors":0,"warnings":1,"messages":[{"message":"No PHP code was found in this file and short open tags are not allowed by this install of PHP. This file may be using short open tags but PHP does not allow them.","source":"Internal.NoCodeFound","severity":5,"fixable":false,"type":"WARNING","line":1,"column":1}]}}}

Fortunately PHP supports displaying errors to stderr:

echo test | /usr/local/bin/php -d display_errors="stderr" /usr/local/bin/phpcs --report=json -q --encoding=UTF-8 --error-severity=5 --warning-severity=5 --stdin-path=/somewhere.php
PHP Deprecated:  Return type of PHP_CodeSniffer\Files\FileList::current() should either be compatible with Iterator::current(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2
Deprecated: Return type of PHP_CodeSniffer\Files\FileList::current() should either be compatible with Iterator::current(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2
PHP Deprecated:  Return type of PHP_CodeSniffer\Files\FileList::next() should either be compatible with Iterator::next(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2
Deprecated: Return type of PHP_CodeSniffer\Files\FileList::next() should either be compatible with Iterator::next(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2
PHP Deprecated:  Return type of PHP_CodeSniffer\Files\FileList::key() should either be compatible with Iterator::key(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2
Deprecated: Return type of PHP_CodeSniffer\Files\FileList::key() should either be compatible with Iterator::key(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2
PHP Deprecated:  Return type of PHP_CodeSniffer\Files\FileList::valid() should either be compatible with Iterator::valid(): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2
Deprecated: Return type of PHP_CodeSniffer\Files\FileList::valid() should either be compatible with Iterator::valid(): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2
PHP Deprecated:  Return type of PHP_CodeSniffer\Files\FileList::rewind() should either be compatible with Iterator::rewind(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2
Deprecated: Return type of PHP_CodeSniffer\Files\FileList::rewind() should either be compatible with Iterator::rewind(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2
PHP Deprecated:  Return type of PHP_CodeSniffer\Files\FileList::count() should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2
Deprecated: Return type of PHP_CodeSniffer\Files\FileList::count() should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Files/FileList.php on line 2
PHP Deprecated:  Return type of PHP_CodeSniffer\Filters\Filter::getChildren() should either be compatible with RecursiveFilterIterator::getChildren(): ?RecursiveFilterIterator, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Filters/Filter.php on line 2
Deprecated: Return type of PHP_CodeSniffer\Filters\Filter::getChildren() should either be compatible with RecursiveFilterIterator::getChildren(): ?RecursiveFilterIterator, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Filters/Filter.php on line 2
PHP Deprecated:  Return type of PHP_CodeSniffer\Filters\Filter::accept() should either be compatible with FilterIterator::accept(): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Filters/Filter.php on line 2
Deprecated: Return type of PHP_CodeSniffer\Filters\Filter::accept() should either be compatible with FilterIterator::accept(): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///usr/local/Cellar/php-code-sniffer/3.6.1/bin/phpcs/src/Filters/Filter.php on line 2
{"totals":{"errors":0,"warnings":1,"fixable":0},"files":{"\/somewhere.php":{"errors":0,"warnings":1,"messages":[{"message":"No PHP code was found in this file and short open tags are not allowed by this install of PHP. This file may be using short open tags but PHP does not allow them.","source":"Internal.NoCodeFound","severity":5,"fixable":false,"type":"WARNING","line":1,"column":1}]}}}

which will help these tools since they parse stdout only:

% echo test | /usr/local/bin/php -d display_errors="stderr" /usr/local/bin/phpcs --report=json -q --encoding=UTF-8 --error-severity=5 --warning-severity=5 --stdin-path=/somewhere.php 2> /dev/null
{"totals":{"errors":0,"warnings":1,"fixable":0},"files":{"\/somewhere.php":{"errors":0,"warnings":1,"messages":[{"message":"No PHP code was found in this file and short open tags are not allowed by this install of PHP. This file may be using short open tags but PHP does not allow them.","source":"Internal.NoCodeFound","severity":5,"fixable":false,"type":"WARNING","line":1,"column":1}]}}}

so please set display errors to stderr in phpcs. Would complete #1612

Versions (please complete the following information):

• OS: MacOS 12.0.1
• PHP: 8.1 (could be unsupported with current phpcs but above is generic behaviour and just deprecation errors from 8.1 are an example)
• PHPCS: 3.6.1
• Standard: default

[jrfnl]

@arekm Couple of points here:

1. Your install appears to be broken. PHPCS 3.6.1 does not display the PHP errors you are showing on PHP 8.1 - those were fixed in #3400, which is included in PHPCS 3.6.1.
2. As per the milestone, #1612 has been implemented for PHPCS 4.0.0. It will not be backported to the 3.x branch, but will be included in 4.x once it is released.

[arekm]

It's version installed via "brew install php-code-sniffer" on macos, they could broke something packaging it. Anyway these errors were just examples of php errors printed to stdout.

Is #1612 for 4.x also telling php to print php own errors (like deprecation errors and other) to stderr? If yes then nice and this issue can be just closed. Otherwise still room for improvement on phpcs side.

[jeffbyrnes]

@jrfnl interestingly, I’m seeing these same PHP errors, installed via Homebrew:

♠ phpcs --version
PHP_CodeSniffer version 3.6.2 (stable) by Squiz (http://www.squiz.net)

I’ll take a look at what’s up with the Homebrew formula, seems like it’s not really v3.6.2.

[jeffbyrnes]

I downloaded the .phar files from the 3.6.2 release, and I’m still seeing this deprecation errors. Perhaps something else has changed?

[jrfnl]

@jeffbyrnes In that case, yes, more information would be needed and it is most likely either a Homebrew snafu or a PATH issue on your computer. Could there be multi installs on your computer, with the global phpcs command going to an older version ?

[jeffbyrnes]

@jrfnl so I think I tracked it down, but I’m flying a little blind.

The errors reported originally in this issue, and that I see, specify #[\ReturnTypeWillChange] to be added.

Your commit, be8fe7250adbdb7023822f7d39f3cd5721064301, mentions #[ReturnTypeWillChange] in the commit message, and that’s also what you added to FileList.php and Filter.php.

I updated those files to use #[\ReturnTypeWillChange] instead, and the errors no longer appear when I run from my local clone.

I’ll open a PR for your consideration.

[jrfnl]

Hmm.. just did some testing with the latest PHAR files and am seeing these notices as well... I'll go do some digging in the PHAR generation to see if there's something wrong there.

[jrfnl]

Found it. The PHARs for the release are (were) being generated with PHP 7.4, which strips attributes as if they were comments.

PR #3442 already splits off the PHAR building in CI and if it had been merged, we probably would have detected this issue in CI.

I've updated that PR now to make sure the PHAR generation in CI will be done in PHP 8.0.

Other than that, @gsherwood will need to make sure to generate the PHAR files on PHP 8.0 (or higher) when doing the actual release. (or use the artifacts as generated via PR #3442 for the tagged commit)

[jrfnl]

Argh.. things are even worse... see my comment here: https://github.com/squizlabs/PHP_CodeSniffer/pull/3442#issuecomment-996072459

[jrfnl]

@jeffbyrnes You want to give the PHPCS Phar via Homebrew another try ? The PHAR files for PHPCS 3.6.2 have been regenerated and uploaded anew. Would be good to get confirmation that Homebrew picks up on that correctly.

[jeffbyrnes]

Sure thing! I’ll give it a spin first thing tomorrow.

-- Jeff Byrnes my.pronoun.is/he/him @thejeffbyrnes thejeffbyrnes.com On Dec 16, 2021, 7:23 PM -0500, Juliette @.***>, wrote:

@jeffbyrnes You want to give the PHPCS Phar via Homebrew another try ? The PHAR files for PHPCS 3.6.2 have been regenerated and uploaded anew. Would be good to get confirmation that Homebrew picks up on that correctly. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[jeffbyrnes]

It seems not, and that’s probably b/c of how versioning & packaging are done with Homebrew. I’ll take a look to see if I can give it a “kick” to pick up the new phars.

[jeffbyrnes]

Ok, I remember how this all works w/ Homebrew now; they have a packaging functionality that needs a prod to get it to re-package things if the version number hasn’t changed.

[jeffbyrnes]

Somebody beat me to it 😆 Check Homebrew/homebrew-core#91499, that should fix things up.

[jrfnl]

@jeffbyrnes Thanks for checking this and making sure it is (going to be) picked up!

[jeffbyrnes]

@jrfnl you’re welcome, thanks for such prompt attention to figuring out what was busted! An essential tool, esp. for those of us who only write PHP every now & then 😄

[jeffbyrnes]

All squared up over on Homebrew, and I can confirm it installs the fixed binary, so this can be closed!

[arekm]

This bug isn't about that. It's about telling php to redirect errors to stderr.

[jrfnl]

This bug isn't about that. It's about telling php to redirect errors to stderr.

@arekm Well, I've been thinking about that one too and I'm not sure that's really an option for PHPCS.

Some examples of situations which I believe will need investigation before it could even be contemplated:

1. There is Generic.PHP.Syntax sniff which lints PHP files and reports the errors in a PHPCS report. It would need investigation to see if that sniff would still work with the change you are proposing.
2. There is a build-in mechanism that when the running of a sniff results in a PHP error, the rest of the file is skipped and the PHP error is reported in the PHPCS report for the file. Again, this will need to be investigated, but I have a niggly suspicion that if these kind of PHP errors would be written to stderr, it will leave end-users of PHPCS with the impression that the file was scanned correctly and completely (which it was not) and that these type of bugs in sniffs would never get reported, nor fixed.

I have to admit that I'd need to brush up on how error handling vs stderr/stdout works again, so the above may not be problematic, but IMO it will need investigating before considering this proposed change.