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

Faulty Core fixes: else control structures without braces #971

Closed
jrfnl opened this Issue Jun 17, 2017 · 3 comments

Comments

@jrfnl
Contributor

jrfnl commented Jun 17, 2017

Summary:

The autofixer for else / elseif used without braces sometimes removes a new line which should be there.

Most likely cause:

Conflict between the Squiz.ControlStructures.ControlSignature.SpaceAfterKeyword - "Expected 1 space after ELSE keyword; newline found" - and the Generic.ControlStructures.InlineControlStructure.NotAllowed - "Inline control structures are not allowed" - sniffs.

Examples:

File: various files

Example from wp-login.php around line 194:

				if ( 'message' == $severity )
					$messages .= '	' . $error_message . "<br />\n";
				else
					$errors .= '	' . $error_message . "<br />\n";

Fixed as:

				if ( 'message' == $severity ) {
					$messages .= '	' . $error_message . "<br />\n";
				} else {                  $errors .= '	' . $error_message . "<br />\n";
				}

Expected fix:

				if ( 'message' == $severity ) {
					$messages .= '	' . $error_message . "<br />\n";
				} else {
					$errors .= '	' . $error_message . "<br />\n";
				}

Example from wp-signup.php around line 105:

	if ( !is_subdomain_install() )
		echo '<label for="blogname">' . __('Site Name:') . '</label>';
	else
		echo '<label for="blogname">' . __('Site Domain:') . '</label>';

Fixed as:

	if ( ! is_subdomain_install() ) {
		echo '<label for="blogname">' . __( 'Site Name:' ) . '</label>';
	} else {      echo '<label for="blogname">' . __( 'Site Domain:' ) . '</label>';
	}

Expected fix:

	if ( ! is_subdomain_install() ) {
		echo '<label for="blogname">' . __( 'Site Name:' ) . '</label>';
	} else {
		echo '<label for="blogname">' . __('Site Domain:') . '</label>';
	}
@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jun 17, 2017

Contributor

Fix has been pulled upstream squizlabs/PHP_CodeSniffer#1514.

Contributor

jrfnl commented Jun 17, 2017

Fix has been pulled upstream squizlabs/PHP_CodeSniffer#1514.

@jrfnl jrfnl changed the title from Faulty Core fixes: else/elseif control structures without braces to Faulty Core fixes: else control structures without braces Jun 17, 2017

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jun 17, 2017

Contributor

Darn! Found yet another example which is related to this issue, but not (yet) fixed by the PR. I'll see if I can improve on this solution some more.

Example code:

while ( isset($menu[$ptype_menu_position]) || in_array($ptype_menu_position, $core_menu_positions) )
	$ptype_menu_position++;

Is currently being fixed as:
Example code:

while ( isset( $menu[ $ptype_menu_position ] ) || in_array( $ptype_menu_position, $core_menu_positions ) ) {        $ptype_menu_position++;
}
Contributor

jrfnl commented Jun 17, 2017

Darn! Found yet another example which is related to this issue, but not (yet) fixed by the PR. I'll see if I can improve on this solution some more.

Example code:

while ( isset($menu[$ptype_menu_position]) || in_array($ptype_menu_position, $core_menu_positions) )
	$ptype_menu_position++;

Is currently being fixed as:
Example code:

while ( isset( $menu[ $ptype_menu_position ] ) || in_array( $ptype_menu_position, $core_menu_positions ) ) {        $ptype_menu_position++;
}
@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jun 19, 2017

Contributor

Ok, so I found the real cause of this issue with some help from upstream. It's not so much a bug in the upstream sniff, but a "feature" in our ruleset

PR #366 enabled the sniffing for control structure signatures, but explicitly excludes the rule to add a new line after the brace.

Removing that exclusion would fix this issue + #974.

@JDGrimes As you pulled that PR originally, how do you feel about removing the exclusion ?

Everyone else is of course also welcome to weight in.

Contributor

jrfnl commented Jun 19, 2017

Ok, so I found the real cause of this issue with some help from upstream. It's not so much a bug in the upstream sniff, but a "feature" in our ruleset

PR #366 enabled the sniffing for control structure signatures, but explicitly excludes the rule to add a new line after the brace.

Removing that exclusion would fix this issue + #974.

@JDGrimes As you pulled that PR originally, how do you feel about removing the exclusion ?

Everyone else is of course also welcome to weight in.

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