WordPress-Coding-Standards: Faulty core fixes: switch/case indentation

Summary:

There are a number of places in WP core which use - very large - switch/case constructs which are incorrectly indented. For these, the current fixer seems to create a worse inconsistency in the indentations for:

break - indentation not corrected
docblocks - indentation of the first line corrected, but not the rest of the docblock.
escaping out of PHP

Most likely cause: Generic.WhiteSpace.ScopeIndent

I’ve seen other bugs with this sniff before and it may actually be simpler to pre-indent the case statement manually before running phpcbf than to fix this sniff. Alternatively, turning that particular sniff off for the fixer might also save some grief.

Example:

File: various files across the board src/wp-admin/comment.php to name just one, src/wp-admin/edit-tags.php for another.

Example snippet:

switch ( $a ) {
case 'a':
	$b = 1;
	break;

case 'b':
	$b = 2;
	/**
	 * A comment about a hook.
	 */
	apply_filter( 'something', $b );
	break;

case 'a':
	$b = 3;
?>
	<input name="something" value=<?php echo $b; ?>
<?php
	$b = 4;
	break;

default:
	$b = 5;
	break;
}

Fixed as:

switch ( $a ) {
	case 'a':
		$b = 1;
	break;

	case 'b':
		$b = 2;
		/**
	 * A comment about a hook.
	 */
		apply_filter( 'something', $b );
	break;

	case 'a':
		$b = 3;
		?>
		<input name="something" value=<?php echo $b; ?>
	<?php
	$b = 4;
	break;

	default:
		$b = 5;
	break;
}

Expected fix:

switch ( $a ) {
	case 'a':
		$b = 1;
		break;
	
	case 'b':
		$b = 2;
		/**
		 * A comment about a hook.
		 */
		apply_filter( 'something', $b );
		break;
	
	case 'a':
		$b = 3;
		?>
		<input name="something" value=<?php echo $b; ?>
		<?php
		$b = 4;
		break;
	
	default:
		$b = 5;
		break;
}

About this issue

Original URL
State: closed
Created 7 years ago
Comments: 25 (24 by maintainers)

Commits related to this issue

Most upvoted comments

I prefer to add a blank line at the start of any control structure block that is more than one or two lines long. I find this more readable.

Same here, especially as a dyslectic. Whitespace can make a lot of difference in code readability.

jrfnl on Jul 9, 2017

Just to clarify, this would only apply when there’s a body to the case statement? A series of empty cases wouldn’t require a comment for each.

Correct.

// This would be fine.
switch ( $foo ) {
	case 'a':
	case 'b':
		break;
}

// This would be fine too.
switch ( $foo ) {
	case 'a':
	case 'b':
		return;
}

// This would require a comment for `case 'a'`.
switch ( $foo ) {
	case 'a':
		echo $foo;
	case 'b':
		return;
}

jrfnl on Jul 9, 2017

Practical point: What about leaving the “blank line at top” part out of the handbook as well as the ruleset for now and moving it to another issue to be discussed there ?

It’s a discussion which is unrelated to this particular issue (switch layout not being fixed correctly) - and as far as I can see at this moment - not one which is currently causing fixer conflicts -, so we can leave deciding upon whether to have a soft/hard rule about blank lines and if so, the wording and the sniffs we’d need, for a later point in time.

jrfnl on Jul 10, 2017

Hrrm, that’s an excellent point, it can improve readability. It’s really not used very often in Core, though.

Given variations of the following command, here are the stats I got.

src$ ag --php --stats '^(\t+)(if|while|for|do|switch|case|foreach|[a-z]* ?function|(} )?else).*\n\n(\1\t.*\n){1}\1[^\t]' wp-includes/ wp-admin/

Block Length	No Blank	Blank
1	8924	4
2	2447	8
3	1112	4
4	889	8
5	670	7
6	462	1
7	381	2
8	237	9
9	235	9
10+	663	23

It’s not at all common, though it does get slight more use as blocks get longer, which is to be expected.

I’m not really excited about any sort of hard rule depending on number of lines, though. Perhaps the best option is to soften the language a bit.

The first line of a new code block should not be blank, unless it clearly improves readability. The exception to this is the block inside a case statement, which must never start with a blank line.

pento on Jul 10, 2017

I fully agree with the other two additions to the handbook, but I would note that personally I prefer to add a blank line at the start of any control structure block that is more than one or two lines long. I find this more readable. But I do realize that core usually doesn’t follow this pattern, so I’ll understand if core is going to standardize on no blank lines at the start of control structures. (And even I usually omit the blank line at the start of a case block, I only add them for other control structures.) I just thought that I would make my voice heard.

JDGrimes on Jul 9, 2017

Okay, they’ve all been added to the handbook.

pento on Jul 9, 2017

“There must be no space before the colon in a case statement.”

Yes.

“There must be no blank line at the start of the code in a case or default statement.”

Yes.

“When a case statement statement does not end in a break/return, but deliberately falls through to the next case, it must be so documented with a comment.”

Just to clarify, this would only apply when there’s a body to the case statement? A series of empty cases wouldn’t require a comment for each.

pento on Jul 9, 2017

Oh - just noticed: the new example which was added to the handbook for the case/break indentation actually uses a space before the colon in the case statement, so this will need to be adjusted. /cc @ntwb

Done 👍

ntwb on Jul 4, 2017

I’ve got no objection to these being pushed through WordPress-Core, but it would be good to get them documented in the Handbook as well.

Ok, so I’ve dug into this a bit deeper.

Current practices used in Core

I figured it might make sense to actually see what’s currently used in Core to decide which rules to adopt anyhow. The SwitchDeclaration sniff does not create any PHPCS metrics, so we can’t see how this has evolved in WP historically. For now, I’ve added some metrics code locally and ran the info report (and opened a related upstream issue https://github.com/squizlabs/PHP_CodeSniffer/issues/1541).

The results were both interesting as well as surprising in some aspects:

PHP CODE SNIFFER INFORMATION REPORT
----------------------------------------------------------------------
Blank line at start of case/default:
        0 [3297/3325, 99.16%]
        1 => 28 (0.84%)

Case followed by space:
        1 [3056/3058, 99.93%]
        2 => 2 (0.07%)

Default/case statement followed by colon:
        yes [3325/3327, 99.94%]
        no => 2 (0.06%)

Fall through case:
        no [149/276, 53.99%]
        yes, with comment => 116 (42.03%)
        yes, but without comment => 11 (3.99%)

Keywords: lowercase [3327/3327, 100%]

Space before colon:
        no [2939/3325, 88.39%]
        1 => 365 (10.98%)
        5 => 4 (0.12%)
        4 => 4 (0.12%)
        3 => 4 (0.12%)
        2 => 4 (0.12%)
        8 => 2 (0.06%)
        7 => 1 (0.03%)
        9 => 1 (0.03%)
        6 => 1 (0.03%)

Terminating statement indented one tab from case:
        yes [1687/1873, 90.07%]
        no => 186 (9.93%)

Terminating statement on line by itself:
        yes [1873/1881, 99.57%]
        no => 8 (0.43%)
----------------------------------------------------------------------

Updated proposal

Based on this, I believe we should add the sniff:

SpacingAfterCase (99.9% compliance)
SpaceBeforeColon (88,3% compliance)
BodyOnNextLine (99.2% compliance)
BreakIndent: (90.1% compliance)
WrongOpener (99.9% compliance)
TerminatingComment (86.6% compliance where relevant)

and exclude:

NotLower (100% compliance) - duplicate msg
BreakNotNewLine (99.6% compliance) - duplicate msg

Handbook updates needed

Based on the rules I mentioned above and what’s already covered in a different way by WPCS (and therefore already in the handbook), I believe the only things which would still need to be added to the handbook would be:

“There must be no space before the colon in a case statement.”
“When a case statement statement does not end in a break/return, but deliberately falls through to the next case, it must be so documented with a comment.”
“There must be no blank line at the start of the code in a case or default statement.” - providing we’d choose to add BodyOnNextLine - this is not a hard rule for other control structures, so we may still need to think about this and/or add the same for other control structures in that case for consistency.

Oh - just noticed: the new example which was added to the handbook for the case/break indentation actually uses a space before the colon in the case statement, so this will need to be adjusted. /cc @ntwb

Detailed look at some of the results of the sniff

WrongOpener

WrongOpener would IMHO not need to be documented as code like that would mostly cause a parse error, so wouldn’t be acceptable anyway. I’ve investigated the two cases where this was flagged:

In wp-includes/class-wp-network.php around line 157 a semi-colon is used, which is clearly a typo and needs to be fixed.
Same goes for wp-includes/date.php around line 848.

I’ve opened a ticket in trac with a patch for these: https://core.trac.wordpress.org/ticket/41234

TerminatingComment

I’ve also looked at the TerminatingComment cases - most are legitimately flagged and could use a comment, a few are quite possibly bugs in the code where the fall-through is unintentional, and a few are situations where there is a control structure within the case and all possible pathways contain a return or break, but there is no return or break at the very end of the case. We could consider turning this into a warning and for the last part (all pathways contain an exit) this should possibly be reported upstream as a bug.

Side-note: Adding this sniff and making these rules explicit in the handbook, funnily enough fills a void already pointed out in 2013 in some comments on the handbook https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#comment-9283 and https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#comment-9984 /cc @JDGrimes

jrfnl on Jul 3, 2017

Now the standard for break indentation in switch control structures has been added to the handbook, I’ve looked into whether there was an upstream sniff we can use to cover and auto-fix this.

The PSR2.ControlStructures.SwitchDeclaration sniff would cover this, however it does more than we “need”, though the extra checks included aren’t necessarily a bad thing.

Error messages in the sniff:

NotLower: switch/case/default keyword must be lowercase => Already covered by Generic.PHP.LowerCaseKeyword.Found
SpacingAfterCase: CASE keyword must be followed by a single space
SpaceBeforeColon: There must be no space before the colon in a case/default statement
BodyOnNextLine: The case/default body must start on the line following the statement
BreakNotNewLine: Terminating statement must be on a line by itself (=break/return) => Already covered by Generic.Formatting.DisallowMultipleStatements
BreakIndent: Terminating statement must be indented to the same level as the CASE body (=break/return) <= This is the one we need.
WrongOpener: Case/default statements must be defined using a colon
TerminatingComment: There must be a comment when fall-through is intentional in a non-empty case body

Aside from the The case/default body must start on the line following the statement message, I think they would all be good additions to WPCS.

I propose adding this sniff to WordPress-Core, excluding error codes which would lead to duplicate messages and possibly excluding the BodyOnNextLine one if others agree and it would not lead to fixer conflicts.

The WrongOpener and TerminatingComment error codes are more best practice/bug prevention kind of checks, but I don’t think it would be a bad thing to leave them enabled in the Core sniffs in this case.

Note: Adding this sniff would not (yet) solve the docblock/escaping out of PHP indentation issues. Those are probably issues with the Generic.WhiteSpace.ScopeIndent sniff and would need to be looked at separately.

The alternative would be to add a WPCS native sniff to only check the indentation of switch control structures. In that case, the docblock/escaping out of PHP indentation issues should be covered by that sniff as well.

Opinions ?

/cc @pento

jrfnl on Jul 3, 2017