Package home | Report new bug | New search | Development Roadmap Status: Open | Feedback | All | Closed Since Version 2.0.0RC4

Bug #20230 Squiz ControlStructureSpacing sniff assumes specific condition formatting
Submitted: 2014-03-29 20:28 UTC Modified: 2014-07-03 04:57 UTC
From: aik099 Assigned: squiz
Status: Closed Package: PHP_CodeSniffer (version 1.5.2)
PHP Version: 5.4.20 OS: Linux
Roadmaps: (Not assigned)    
Subscription  


 [2014-03-29 20:28 UTC] aik099 (Alexander Obuhovich)
Description: ------------ The Squiz_Sniffs_WhiteSpace_ControlStructureSpacingSniff reports following code as an error: $a = 1; if ($a == 1) { echo "b"; } elseif ($a == 2} { echo "c"; } else { echo "d"; } The problem is that it reports the "elseif" and "else" and a problematic code because there is no empty line after control structure closing brace. This is the code that makes wrong decision: https://github.com/squizlabs/PHP_CodeSniffer/blob/a76a39b317c e8106abe6264daa505e24e1731860/CodeSniffer/Standards/Squiz /Sniffs/WhiteSpace/ControlStructureSpacingSniff.php#L197-L202

Comments

 [2014-03-29 20:34 UTC] aik099 (Alexander Obuhovich)
I've added the following code inside that IF: if ($tokens[$trailingContent]['code'] === T_ELSE || $tokens[$trailingContent]['code'] === T_ELSEIF) { // Allow else/elseif to come right after 'if' closing brace. return; }
 [2014-03-29 20:47 UTC] aik099 (Alexander Obuhovich)
Actually ignoring T_COMMENT that comes right after a closing brace of a control structure is a good idea too.
 [2014-03-30 22:52 UTC] squiz (Greg Sherwood)
-Summary: The Squiz_Sniffs_WhiteSpace_ControlStructureSpacingSniff sniff misbehaves +Summary: Squiz ControlStructureSpacing sniff assumes specific condition formatting -Assigned To: +Assigned To: squiz
 [2014-03-30 22:54 UTC] squiz (Greg Sherwood)
-Status: Assigned +Status: Closed
Fix committed to Github repo: https://github.com/squizlabs/PHP_CodeSniffer/commit/e6c0e414ef8b9f6ec2b1b65924f 491d0aa989f17 Should also be ignoring all sort of comments rather than just the specific ones it was coded for.
 [2014-04-20 16:02 UTC] aik099 (Alexander Obuhovich)
The fix actually doesn't really do what I asked. For example following code fails: <?php function someFunction() { // BEFORE if ($condition) { echo 'ok'; } // AFTER } And it fails because when looking for trailing content after scope closer all empty tokens (new lines, comment, doc block) are ignored so the sniff thinks that the comment after scope closer placed on a new line is "an empty line after conditional statement" and reports that as an error. I think that we might need to check for trailing content to be a T_WHITESPACE and only then report it as en empty line after conditional statement.
 [2014-04-20 18:30 UTC] aik099 (Alexander Obuhovich)
And with that fix you made now the following construct is perfectly valid: if ( $condition ) { echo "a"; } else { echo "b"; }
 [2014-04-20 19:37 UTC] aik099 (Alexander Obuhovich)
Yet another case. When inside a SWITCH and a control structure is placed right before "break" of a "case ...:/default:" then it's blank lines are not validated (see https://github.com/squizlabs/PHP_CodeSniffer/blob/1.5.2/CodeSniffer/Standards/Squiz/Sniffs/WhiteSpace/ControlStructureSpacingSniff.php#L168-L175). This way I can place a blank line before closing } of control struct and "break;" and this won't be reported as error. In more general case if a standard doesn't require a blank line, then it should report is as error in all cases.
 [2014-04-29 06:43 UTC] squiz (Greg Sherwood)
-Status: Closed +Status: Feedback
I've added another fix here: https://github.com/squizlabs/PHP_CodeSniffer/commit/ee165e7f10b300d343f66b01 63859ad05d9fd30a Regarding this code: if ( $condition ) { echo "a"; } else { echo "b"; } It isn't something this sniff checks for (other sniffs do) and it never has. It doesn't report any errors in 1.5.2 for this code, so the fix didn't change anything here.
 [2014-04-29 07:50 UTC] aik099 (Alexander Obuhovich)
Are you referring to https://github.com/squizlabs/PHP_CodeSniffer/blob/1.5.2/CodeSniffer/Standards/Squi z/Sniffs/ControlStructures/ControlSignatureSniff.php sniff? It does check brace position against control structure (e.g. "else" and it's "{"), but it doesn't check how "if" relates to "else" in the code.
 [2014-04-29 07:58 UTC] aik099 (Alexander Obuhovich)
About https://github.com/squizlabs/PHP_CodeSniffer/blob/1.5.2/CodeSniffer/Standards/Squiz/Sniffs/WhiteSpace/ControlStructureSpacingSniff.php#L168-L175 . I've actually solved this in a bit different fashion. I look for $emptyTokens to determine next non-empty content. If it turns out that next content is start of another control structure or end of this one and previous line (from found content) is an inline comment then I move stackPtr up to the comment line. This way counting how much empty lines I have between control structure and next code becomes easier. Also in my case the I only allow inline comment right before/after control structure, so 2 empty lines and then inline comment will cause an error to be reported. I'll show you what I have when I'll finish test writing. ---- I've also: - did some refactoring on the Sniff, since it's "process" method is too big to understand - added check for leading content (e.g. ask not to put empty line before control structure if it's placed at the function scope start (or another control structure) - handled case/default case, where this sniff just exited and never checked for blank lines before/after control structure when placed between "case/default" and "break"
 [2014-07-03 04:57 UTC] squiz (Greg Sherwood)
-Status: Feedback +Status: Closed
Closing this for now. If you'd like to submit a patch, please re-open, or just submit a new issue or PR and I'll take a look.