search

squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.66k stars 1.48k forks source link

Allow multiple spaces in PropertyDeclaration sniff to respect more carefully the PSR-12. #3756

Open niconoe- opened 1 year ago

niconoe- commented 1 year ago

Fix #3729

Note to maintainers: I wasn't in capacity of unit test this before any push. I'm having PHP 8.0 on my machine and when I run php vendor/bin/phpunit I have a fatal error due to a usage of the each() internal function by PHPUnit which is now removed from the PHP lib. I really think a more recent version of PHPUnit should be used, but that's another story.

niconoe- commented 1 year ago

@gsherwood I'm not actually a fan of allowing multiple spaces here. I took this issue a little bit because of the state of art, and also mainly because I wish I could contribute more to PHPCS in the future, and I needed a small thing to start, but I'm not as convinced as OP about how to interpret the PSR statement about it.

If I can help you in another way without taking care of something drown into its own complexity for my first contributions, I would love to do it. Thanks a lot.

jrfnl commented 1 year ago

Re: the PR - as this is a PSR-2 sniff, both the text in PSR2 as well as PSR12 will need to be taken into account. It also may need clarification for the PSR group (may already be available in the errata, but I haven't checked).

I have a fatal error due to a usage of the each() internal function by PHPUnit

IIRC running the test on PHP 8.0 will work fine with PHPUnit 7.x. I usually use the PHPUnit Phar, but you can also install via Composer using --ignore-platform-reqs. Also see: https://github.com/squizlabs/PHP_CodeSniffer/blob/add95a74551c3ba8fc99ef7651ad05f553b3fbbf/.github/workflows/test.yml#L115-L142

niconoe- commented 1 year ago

Re: the PR - as this is a PSR-2 sniff, both the text in PSR2 as well as PSR12 will need to be taken into account. It also may need clarification for the PSR group (may already be available in the errata, but I haven't checked).

Here are the definitions of PSR-12 and PSR-2. I wasn't able to find any erratum on any of those PSR.

The PER that extends and replaces [PSR-12] is a copy/paste of PSR-12 content on the section about the properties and constants style, so it doesn't help much more.

I usually use the PHPUnit Phar, but you can also install via Composer using --ignore-platform-reqs. I tried the PHAR way but it requires me to change my $PATH which I don't want to, as it will affect the way I'm dealing with the projects of my company. I tried also with the --ignore-platform-reqs but I got the same error message.

TBH, I was expecting GitHub to trigger some tests based on its workflow files and if any issue came this way, I would have been able to fix them, but a maintainer has to approve running the workflows instead of being automatic…

I'll try with a docker container in PHP 5.4 if I can, but I just hope it won't trigger other issues like absolute paths usages in unit tests that can't exist in my container as well.

niconoe- commented 1 year ago

I'm sorry, but I tried to run ./vendor/bin/phpunit from:

And no matter what, I always have either the undefined each() function error, or the Class 'PHP_Timer' not found error.

Without a better way to run PHPUnit on this repository, I won't be able to contribute anymore.

jrfnl commented 1 year ago

And no matter what, I always have either the undefined each() function error, or the Class 'PHP_Timer' not found error.

I'm not familiar with those Docker containers, but that sounds like the wrong PHPUnit version is being used (PHPUnit 4.x before a PHP requirement was included in the PHPUnit composer.json) and that those Docker containers still run the tests against PHP 8.0 (as otherwise you wouldn't get the notice about each()).

Without a better way to run PHPUnit on this repository, I won't be able to contribute anymore.

IIRC, once at least one PR has been approved to run and the PR has been accepted, CI will run automatically for PRs to the repo by the same contributor submitted after that.

In the mean time, if you want to see what CI will spit out, you can turn on the GH Actions runs in your own fork of this repo. This is not enable by default by GitHub, but IIRC, if you go to the "Actions" tab in the GH webinterface for your fork, you should be able to turn actions on. If you then force push this PR without changes, the actions should run.

niconoe- commented 1 year ago

I finally found a docker image (jakzal/phpqa:php7.4) that is having composer and PHP 7.4 so I was able to update my dependencies and use PHPUnit 7.5 instead of PHPUnit 5 which is bugged. This made me fix the unit tests regarding my changes, so now, it passes.

This PR now needs review and approval. Thanks a lot for your feedbacks @jrfnl ! :slightly_smiling_face: