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

PSR2.ControlStructures.SwitchDeclaration.BreakIndent false positive when case keyword is not indented #3157

Closed
umherirrender opened this issue Nov 3, 2020 · 2 comments
Milestone

Comments

@umherirrender
Copy link

Describe the bug
The SwitchDeclaration sniff fails to determine the correct indent for the break statement when the case is on the same level as switch

Code sample

				switch ( $tokens[$next]['code'] ) {
				case T_OPEN_CURLY_BRACKET:
				case T_OPEN_SQUARE_BRACKET:
				case T_OPEN_PARENTHESIS:
					if ( isset( $tokens[$next]['parenthesis_closer'] ) ) {
						// jump to closing parenthesis to ignore commas between opener and closer
						$next = $tokens[$next]['parenthesis_closer'];
					}
					break; # <!--- ERROR | [x] Terminating statement must be indented to the same level as the CASE body
				case T_COMMA:
					$argCount++;
					break; # <!--- ERROR | [x] Terminating statement must be indented to the same level as the CASE body
				}

Expected behavior
The autofix adds a indent of 4 spaces to the break. Expected is no error reporting and no autofix or the cases are also reported and autofixed.

Versions (please complete the following information):

  • PHPCS: 3.5.8
@gsherwood
Copy link
Member

The sniff is making an assumption that the CASE keywords are indented 4 spaces from the SWITCH keyword. So it is then enforcing the rule that the breaking statements must be indented 8 spaces from the SWITCH keyword. Obviously, the error message is confusing here due to this assumption.

When taken as a whole, the PSR2 standard does reformat this code correctly because the ScopeIndent sniff is the one that ensures CASE statements are indented 4 spaces from the SWITCH. When taken alone, this sniff will not enforce that part of the standard.

So there are 2 ways forward here:

  1. Have the sniff enforce that CASE statements must be indented 4 spaces from the SWITCH keyword, which will make the sniff more valuable by itself, and fix the error message. This does mean a double-up of checking CASE indents though as the ScopeIndent sniff is going to do it regardless.
  2. Hide the error message when it is confusing and let ScopeIndent report an error for the CASE statement.

I'm learning towards option 2 at the moment, but will look into it more.

@gsherwood gsherwood added this to the 3.6.0 milestone Nov 3, 2020
@gsherwood gsherwood changed the title PSR2.ControlStructures.SwitchDeclaration.BreakIndent fails when case: is not indented PSR2.ControlStructures.SwitchDeclaration.BreakIndent false positive when case keyword is not indented Nov 3, 2020
gsherwood added a commit that referenced this issue Nov 3, 2020
…t false positive when case keyword is not indented
gsherwood added a commit that referenced this issue Nov 3, 2020
…t false positive when case keyword is not indented
@gsherwood
Copy link
Member

I played around with option 1 but I wasn't confident it would catch all cases, so I implemented option 2 instead.

Thanks for the bug report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants