Skip to content

Bug fix: DisallowSpaceIndent did not recognize nor fix mixed tab/space indentations. #1404

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.

Sign up for GitHub

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

Merged
gsherwood merged 1 commit into from
Apr 5, 2017
Merged

Bug fix: DisallowSpaceIndent did not recognize nor fix mixed tab/space indentations. #1404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 75 additions & 25 deletions CodeSniffer/Standards/Generic/Sniffs/WhiteSpace/DisallowSpaceIndentSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
/**
* Generic_Sniffs_WhiteSpace_DisallowSpaceIndentSniff.
*
* Throws errors if spaces are used for indentation.
* Throws errors if spaces are used for indentation other than precision indentation.
*
* @category PHP
* @package PHP_CodeSniffer
Expand Down Expand Up @@ -94,42 +94,92 @@ public function process(PHP_CodeSniffer_File $phpcsFile, $stackPtr)
continue;
}

// If tabs are being converted to spaces, the original content
// should be used instead of the converted content.
// As tabs are being converted to spaces by the Tokenizer, the
// original content should be used instead of the converted content.
if (isset($tokens[$i]['orig_content']) === true) {
$content = $tokens[$i]['orig_content'];
} else {
$content = $tokens[$i]['content'];
}

if ($content[0] === ' ') {
if ($tokens[$i]['code'] === T_DOC_COMMENT_WHITESPACE && $content === ' ') {
// Ignore file/class-level DocBlock.
continue;
// If this is an inline HTML token, split the content into
// indentation whitespace and the actual HTML/text.
$nonWhitespace = '';
if ($tokens[$i]['code'] === T_INLINE_HTML && preg_match('`^(\s*)(\S.*)`s', $content, $matches) > 0) {
if (isset($matches[1]) === true) {
$content = $matches[1];
}

if (isset($matches[2]) === true) {
$nonWhitespace = $matches[2];
}
}

$hasSpaces = strpos($content, ' ');
$hasTabs = strpos($content, "\t");

if ($hasSpaces === false && $hasTabs === false) {
// Empty line.
continue;
}

// Space are considered ok if they are proceeded by tabs and not followed
// by tabs, as is the case with standard docblock comments.
if ($hasSpaces === false && $hasTabs !== false) {
// All ok, nothing to do.
$phpcsFile->recordMetric($i, 'Line indent', 'tabs');
continue;
}

if ($tokens[$i]['code'] === T_DOC_COMMENT_WHITESPACE && $content === ' ') {
// Ignore file/class-level DocBlock, especially for recording metrics.
continue;
}

// OK, by now we know there will be spaces.
// We just don't know yet whether they need to be replaced or
// are precision indentation, nor whether they are correctly
// placed at the end of the whitespace.
$trimmed = str_replace(' ', '', $content);
$numSpaces = (strlen($content) - strlen($trimmed));
$numTabs = (int) floor($numSpaces / $this->_tabWidth);
$tabAfterSpaces = strpos($content, "\t", $hasSpaces);

if ($hasTabs === false) {
$phpcsFile->recordMetric($i, 'Line indent', 'spaces');
$error = 'Tabs must be used to indent lines; spaces are not allowed';
$fix = $phpcsFile->addFixableError($error, $i, 'SpacesUsed');
if ($fix === true) {
$trimmed = ltrim($content, ' ');
$numSpaces = (strlen($content) - strlen($trimmed));
if ($numSpaces < $this->_tabWidth) {
$numTabs = 1;
$padding = "\t";
} else {
$numTabs = floor($numSpaces / $this->_tabWidth);
$remaining = ($numSpaces - ($numTabs * $this->_tabWidth));
$padding = str_repeat("\t", $numTabs).$padding = str_repeat(' ', $remaining);
}

$phpcsFile->fixer->replaceToken($i, $padding.$trimmed);
if ($numTabs === 0) {
// Ignore: precision indentation.
continue;
}
} else {
if ($numTabs === 0) {
// Precision indentation.
Copy link
Copy Markdown

@Frenzie Frenzie Aug 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic looks very suspicious to me (compare #547 (comment)). Consider something like this:

		if (! ($env === 'silent'
		       || ($env === 'production'
		       && ($level >= Minz_Log::NOTICE)))) {

Greatly increasing the tab width doesn't suffice as a workaround because then you'll get nonsensical errors about line length.

$phpcsFile->recordMetric($i, 'Line indent', 'tabs');

if ($tabAfterSpaces === false) {
// Ignore: precision indentation is already at the
// end of the whitespace.
continue;
}
} else {
$phpcsFile->recordMetric($i, 'Line indent', 'mixed');
}
} else if ($content[0] === "\t") {
$phpcsFile->recordMetric($i, 'Line indent', 'tabs');
}//end if

$error = 'Tabs must be used to indent lines; spaces are not allowed';
$errorCode = 'SpacesUsed';

if ($tabAfterSpaces !== false) {
$error = 'Tabs must be used to indent lines; spaces are only allowed at the end for precision indentation';
$errorCode = 'TabsAfterSpaces';
}

$fix = $phpcsFile->addFixableError($error, $i, $errorCode);
if ($fix === true) {
$remaining = ($numSpaces % $this->_tabWidth);
$padding = str_repeat("\t", $numTabs);
$padding .= str_repeat(' ', $remaining);
$phpcsFile->fixer->replaceToken($i, $trimmed.$padding.$nonWhitespace);
}
}//end for

// Ignore the rest of the file.
Expand Down
4 changes: 4 additions & 0 deletions CodeSniffer/Standards/Generic/Tests/WhiteSpace/DisallowSpaceIndentUnitTest.css.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#login-container {
margin-left: -225px;
width: 450px;
}
9 changes: 9 additions & 0 deletions CodeSniffer/Standards/Generic/Tests/WhiteSpace/DisallowSpaceIndentUnitTest.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,12 @@ $x = 1;
</div>
</body>
</html>

<?php

// Issue #1404
// This is a line with mixed tabs and spaces.
echo 'And here is another line with mixed tabs and spaces.'
echo 'And another one.'
echo 'And another one.'
echo 'And another one.'
21 changes: 15 additions & 6 deletions CodeSniffer/Standards/Generic/Tests/WhiteSpace/DisallowSpaceIndentUnitTest.inc.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@ EOF;
$correctVar = true;

// Indent with spaces is not allowed
$hello = array();
$world = '';
$hello = array();
$world = '';
// here the indention is mixed with tabs and spaces
// [tab][space][space][space][tab]return "<$name$xmlns$type_str $atts xsi:nil=\"true\"/>";
return "<$name$xmlns$type_str $atts xsi:nil=\"true\"/>";
return "<$name$xmlns$type_str $atts xsi:nil=\"true\"/>";
// [space][space][space][tab]return "<$name$xmlns$type_str $atts xsi:nil=\"true\"/>";
return "<$name$xmlns$type_str $atts xsi:nil=\"true\"/>";
return "<$name$xmlns$type_str $atts xsi:nil=\"true\"/>";
// Doc comments are indent with tabs and one space
//[tab]/**
//[tab][space]*
Expand Down Expand Up @@ -54,8 +54,17 @@ $x = 1;
<div>
<div>
<div>
</div>
</div>
</div>
</div>
</div>
</body>
</html>

<?php

// Issue #1404
// This is a line with mixed tabs and spaces.
echo 'And here is another line with mixed tabs and spaces.'
echo 'And another one.'
echo 'And another one.'
echo 'And another one.'
9 changes: 9 additions & 0 deletions CodeSniffer/Standards/Generic/Tests/WhiteSpace/DisallowSpaceIndentUnitTest.js.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
var x = {
abc: 1,
zyz: 2,
abc: 5,
mno: {
abc: 4
},
abc: 5
}
10 changes: 8 additions & 2 deletions CodeSniffer/Standards/Generic/Tests/WhiteSpace/DisallowSpaceIndentUnitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,16 +48,22 @@ public function getErrorList($testFile='DisallowSpaceIndentUnitTest.inc')
5 => 1,
9 => 1,
15 => 1,
18 => 1,
19 => 1,
22 => 1,
24 => 1,
30 => 1,
35 => 1,
50 => 1,
55 => 1,
57 => 1,
58 => 1,
59 => 1,
60 => 1,
65 => 1,
66 => 1,
67 => 1,
68 => 1,
69 => 1,
70 => 1,
);
break;
case 'DisallowSpaceIndentUnitTest.js':
Expand Down
2 changes: 2 additions & 0 deletions package.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,9 +804,11 @@ http://pear.php.net/dtd/package-2.0.xsd">
</dir>
<dir name="WhiteSpace">
<file baseinstalldir="PHP" name="DisallowSpaceIndentUnitTest.css" role="test" />
<file baseinstalldir="PHP" name="DisallowSpaceIndentUnitTest.css.fixed" role="test" />
<file baseinstalldir="PHP" name="DisallowSpaceIndentUnitTest.inc" role="test" />
<file baseinstalldir="PHP" name="DisallowSpaceIndentUnitTest.inc.fixed" role="test" />
<file baseinstalldir="PHP" name="DisallowSpaceIndentUnitTest.js" role="test" />
<file baseinstalldir="PHP" name="DisallowSpaceIndentUnitTest.js.fixed" role="test" />
<file baseinstalldir="PHP" name="DisallowSpaceIndentUnitTest.php" role="test">
<tasks:replace from="@package_version@" to="version" type="package-info" />
</file>
Expand Down