Fix fixer conflict: PSR12 / Squiz.Functions.FunctionDeclarationArgumentSpacing

[fredden]

Description

While looking into a fixer conflict for the PSR12 standard regarding the tests/Core/File/GetMethodParametersTest.inc file, it was noted that there is a conflict between Squiz.Functions.FunctionDeclarationArgumentSpacing and Squiz.WhiteSpace.SuperfluousWhitespace.

To see this, run: php bin/phpcbf --standard=PSR12 -v tests/Core/File/GetMethodParametersTest.inc

With verbosity increased, we can see the conflict:

* fixed 1 violations, starting loop 47 *
Squiz.Functions.FunctionDeclarationArgumentSpacing:252 replaced token 1585 (T_FALSE on line 337) "false" => "false·"
      => Fixing file: 1/169 violations remaining [made 47 passes]...
* fixed 1 violations, starting loop 48 *
Squiz.WhiteSpace.SuperfluousWhitespace:212 replaced token 1586 (T_WHITESPACE on line 337) "·\n····" => "\n····"
      => Fixing file: 1/169 violations remaining [made 48 passes]...
* fixed 1 violations, starting loop 49 *
Squiz.Functions.FunctionDeclarationArgumentSpacing:252 replaced token 1585 (T_FALSE on line 337) "false" => "false·"
      => Fixing file: 1/169 violations remaining [made 49 passes]...
* fixed 1 violations, starting loop 50 *
Squiz.WhiteSpace.SuperfluousWhitespace:212 replaced token 1586 (T_WHITESPACE on line 337) "·\n····" => "\n····"
      => Fixing file: 1/169 violations remaining [made 50 passes]...
* fixed 1 violations, starting loop 51 *
*** Reached maximum number of loops with 1 violations left unfixed ***

Here is a small reproducible test case. Note the newline after the parameter type.

<?php

public function example(int
$number) {}

This pull request fixes the conflict by changing the logic within the sniff to no longer look at only one T_WHITESPACE token (and its length), but instead all T_WHITESPACE tokens between the type and parameter tokens, and validate on their content (not length).

Suggested changelog entry

Fix a fixer conflict: PSR12 / Squiz.Functions.FunctionDeclarationArgumentSpacing - newlines after type are now fixed properly.

Related issues/external references

#152

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]

Hi @fredden, thanks for this PR and for your continued work on the fixer conflicts.

I've reviewed the PR in detail and have a couple of remarks:

1. First of all, please use a descriptive commit message.
   In my opinion, it is important to have the information about the reason for the change and what problem is being solved codified in the commit message which is part of the code base. Now the information is only in the PR description, which makes a git blame more involved and adds a dependency on the code hosting platform for finding the context for a commit (and those can change/go away, think: Sourceforge -> PEAR -> GitHub -> ??)
2. Second, please have a look at the text of the updated error message.
   Previously, an error for the code snippet added in the tests would be reported like so:

   3 | ERROR | [x] Expected 1 space between type hint and argument "$number"; 0 found
   

   Note: the 0 found which was wrong and should probably have read newline found.
   However, now it is reported like so:

   3 | ERROR | [x] Expected 1 space between type hint and argument "$number";
     |       |     found
   

   ... which I don't think is very informative.
3. I think the fixer code can be made more straight-forward by:
   • Always adding a single whitespace to the $param['type_hint_end_token'].
   • Then looping over any whitespace tokens after the $param['type_hint_end_token'] token and removing them.
4. On that note: is the nondescript $i variable (in two places) really needed ? The $typeHintToken (which would be better named $typeHintEndToken) can be reset to the value of $param['type_hint_end_token'] at any time, so not sure the extra $i variable has value.
5. Looking at the test now added, that test doesn't fully test the change being made as it still only fixes a single whitespace token.
   I suggest adding a second test with an indented function declaration to ensure that multiple whitespace tokens are handled correctly and in one go:

       function multipleWhitespaceTokensAfterType(int
   
           $number) {}

   Also the public should probably be removed from the test already added, as it is a parse error outside of a class and is not needed for the test to be of value.

Other observations outside the scope of this PR/future scope:

1. Should the error still be auto-fixable if there is a comment between the type and the variable ?

   function commentBetweenTypeAndVariable(int
   //comment
   $number) {}

2. Looking at other parts of the sniff, it seems that nearly all errors/fixers suffer from the same presumption - that a parameter declaration is always on one line - and only handle one whitespace token at a time.
   Small PRs fixing it for one error at a time or a single PR with one commit per fix would allow for such a fix to stay reviewable.

Both of these last points should probably get their own ticket until these are solved.