Function declaration verification

[jrfnl]

Following up on my remark here: #1084 (comment)

The _T_FUNCTION and T_CLOSURE (and T_USE for that matter) tokens have been added to that sniff in WPCS, but are not control structures. They should be handled separately completely as the control structure sniff is not suitable for those.

Inventarisation

Function declaration verification basically can be broken down into a number of separate issues/checks:

1. Spacing around the function / use keywords.
   Status: Problematic. Currently covered by the WordPress.WhiteSpace.ControlStructureSpacing sniff, but this is leading to issues.
   Viable upstream sniffs: PEAR.Functions.FunctionDeclaration, Squiz.Functions.FunctionDeclaration and Squiz.Functions.MultiLineFunctionDeclaration
2. Formatting of multi-line function declarations, i.e. parameters declarations and/or use statement over multiple lines.
   Status: Currently not covered.
   Viable upstream sniffs: Squiz.Functions.MultiLineFunctionDeclaration
3. Spacing around the function parameters and the comma's separating these.
   Status: Covered in WordPress-Core
   Sniff: Squiz.Functions.FunctionDeclarationArgumentSpacing
4. Spacing around variables being imported via a closure use statement and the comma's separating these.
   Status: Covered in WordPress-Core
   Sniff: Squiz.Functions.FunctionDeclarationArgumentSpacing
5. (No) Spacing within the parentheses if there are no parameters.
   Status: Covered in WordPress-Core
   Sniff: Squiz.Functions.FunctionDeclarationArgumentSpacing
6. Spacing of a return type declaration.
   Status: Open PR Add ReturnType Sniff #1084
7. Placement of the opening curly brace.
   Status: Covered in Wordpress-Core
   Sniff: Generic.Functions.OpeningFunctionBraceKernighanRitchie
8. Placement of the closing curly brace.
   Status: Partially covered in Wordpress-Core. If the closing brace is the first non whitespace token of a new line, the indent is checked. That it should be on a new line, is, however, currently NOT checked.
   Sniff: Generic.WhiteSpace.ScopeIndent
   Viable upstream sniffs: PEAR.WhiteSpace.ScopeClosingBrace or Squiz.WhiteSpace.ScopeClosingBrace
9. Methods: Verify that all methods have visibility declared.
   Status: Currently not covered.
   Viable upstream sniffs: Squiz.Scope.MethodScope
   Related issue: whether properties in a class have visibility declared is also not covered and could be covered by the Squiz.Scope.MemberVarScope sniff.
10. Methods: Verify spacing around the function modifier keywords, i.e. public, final etc.
    Status: Currently not covered.
    Viable upstream sniffs: Squiz.WhiteSpace.ScopeKeywordSpacing
11. Methods: Ordering of the function modifier keywords.
    Status: Currently not covered.
    Viable upstream sniffs: PSR2.Methods.MethodDeclaration enforces "final abstract visibility static" order.

Proposal 1 - check 8:

Add either the PEAR.WhiteSpace.ScopeClosingBrace or the Squiz.WhiteSpace.ScopeClosingBrace sniff to WordPress-Core.

I'd need to run some tests with these sniffs to see which one would suit best. The logic is different in both and the PEAR sniff has some additional differentiation for the scope closer indentation when case/default statements are "closed" by a break/return statement.
I don't expect either to cause fixer conflicts, but would like to verify this.

Proposal 2 - check 9 to 11:

1. Add the Squiz.Scope.MethodScope sniff to WordPress-Extra. The sniff throws an error for missing method visibility indicators.
2. Add the Squiz.Scope.MemberVarScope sniff to WordPress-Extra. The sniff throws an error for missing property visibility indicators.
3. Add the Squiz.WhiteSpace.ScopeKeywordSpacing sniff to WordPress-Extra. The error thrown by this sniff is auto-fixable.
4. Add the PSR2.Methods.MethodDeclaration sniff to WordPress-Extra. The error thrown by this sniff is auto-fixable.
   This sniff also throws a warning when method names are prefixed by a single underscore (PHP4 style visibility indicator). If so desired, that warning can be excluded. Personally, I'd like to keep it as IMHO the warning has merit.

Any of these errors can, of course, be downgraded to warnings if so desired.

I would welcome a discussion on whether any of the rules covered by the above mentioned sniffs should be added to/made explicit in the WP Core handbook. /cc @pento @ntwb

To discuss - check 1 + 3:

I've been looking into what upstream sniffs are available to handle this and if these would be suitable for use in WPCS.

The sniff would basically need to check:

• There should be no space between the function name and the open parenthesis.
• The function keyword should be followed by one space with the exception of closures. For closures whether this should be checked and if so whether a space should be enforced was previously determined based on a public property.
• If applicable, the use keyword should be both preceded and followed by one space.
• There should be exactly one space after each closing parenthesis in the function declaration.

Optionally:

• If a function declaration is multi-line (i.e. parameters not all on the one line):
  • Each parameter, including the first, should start on a new line.
  • The close parenthesis should be on the next line after the last parameter.
  • No blank lines allowed between the parameters.
  • Parameters indented one in from the first line of the function declaration.

There are three upstream sniffs which each in their own way cover this: PEAR.Functions.FunctionDeclaration, Squiz.Functions.FunctionDeclaration and Squiz.Functions.MultiLineFunctionDeclaration.

The Squiz.Functions.FunctionDeclaration does not allow for closures nor return type declarations, so IMHO that's not really a useful option.

The Squiz.Functions.MultiLineFunctionDeclaration sniff extends the PEAR.Functions.FunctionDeclaration sniff. They mainly differ in how they sniff multi-line function declarations.

The PEAR sniff (and by extending it the Squiz sniff) also checks the brace placement for single line declarations in a way which conflicts with the Generic.Functions.OpeningFunctionBraceKernighanRitchie sniff.
That part can, however, easily be disabled by extending the upstream sniff and just overloading the processSingleLineDeclaration() method.

AFAICS the space after closing parenthesis is not checked by these sniffs, but the Generic.Functions.OpeningFunctionBraceKernighanRitchie sniff combined with the "one space before use keyword" checks should cover all applicable cases.

The one real problem I see with using either of the above sniffs, is that the "space between function keyword and open parenthesis" for closures is NOT configurable and is enforced to be one space in both sniffs.

❓ Have we reached a point in time yet where we are willing to take a hard decision on this ? Enforce no or one space between the function keyword and the open parenthesis for closures ?

Depending on that decision, we'd either need to write our own sniff or can use one of the upstream ones.

If we'd use one of the upstream ones, we could also benefit from the multi-line checks, though I imagine some discussion on whether any specific formatting for multi-line should be enforced and if so, which, will need to had before that time.

Once a decision regarding this has been taken and a replacement sniff has been pulled, the function specific code can be removed from the WordPress.WhiteSpace.ControlStructureSpacing sniff and the exclusion of the Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingBeforeClose error code should be removed from the WordPress-Core ruleset (was added to prevent duplicate messages being thrown).

@WordPress-Coding-Standards/wpcs-collaborators @pento @ntwb Opinions ?

[JDGrimes]

Proposal 2 - check 9 to 11

I added the visibility indicators sniff to my custom ruleset some time ago. These other related sniffs look good too.

I would welcome a discussion on whether any of the rules covered by the above mentioned sniffs should be added to/made explicit in the WP Core handbook

I would say that the first three definitely ought to be, IMO.

For the modifier order check, on the other hand, I'm not so sure. I agree that final and abstract should come before the visibility, but I also tend to put static before the visibility keyword as well. Maybe that is just me, and most people do it the other way. If so, I'd be OK adding that rule to the core handbook in the order required by that sniff, as well.

To discuss - check 1 + 3:

I agree with the suggested rules for multiline function declarations.

❓ Have we reached a point in time yet where we are willing to take a hard decision on this ? Enforce no or one space between the function keyword and the open parenthesis for closures ?

I've always favored one space (#439 (comment)), so I'd be fine with requiring that and using an upstream sniff. But this might disagree with others' preferred style, and seems to be different than the JS standards, FWIW.

[GaryJones]

Proposal 2 - check 9 to 11:

+1 to WordPress-Extra, and +2 for Handbook + WordPress-Core.

❓ Have we reached a point in time yet where we are willing to take a hard decision on this ? Enforce no or one space between the function keyword and the open parenthesis for closures ?

I can see both sides (no space or one space), so I'm not too fussed which is chosen, so long as one is chosen and added to the Handbook, with the following factors (applies to most code standards discussions):

• firstly, ideally matches up to any de facto standard or PSR from the rest of the PHP community.
• secondly, and if it doesn't conflict with the first factor, ideally matches up to any de facto standard from our WP JS standards.

Saying that, since closures have been around for considerably longer than some of the PHP 7 syntaxes, I could imagine that many companies have plugins and themes that already favour no space or one space for closures. While we might agree and document to have one or the other, and set it as a default, I could imagine that those companies would like the ability to be able to choose the other. This is not the same as WPCS not checking / expressing a preference though.

[JDGrimes]

With regard to anonymous functions, I just ran across the WordPress.Classes.ClassInstantiation.SpaceBeforeParenthesis sniff flagging construction of anonymous classes.

new class( $var ) {}; // OK.
new class ( $var ) {}; // Flagged.

My instinct here again was to have a space (which is why it got flagged by WPCS). Might be worth considering consistency with anonymous classes when deciding what to do with anonymous functions.

[GaryJones]

❓ Have we reached a point in time yet where we are willing to take a hard decision on this ? Enforce no or one space between the function keyword and the open parenthesis for closures ?

I've always favored one space (#439 (comment)), so I'd be fine with requiring that and using an upstream sniff.

PSR-12 does seem to favour having a space, just like the use bit, and for anonymous classes. Since that's more inline with WP-preference of whitespace anyway, I'd say let's go with that.

[jrfnl]

Proposal 1 is being further investigated and discussed in issue #1474