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

[2.x] Fix auto-fixer conflict between Squiz/ControlSignature and Generic/InlineControlStructure #1514

Closed
wants to merge 1 commit into
base: 2.9
from

Conversation

Projects
None yet
2 participants
@jrfnl
Contributor

jrfnl commented Jun 17, 2017

The WordPress project has (finally) decided to fix the remaining code style issues in the code base according to their own standard. See https://core.trac.wordpress.org/ticket/41057

An initial run of the auto-fixer yielded a number of bugs in the auto-fixers however. This PR is part of an effort to fix these issues.

As the WordPress Coding Standards are currently not (yet) compatible with PHPCS 3.x, it would be very much appreciated if this fix could (also) be merged into PHPCS 2.x.

This PR is the version for PHPCS 2.x. I'll be opening a sister-PR for the PHPCS 3.x/master branch.


Issue summary:

The autofixer for else / do ... while control structure signatures removes a new line which should be there when these control structures are used without braces.

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.
Basically, the Squiz sniff kicks in too early, removing the new line before the Generic sniff has had a chance to add the braces.

Fix:

I've fixed this by setting the Squiz.ControlStructures.ControlSignature sniff to ignore the "Expected 1 space after keyword" violation for inline control structures in combination with a new line.
This allows the Generic.ControlStructures.InlineControlStructure sniff to kick in in an early auto-fix pass to add the necessary braces.
In the next auto-fix pass, the Squiz.ControlStructures.ControlSignature will kick in (if still needed) as now the braces are in place.

Example code:

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

Was being fixed as:

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

Expected fix /fix after this PR would be merged:

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

Related: WordPress-Coding-Standards/WordPress-Coding-Standards#971

@jrfnl jrfnl changed the title from Fix auto-fixer conflict between Squiz/ControlSignature and Generic/InlineControlStructure to [2.x] Fix auto-fixer conflict between Squiz/ControlSignature and Generic/InlineControlStructure 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 this 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 this 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++;
}
@gsherwood

This comment has been minimized.

Show comment
Hide comment
@gsherwood

gsherwood Jun 19, 2017

Member

In my testing, your sample code is fixed like this:

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

When I ran phpcbf with a standard containing only those two sniffs:

<?xml version="1.0"?>
<ruleset name="MyStandard">
  <description>My custom coding standard.</description>
  <rule ref="Squiz.ControlStructures.ControlSignature" />
  <rule ref="Generic.ControlStructures.InlineControlStructure" />
</ruleset>

The Generic sniff is kicking in before the Squiz one though, even though I've defined them in the opposite order.

The other thing that I'm wondering is why not just include additional sniffs to fix this code:

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

So that it matches what you want? The Squiz and PSR2 standards will both fix that code to look the correct way, although the PEAR standard will not. You could pull individual rules from those standards to ensure that the code is fixed even if someone manually writes their code like that.

Member

gsherwood commented Jun 19, 2017

In my testing, your sample code is fixed like this:

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

When I ran phpcbf with a standard containing only those two sniffs:

<?xml version="1.0"?>
<ruleset name="MyStandard">
  <description>My custom coding standard.</description>
  <rule ref="Squiz.ControlStructures.ControlSignature" />
  <rule ref="Generic.ControlStructures.InlineControlStructure" />
</ruleset>

The Generic sniff is kicking in before the Squiz one though, even though I've defined them in the opposite order.

The other thing that I'm wondering is why not just include additional sniffs to fix this code:

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

So that it matches what you want? The Squiz and PSR2 standards will both fix that code to look the correct way, although the PEAR standard will not. You could pull individual rules from those standards to ensure that the code is fixed even if someone manually writes their code like that.

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jun 20, 2017

Contributor

Oh darn! Thanks for your response.

The Squiz and PSR2 standards will both fix that code to look the correct way

That remark pointed me in the right direction and I think I've found the real culprit now. The Squiz.ControlStructures.ControlSignature.NewlineAfterOpenBrace error was excluded in the WordPress-Core ruleset. sigh

Sorry for wasting your (and my) time with this.

I've opened the discussion downstream to enable that check and will run some more tests.

For now, I'll close this issue.

Contributor

jrfnl commented Jun 20, 2017

Oh darn! Thanks for your response.

The Squiz and PSR2 standards will both fix that code to look the correct way

That remark pointed me in the right direction and I think I've found the real culprit now. The Squiz.ControlStructures.ControlSignature.NewlineAfterOpenBrace error was excluded in the WordPress-Core ruleset. sigh

Sorry for wasting your (and my) time with this.

I've opened the discussion downstream to enable that check and will run some more tests.

For now, I'll close this issue.

@jrfnl jrfnl closed this Jun 20, 2017

@gsherwood

This comment has been minimized.

Show comment
Hide comment
@gsherwood

gsherwood Jun 20, 2017

Member

No problem

Member

gsherwood commented Jun 20, 2017

No problem

@jrfnl jrfnl deleted the jrfnl:feature/2.x-fixing-else-inline-control-structures branch Oct 17, 2017

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