You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Core: properly check formatting of function declaration statements
As things were, the `WordPress.WhiteSpace.ControlStructureSpacing` sniff was being _abused_ to also check the formatting of function declaration statements, while it really isn't suitable for this.
There have been numerous issues regarding this and various "plaster on the wound" fixes have been made previously.
This PR is intended to fix the problem once and for all in a more proper fashion.
The checks which were previously done by the `WordPress.WhiteSpace.ControlStructureSpacing` sniff consisted of the following:
* Exactly one space after the `function` keyword (configurable for closures).
* No space between the function _name_ and the open parenthesis.
* Exactly one space after the open parenthesis.
* Exactly one space before the close parenthesis.
* Exactly one space before the open brace/after the close parenthesis.
The above didn't take return type declarations into account properly, would run into problems with function declarations which didn't have an open brace, such as `abstract` functions or interface functions (also see 2204), and handling multi-line function declarations was undefined territory.
It also led to:
* Duplicate messages - the `ControlStructureSpacing` sniff reporting things also reported by the `Squiz.Functions.FunctionDeclarationArgumentSpacing` sniff or by the `Generic.WhiteSpace.LanguageConstructSpacing` sniff.
* Unclear messages [1] - messages were often thrown on the `function` keyword, while the problem was further down the line.
* Unclear messages [2] - messages would refer to control structures instead of functions and would otherwise not always be clear.
This commit:
* Removes all code related to checking function declarations from the `WordPress.WhiteSpace.ControlStructureSpacing` sniff.
Includes removing the tests which were specific to function declarations.
This effectively reverts the function related changes in PRs 417, 432, 440, 463, 1319, 1837 in so far as those touched the `ControlStructureSpacing` sniff.
* Adds the `Squiz.Functions.MultiLineFunctionDeclaration` sniff to the `Core` ruleset to do those checks instead.
The change as currently proposed has been extensively researched and tested and should provide a smooth switch over.
Notes about the ruleset changes:
* The `Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingBeforeClose` code was previously silenced to prevent duplicate messages, but can now be re-enabled as we'll defer to that sniff for the spacing on the inside of the parentheses.
* The `Squiz.Functions.MultiLineFunctionDeclaration` sniff, by default, expects the opening brace for named functions on the line _after_ the declaration and for closures, it expects the opening brace on the _same_ line as the closure declaration.
WPCS expects the opening brace on the _same_ line as the function declaration in both cases and enforces this via the `Generic.Functions.OpeningFunctionBraceKernighanRitchie` sniff (which is also used by the `Squiz.Functions.MultiLineFunctionDeclaration` sniff for the closure check).
The ruleset now contains three error code exclusions to bypass the "brace on new line for named functions" checks.
* As the opening brace for closures is enforced to be on the same line, we can let the new Squiz sniff handle this.
To that end the `checkClosures` property for the `Generic.Functions.OpeningFunctionBraceKernighanRitchie` sniff is set to `false` to prevent duplicate messages for the same.
* To prevent duplicate messages about code after the open brace for named functions, the `Generic.Functions.OpeningFunctionBraceKernighanRitchie.ContentAfterBrace` code has to be silenced.
We cannot silence the same error code for the Squiz sniff as that would also silence the notice for closures, so silencing it for the `OpeningFunctionBraceKernighanRitchie` sniff, which now only handles named functions, is the only option, but should prevent the duplicate messages.
* Additionally, the `Squiz.Functions.MultiLineFunctionDeclaration.SpaceAfterUse` error code is excluded as it overlaps with the `Generic.WhiteSpace.LanguageConstructSpacing` sniff, which also checks this.
Notes about changes in behaviour before/after this change:
* If the open & close parentheses are not on the same line, the space on the inside of the parentheses is no longer checked (as it is seen as a multi-line function declaration for which different rules may apply).
* While not commonly used in WP, for multi-line function declarations, the sniff will now enforce one parameter per line with the first parameter on the line _after_ the open parenthesis and the close parenthesis on the line after the last parameter and indented the same as the start of the function declaration statement.
A function is considered multi-line when the open and close parentheses are not on the same line, which means that for closures with `use` statements, these rules will apply to both the function parameter list as well as the use parameter list.
* It is no longer possible to choose the spacing between the `function` keyword for closures and the open parenthesis.
This will no always be enforced to be a single space, which is in line with PSR12.
A check of WP Core showed that there was no consistency at all regarding this spacing, with approximately 50/50 of closures having no space vs 1 space, so defering to PSR12 and enforcing consistency in this, seems like a reasonable choice.
Other notes:
* The `WordPress.WhiteSpace.ControlStructureSpacing` sniff did not support formatting checks for PHP 7.4 arrow functions.
At this time, the `Squiz.Functions.MultiLineFunctionDeclaration` sniff does not do so either.
There is a PR open upstream squizlabs/php_codesniffer 3661 to add support for arrow functions, though that PR still needs an update for the (very inconsistent) rule chosen by PSR-PER to [enforce _no_ space between the `fn` keyword and the open parenthesis for arrow functions](php-fig/per-coding-style#53).
* There is also a PR open upstream - squizlabs/php_codesniffer 3739 - to fix two fixer issues. At this time, these do not pose a problem for WP Core as (aside from the closure keyword spacing), WP Core is "clean", but would be nice if that PR got merged to prevent future issues.
* While doing the research for this PR, I discovered an issue with the `Generic.Functions.OpeningFunctionBraceKernighanRitchie` sniff. It doesn't check the spacing before the open brace for empty functions.
PR squizlabs/php_codesniffer 3869 has been opened to fix that.
As empty functions are the exception, not the rule, I'm hoping it will get merged before any issues about this are reported.
Fixes 1101
Note: check 8 identified in issue 1101 is still open, but there is a separate issue open to discuss that in more depth - 1474 - and that issue remains open (for now).
0 commit comments