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

[niconoe-]

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-]

@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]

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:

PHP_CodeSniffer/.github/workflows/test.yml

Lines 115 to 142 in add95a7

	# Install dependencies and handle caching in one go.
	# @link https://github.com/marketplace/actions/install-composer-dependencies
	- name: Install Composer dependencies - normal
	if: ${{ matrix.php < '8.0' }}
	uses: "ramsey/composer-install@v2"
	# For PHP 8.0+, we need to install with ignore platform reqs as PHPUnit 7 is still used.
	- name: Install Composer dependencies - with ignore platform
	if: ${{ matrix.php >= '8.0' }}
	uses: "ramsey/composer-install@v2"
	with:
	composer-options: --ignore-platform-reqs
	# Note: The code style check is run multiple times against every PHP version
	# as it also acts as an integration test.
	- name: 'PHPCS: set the path to PHP'
	run: php bin/phpcs --config-set php_path php
	- name: 'PHPUnit: run the tests'
	if: ${{ matrix.php != '8.1' && matrix.php != '8.2' }}
	run: vendor/bin/phpunit tests/AllTests.php
	# We need to ignore the config file so that PHPUnit doesn't try to read it.
	# The config file causes an error on PHP 8.1+ with PHPunit 7, but it's not needed here anyway
	# as we can pass all required settings in the phpunit command.
	- name: 'PHPUnit: run the tests on PHP > 8.0'
	if: ${{ matrix.php == '8.1' || matrix.php == '8.2' }}
	run: vendor/bin/phpunit tests/AllTests.php --no-configuration --bootstrap=tests/bootstrap.php --dont-report-useless-tests

[niconoe-]

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-]

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

• docker container php:5.4-cli
• docker container php:5.6-cli
• docker container php:7.4-cli
• docker container composer:2 (using PHP 8.1 internally)

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]

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-]

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 ! 🙂