Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: properly check formatting of function declaration statements #2328

Merged
merged 3 commits into from
Aug 11, 2023

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Aug 11, 2023

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:

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 now 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 deferring to PSR12 and enforcing consistency in this, seems like a reasonable choice.

Other notes:

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).

CS: fix up closure spacing to comply with new rules

GlobalVariablesOverride: move parse error test to separate file

... as it would cause a fixer conflict with the new rules.

As the fixer conflict is directly related to the parse error, I'm not concerned about it and I don't believe a fix is needed for the sniff.

Excluding this particular parse error file from the fixer conflict check should be an acceptable solution.

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).
... as it would cause a fixer conflict with the new rules.

As the fixer conflict is directly related to the parse error, I'm not concerned about it and I don't believe a fix is needed for the sniff.

Excluding this particular parse error file from the fixer conflict check should be an acceptable solution.
Copy link
Member

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GaryJones GaryJones merged commit 306542c into develop Aug 11, 2023
35 checks passed
@GaryJones GaryJones deleted the feature/core-check-function-declarations branch August 11, 2023 22:41
@jrfnl
Copy link
Member Author

jrfnl commented Aug 11, 2023

FYI: I've done a quick check of the handbook and the only code sample showing a closure is using a space between the function keyword and the open parenthesis, so is already in line with the change made in this PR that that is now enforced.

pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Sep 12, 2023
…ures.

Note: This is enforced by WPCS 3.0.0.

Reference: [WordPress/WordPress-Coding-Standards#2328 WPCS: PR #2328 Core: properly check formatting of function declaration statements].

Props jrf.
See #59161, #58831.

git-svn-id: https://develop.svn.wordpress.org/trunk@56559 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Sep 12, 2023
…ures.

Note: This is enforced by WPCS 3.0.0.

Reference: [WordPress/WordPress-Coding-Standards#2328 WPCS: PR #2328 Core: properly check formatting of function declaration statements].

Props jrf.
See #59161, #58831.
Built from https://develop.svn.wordpress.org/trunk@56559


git-svn-id: http://core.svn.wordpress.org/trunk@56071 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to gilzow/wordpress-performance that referenced this pull request Sep 12, 2023
…ures.

Note: This is enforced by WPCS 3.0.0.

Reference: [WordPress/WordPress-Coding-Standards#2328 WPCS: PR #2328 Core: properly check formatting of function declaration statements].

Props jrf.
See #59161, #58831.
Built from https://develop.svn.wordpress.org/trunk@56559


git-svn-id: https://core.svn.wordpress.org/trunk@56071 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Function declaration verification
3 participants