Ensure that the closing brace is properly aligned

[david-binda]

The Kernighan opening function Brace Sniff ( Generic.Functions.OpeningFunctionBraceKernighanRitchie.ContentAfterBrace ) which is part of the WordPress Coding Standard on it's own does not always produce desired output when it comes to running phpcbf.

WordPress Coding Standards v1.0 was used for updating following function in WordPress:

function is_admin() {return true;}

with following result:

function is_admin() {
    return true;}

See https://core.trac.wordpress.org/changeset/42343/trunk/src/wp-admin/includes/noop.php for real-live example.

Imho, such a change does not really improve readability of the code. From my point of view, the desired output should be as follows:

function is_admin() {
    return true;
}

While this is not explicitly defined in the WordPress coding standards, an example which can be found in https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#space-usage is very similar to the desired output described above:

function sum( $a, $b ): float {
    return $a + $b;
}

[jrfnl]

@david-binda That sniff only addresses the open brace. There is another sniff - which IIRC is included in WPCS - which checks the closing brace.

[david-binda]

There is another sniff - which IIRC is included in WPCS - which checks the closing brace.

Oh, it does not seem to kick-in then :) Also, I might not have expressed myself properly. I don't think there is an issue in the sniff ensuring the opening brace is the last thing on a line, just that we might be missing a complementary sniff properly handling the closing brace.

[jrfnl]

@david-binda No worries. I'm at WC Nijmegen at the moment, but will try and look into it over the next few days.

[jrfnl]

@david-binda Finally got round to this and you are 100% correct, there is a sniff missing here which should handle this.

This was actually previously reported as part of issue #1101 - item nr 8.

I'm looking into the various sniffs related to this now and will send in a PR to address this.

[david-binda]

Thank you for following up!

[jrfnl]

@WordPress-Coding-Standards/wpcs-admins Open question related to this issue: what should be the fixer behaviour for empty scopes ?

function wp_dashboard_empty() {}

Should this be allowed as-is ?

Or should this be "fixed" to:

function wp_dashboard_empty() {
}

[pento]

Allowed (or even enforced) as is: having the closing brace on the same line makes it clear that it's an empty scope.

[jrfnl]

From issue #1101:

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

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.

OK, so I have (finally) run some pretty extensive tests with these sniffs.

I've basically done separate WP Core fixer runs with each of these sniffs and compared the results:

	Desired result	PEAR sniff	Squiz sniff
Files resulting in fixer conflicts	0	12	10
Handling of empty scopes	no new line	adds new line	adds new line
Checks/fixes indentation of close brace	yes	yes	yes
Additional problems found in initial visual inspection	-	-	Line 243 in wp-admin/setup-config.php get partially removed.

Both sniffs target close braces of all constructs which PHPCS regards as scope openers. This means that the close brace of classes, interface, functions, but also all control structures, such as if, foreach, etc, would be checked by these sniffs.

IMO this is a good thing as the WPCS rulesets currently appear not to check the close brace position for these other constructs either.
This also means that a lot of "view" files will get some extensive fixes related to the placement of the "closing brace" of alternative control structure syntaxes, such as endforeach and endif.

Adding (a variation on) either of these sniffs to WPCS, would also partially address some of the remaining indentation issues we've seen in the WP Core runs, though as some of the PHP open tags for multi-line embedded PHP in WP Core are still in the wrong place, an initial auto-fixer run with such a sniff would need very careful scrutiny and I expect quite a lot of manual tinkering to get things just right.

I suspect that the fixer conflicts are also related to multi-line embedded PHP, but this needs further investigation and until a solution is found for this - either by manual tinkering or by creating fixes for the upstream sniffs -, I can't create a definitive solution (PR) for this issue.

@pento Want to have a look at the current results ? I can put the branches I have created online in a WP Core git fork here on GH.

[pento]

@jrfnl: I'd love to have a look. 🙂

[jrfnl]

@pento I've put them online for your perusal - mind: these are "dirty" branches:

1. TEMP/test-fixer-conflicts - just making sure there were no other fixer conflicts to begin with.
   When running this I noticed a file being skipped and I've contacted the relevant core committer who has promised to fix this. For testing purposes, I've added the fix needed here anyway.
2. TEMP/test-scope-closing-brace-sniffs-pear - Results of the tests with the PEAR sniff. Last commit contains the fixer run report including a list of files with fixer conflicts.
3. TEMP/test-scope-closing-brace-sniffs-squiz - Same, but then for the Squiz sniff.

[jrchamp]

Ran into this again this week trying to automatically start the cleanup process for a plugin. WPCS uses <rule ref="PSR2.Methods.FunctionClosingBrace"/> in its .phpcs.xml file. Have the fixers become any better in the past two years?

[jrfnl]

I believe that to action this issue, the analysis from comment #1474 (comment) will need to be redone with the current version of PHPCS to see if the outcomes are any better.

Based on that redone analysis, a decision can then be taken to either include one of the two pre-existing sniffs or to add a sniff to PHPCSExtra which handles this in a more configurable way.