Faulty core fixes: switch/case indentation #982

jrfnl · 2017-06-18T12:49:54Z

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;
}

The text was updated successfully, but these errors were encountered:

jrfnl · 2017-07-03T10:14:46Z

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 · 2017-07-03T10:56:00Z

FYI: I've also opened an upstream issue regarding the docblock/inline HTML indentation: squizlabs/PHP_CodeSniffer#1539

GaryJones · 2017-07-03T13:11:21Z

SpacingAfterCase: CASE keyword must be followed by a single space
SpaceBeforeColon: There must be no space before the colon in a case/default statement

Do these two not contradict each other?

ntwb · 2017-07-03T13:28:00Z

SpacingAfterCase: CASE keyword must be followed by a single space

SpaceBeforeColon: There must be no space before the colon in a case/default statement
Do these two not contradict each other?

No they don't, here's some examples:

case 'a':
//  ^   ^
//  1   2
case'b': // is a violation of `SpacingAfterCase`
case 'c' : // is a violation of `SpaceBeforeColon`
case'd' : // is a violation of both `SpacingAfterCase` and `SpaceBeforeColon`

jrfnl · 2017-07-03T13:36:32Z

@GaryJones No, they don't - between the case and the : you always have a value or condition ;-)

switch ( $a ) {
    case 'b':
        // One space between `case` and `'b'`.
        // No space between `'b'` and `:`.
        break;
}

jrfnl · 2017-07-03T13:37:45Z

@ntwb Ah, sorry, I didn't see your comment, should have refreshed the page before replying.
@GaryJones What @ntwb said ;-)

GaryJones · 2017-07-03T15:21:21Z

Ah, of course - I was forgetting the value. I apparently don't use switch statements often enough!

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.

jrfnl · 2017-07-03T22:46:16Z

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 squizlabs/PHP_CodeSniffer#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

ntwb · 2017-07-04T01:31:13Z

I'm a little surprised at the space before colon stats, I thought there would be more cases where spaces have been used for alignment:

Here's some examples:

https://github.com/WordPress/wordpress-develop/blob/4.8.0/src/wp-admin/setup-config.php#L341-L348

case 'AUTH_KEY'         :
case 'SECURE_AUTH_KEY'  :
case 'LOGGED_IN_KEY'    :
case 'NONCE_KEY'        :
case 'AUTH_SALT'        :
case 'SECURE_AUTH_SALT' :
case 'LOGGED_IN_SALT'   :
case 'NONCE_SALT'       :

https://github.com/WordPress/wordpress-develop/blob/4.8.0/src/wp-admin/comment.php#L75-L78

case 'delete'  :
case 'approve' :
case 'trash'   :
case 'spam'    :

https://github.com/WordPress/wordpress-develop/blob/4.8.0/src/wp-admin/comment.php#L238-L244

case 'deletecomment'    :
case 'trashcomment'     :
case 'untrashcomment'   :
case 'spamcomment'      :
case 'unspamcomment'    :
case 'approvecomment'   :
case 'unapprovecomment' :

ntwb · 2017-07-04T01:34:52Z

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 👍

jrfnl · 2017-07-09T05:16:15Z

@WordPress-Coding-Standards/wpcs-collaborators I'd like some opinions on the proposed additions to the handbook.

Once that's been decided on, the new sniff could be added - with or without exclusions - to fix the issue.

pento · 2017-07-09T06:52:25Z

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

jrfnl · 2017-07-09T09:56:45Z

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;
}

pento · 2017-07-09T13:01:13Z

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

…` ruleset As proposed in #982 (comment) Also adds the newly added section + rules from the handbook which came out of the discussion in #982. Partially fixes 982

jrfnl · 2017-07-09T13:38:32Z

Thanks @pento.

I've pulled the rule addition now as PR #1021.

Note: This doesn't completely fix the issue yet. There is an open issue upstream regarding the docblock/inline HTML indentation: squizlabs/PHP_CodeSniffer#1539

JDGrimes · 2017-07-09T17:23:34Z

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.

jrfnl · 2017-07-09T18:58:12Z

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.

pento · 2017-07-10T05:45:06Z

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.

GaryJones · 2017-07-10T07:34:34Z

unless it clearly improves readability.

That's open to wildly different interpretation, and that's not what we should be encouraging in the Handbook IMO.

jrfnl · 2017-07-10T11:45:38Z

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.

pento · 2017-07-11T01:15:46Z

Okay, I missed that it wasn't required for this fix. 🙂

I've removed the Blank Lines section from the handbook, pending further discussion.

…` ruleset As proposed in #982 (comment) Also adds the newly added section + rules from the handbook which came out of the discussion in #982. Partially fixes 982

jrfnl · 2017-07-11T01:33:43Z

@pento Sounds good to me. I've updated the open PR to reflect this now.

jrfnl · 2017-07-11T11:21:46Z

Reopened for the scope indent parts of the original report - the auto-close wasn't supposed to get triggered by the PR.

IuliaCazan · 2017-10-19T07:13:19Z

Just an observation: the rule of SpaceBeforeColon: There must be no space before the colon in a case/default statement kinda breaks the spacing consistency rules. Why is the case structure more special?

From the readability standpoint, I am in favor of spacing and empty lines to make the code much more easy to follow. Consistent rules make the code more easy to understand/implement/extend.

In this case, a more consistent rule would mean to treat the colon in the same way in all the contexts, either when it's used in an alternative syntax for control structures, or as a ternary operator.

jrfnl · 2017-12-01T00:37:46Z

The comment indent part will be fixed by the upcoming DocCommentFormat sniff as per #1120.

That leaves the PHP open/close tags and associated indentation. This has been discussed in other issues.
PHPCS needs an anchor point to determine indent and uses the open/close tags for that, so this is a won't fix.

That concludes this issue as far as I'm concerned.

jrfnl added Upstream: PHPCS Status: Needs investigation Type: Bug labels Jun 18, 2017

jrfnl mentioned this issue Jul 9, 2017

Add PSR2.ControlStructures.SwitchDeclaration to the WordPress-Core ruleset #1021

Merged

JDGrimes closed this as completed in #1021 Jul 11, 2017

jrfnl reopened this Jul 11, 2017

jrfnl added the Project: auto-fix WP Core / trac-41057 label Jul 21, 2017

jrfnl closed this as completed Dec 1, 2017

GaryJones removed the Status: Needs investigation label Aug 21, 2018

jrfnl added the Component: Core label Aug 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faulty core fixes: switch/case indentation #982

Faulty core fixes: switch/case indentation #982

jrfnl commented Jun 18, 2017

jrfnl commented Jul 3, 2017

jrfnl commented Jul 3, 2017

GaryJones commented Jul 3, 2017

ntwb commented Jul 3, 2017

jrfnl commented Jul 3, 2017

jrfnl commented Jul 3, 2017

GaryJones commented Jul 3, 2017

jrfnl commented Jul 3, 2017

ntwb commented Jul 4, 2017

ntwb commented Jul 4, 2017

jrfnl commented Jul 9, 2017 •

edited

Loading

pento commented Jul 9, 2017

jrfnl commented Jul 9, 2017

pento commented Jul 9, 2017

jrfnl commented Jul 9, 2017

JDGrimes commented Jul 9, 2017

jrfnl commented Jul 9, 2017

pento commented Jul 10, 2017

GaryJones commented Jul 10, 2017

jrfnl commented Jul 10, 2017

pento commented Jul 11, 2017

jrfnl commented Jul 11, 2017

jrfnl commented Jul 11, 2017

IuliaCazan commented Oct 19, 2017 •

edited

Loading

jrfnl commented Dec 1, 2017

Faulty core fixes: switch/case indentation #982

Faulty core fixes: switch/case indentation #982

Comments

jrfnl commented Jun 18, 2017

Summary:

Example:

jrfnl commented Jul 3, 2017

jrfnl commented Jul 3, 2017

GaryJones commented Jul 3, 2017

ntwb commented Jul 3, 2017

jrfnl commented Jul 3, 2017

jrfnl commented Jul 3, 2017

GaryJones commented Jul 3, 2017

jrfnl commented Jul 3, 2017

Current practices used in Core

Updated proposal

Handbook updates needed

Detailed look at some of the results of the sniff

WrongOpener

TerminatingComment

ntwb commented Jul 4, 2017

ntwb commented Jul 4, 2017

jrfnl commented Jul 9, 2017 • edited Loading

pento commented Jul 9, 2017

jrfnl commented Jul 9, 2017

pento commented Jul 9, 2017

jrfnl commented Jul 9, 2017

JDGrimes commented Jul 9, 2017

jrfnl commented Jul 9, 2017

pento commented Jul 10, 2017

GaryJones commented Jul 10, 2017

jrfnl commented Jul 10, 2017

pento commented Jul 11, 2017

jrfnl commented Jul 11, 2017

jrfnl commented Jul 11, 2017

IuliaCazan commented Oct 19, 2017 • edited Loading

jrfnl commented Dec 1, 2017

jrfnl commented Jul 9, 2017 •

edited

Loading

IuliaCazan commented Oct 19, 2017 •

edited

Loading