Skip to content

Commit cb9b4c5

Browse files
jrfnljrfnl
committed
Squiz.CSS.ClassDefinitionOpeningBrace: add fixed file and various bug fixes
The sniff contains fixers, but didn't have a `fixed` file. While I was add it, I've reviewed the sniff and applied any improvements I could think of: * For the `Before` error code, the sniff would report on a `tab` being found if the whitespace length was 1, but not a space. This meant that for a "3 space" whitespace token, it would also report as `tab`. I've now changed the `$length` determination to be more accurate. * The fixer for the `Before` error will now fix the whitespace between in one go if there is more than one whitespace token. Previously, it would replace the first preceding whitespace token with a space, which in effect left any additional new lines in place as the fixer would not be triggered again. * While the `ContentBefore` error states "Opening brace should be the last content on the line", the way the code was, allowed for trailing comments on the line of the opening brace. I've elected not to change this behaviour, but have documented it with a unit test. * The fixer for the `AfterNesting` error would remove the first non-whitespace token after the opening brace if too many lines after the opener were found. This changes the meaning of the style declaration and/or could remove comments. The existing unit test on line 33 demonstrates this, as well as the new unit tests on line 88 and 95. * The fixer for `AfterNesting` would also inadvertently remove line indentation. Indentation is not the concern of this sniff, so should be left alone. * The `AfterNesting` check would also **not** report an error if the line between the original opening brace and the nested style declaration, contained a comment. * Lastly, the handling of trailing comments was inconsistent with the handling of these in the `ContentBefore` part, i.e. for `ContentBefore` they would be allowed, but if the `AfterNesting` error was triggered, the fixer would move them. * Improved the resilience of the sniff against syntax/parse errors and during live coding. This prevents an `Undefined index: bracket_closer` error. * Minor other defensive coding improvements. In effect, this constitutes a complete rewrite of this sniff. Includes additional unit tests covering all fixes.
1 parent b53f64e commit cb9b4c5

File tree

5 files changed

+277
-51
lines changed

5 files changed

+277
-51
lines changed

‎package.xml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1474,6 +1474,7 @@ http://pear.php.net/dtd/package-2.0.xsd">
14741474
<file baseinstalldir="PHP/CodeSniffer" name="ClassDefinitionNameSpacingUnitTest.css" role="test" />
14751475
<file baseinstalldir="PHP/CodeSniffer" name="ClassDefinitionNameSpacingUnitTest.php" role="test" />
14761476
<file baseinstalldir="PHP/CodeSniffer" name="ClassDefinitionOpeningBraceSpaceUnitTest.css" role="test" />
1477+
<file baseinstalldir="PHP/CodeSniffer" name="ClassDefinitionOpeningBraceSpaceUnitTest.css.fixed" role="test" />
14771478
<file baseinstalldir="PHP/CodeSniffer" name="ClassDefinitionOpeningBraceSpaceUnitTest.php" role="test" />
14781479
<file baseinstalldir="PHP/CodeSniffer" name="ColonSpacingUnitTest.css" role="test" />
14791480
<file baseinstalldir="PHP/CodeSniffer" name="ColonSpacingUnitTest.css.fixed" role="test" />

‎src/Standards/Squiz/Sniffs/CSS/ClassDefinitionOpeningBraceSpaceSniff.php

Lines changed: 96 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -47,79 +47,124 @@ public function register()
4747
*/
4848
public function process(File $phpcsFile, $stackPtr)
4949
{
50-
$tokens = $phpcsFile->getTokens();
51-
52-
if ($tokens[($stackPtr - 1)]['code'] !== T_WHITESPACE) {
53-
$error = 'Expected 1 space before opening brace of class definition; 0 found';
54-
$fix = $phpcsFile->addFixableError($error, $stackPtr, 'NoneBefore');
55-
if ($fix === true) {
56-
$phpcsFile->fixer->addContentBefore($stackPtr, ' ');
57-
}
58-
} else {
59-
$content = $tokens[($stackPtr - 1)]['content'];
60-
if ($content !== ' ') {
61-
if ($tokens[($stackPtr - 1)]['line'] < $tokens[$stackPtr]['line']) {
62-
$length = 'newline';
50+
$tokens = $phpcsFile->getTokens();
51+
$prevNonWhitespace = $phpcsFile->findPrevious(T_WHITESPACE, ($stackPtr - 1), null, true);
52+
53+
if ($prevNonWhitespace !== false) {
54+
$length = 0;
55+
if ($tokens[$stackPtr]['line'] !== $tokens[$prevNonWhitespace]['line']) {
56+
$length = 'newline';
57+
} else if ($tokens[($stackPtr - 1)]['code'] === T_WHITESPACE) {
58+
if (strpos($tokens[($stackPtr - 1)]['content'], "\t") !== false) {
59+
$length = 'tab';
6360
} else {
64-
$length = strlen($content);
65-
if ($length === 1) {
66-
$length = 'tab';
67-
}
61+
$length = $tokens[($stackPtr - 1)]['length'];
6862
}
63+
}
6964

65+
if ($length === 0) {
66+
$error = 'Expected 1 space before opening brace of class definition; 0 found';
67+
$fix = $phpcsFile->addFixableError($error, $stackPtr, 'NoneBefore');
68+
if ($fix === true) {
69+
$phpcsFile->fixer->addContentBefore($stackPtr, ' ');
70+
}
71+
} else if ($length !== 1) {
7072
$error = 'Expected 1 space before opening brace of class definition; %s found';
7173
$data = [$length];
7274
$fix = $phpcsFile->addFixableError($error, $stackPtr, 'Before', $data);
7375
if ($fix === true) {
74-
$phpcsFile->fixer->replaceToken(($stackPtr - 1), ' ');
76+
$phpcsFile->fixer->beginChangeset();
77+
78+
for ($i = ($stackPtr - 1); $i > $prevNonWhitespace; $i--) {
79+
$phpcsFile->fixer->replaceToken($i, '');
80+
}
81+
82+
$phpcsFile->fixer->addContentBefore($stackPtr, ' ');
83+
$phpcsFile->fixer->endChangeset();
7584
}
76-
}
85+
}//end if
7786
}//end if
7887

79-
$next = $phpcsFile->findNext(Tokens::$emptyTokens, ($stackPtr + 1), null, true);
80-
if ($next === false) {
88+
$nextNonEmpty = $phpcsFile->findNext(Tokens::$emptyTokens, ($stackPtr + 1), null, true);
89+
if ($nextNonEmpty === false) {
8190
return;
8291
}
8392

84-
// Check for nested class definitions.
85-
$nested = false;
86-
$found = $phpcsFile->findNext(
87-
T_OPEN_CURLY_BRACKET,
88-
($stackPtr + 1),
89-
$tokens[$stackPtr]['bracket_closer']
90-
);
91-
92-
if ($found !== false) {
93-
$nested = true;
94-
}
95-
96-
if ($tokens[$next]['line'] === $tokens[$stackPtr]['line']) {
93+
if ($tokens[$nextNonEmpty]['line'] === $tokens[$stackPtr]['line']) {
9794
$error = 'Opening brace should be the last content on the line';
9895
$fix = $phpcsFile->addFixableError($error, $stackPtr, 'ContentBefore');
9996
if ($fix === true) {
97+
$phpcsFile->fixer->beginChangeset();
10098
$phpcsFile->fixer->addNewline($stackPtr);
99+
100+
// Remove potentially left over trailing whitespace.
101+
if ($tokens[($stackPtr + 1)]['code'] === T_WHITESPACE) {
102+
$phpcsFile->fixer->replaceToken(($stackPtr + 1), '');
103+
}
104+
105+
$phpcsFile->fixer->endChangeset();
101106
}
102107
} else {
103-
$foundLines = ($tokens[$next]['line'] - $tokens[$stackPtr]['line'] - 1);
104-
if ($nested === true) {
105-
if ($foundLines !== 1) {
106-
$error = 'Expected 1 blank line after opening brace of nesting class definition; %s found';
107-
$data = [$foundLines];
108-
$fix = $phpcsFile->addFixableError($error, $stackPtr, 'AfterNesting', $data);
109-
110-
if ($fix === true) {
111-
if ($foundLines === 0) {
112-
$phpcsFile->fixer->addNewline($stackPtr);
113-
} else {
114-
$next = $phpcsFile->findNext(T_WHITESPACE, ($stackPtr + 1), null, true);
115-
$phpcsFile->fixer->beginChangeset();
116-
for ($i = ($stackPtr + 1); $i < ($next + 1); $i++) {
117-
$phpcsFile->fixer->replaceToken($i, '');
108+
if (isset($tokens[$stackPtr]['bracket_closer']) === false) {
109+
// Syntax error or live coding, bow out.
110+
return;
111+
}
112+
113+
// Check for nested class definitions.
114+
$found = $phpcsFile->findNext(
115+
T_OPEN_CURLY_BRACKET,
116+
($stackPtr + 1),
117+
$tokens[$stackPtr]['bracket_closer']
118+
);
119+
120+
if ($found === false) {
121+
// Not nested.
122+
return;
123+
}
124+
125+
$lastOnLine = $stackPtr;
126+
for ($lastOnLine; $lastOnLine < $tokens[$stackPtr]['bracket_closer']; $lastOnLine++) {
127+
if ($tokens[$lastOnLine]['line'] !== $tokens[($lastOnLine + 1)]['line']) {
128+
break;
129+
}
130+
}
131+
132+
$nextNonWhiteSpace = $phpcsFile->findNext(T_WHITESPACE, ($lastOnLine + 1), null, true);
133+
if ($nextNonWhiteSpace === false) {
134+
return;
135+
}
136+
137+
$foundLines = ($tokens[$nextNonWhiteSpace]['line'] - $tokens[$stackPtr]['line'] - 1);
138+
if ($foundLines !== 1) {
139+
$error = 'Expected 1 blank line after opening brace of nesting class definition; %s found';
140+
$data = [max(0, $foundLines)];
141+
$fix = $phpcsFile->addFixableError($error, $stackPtr, 'AfterNesting', $data);
142+
143+
if ($fix === true) {
144+
$firstOnNextLine = $nextNonWhiteSpace;
145+
while ($tokens[$firstOnNextLine]['column'] !== 1) {
146+
--$firstOnNextLine;
147+
}
148+
149+
if ($found < 0) {
150+
// First statement on same line as the opening brace.
151+
$phpcsFile->fixer->addContentBefore($nextNonWhiteSpace, $phpcsFile->eolChar.$phpcsFile->eolChar);
152+
} else if ($found === 0) {
153+
// Next statement on next line, no blank line.
154+
$phpcsFile->fixer->addNewlineBefore($firstOnNextLine);
155+
} else {
156+
// Too many blank lines.
157+
$phpcsFile->fixer->beginChangeset();
158+
for ($i = ($firstOnNextLine - 1); $i > $stackPtr; $i--) {
159+
if ($tokens[$i]['code'] !== T_WHITESPACE) {
160+
break;
118161
}
119162

120-
$phpcsFile->fixer->addNewline($stackPtr);
121-
$phpcsFile->fixer->endChangeset();
163+
$phpcsFile->fixer->replaceToken($i, '');
122164
}
165+
166+
$phpcsFile->fixer->addContentBefore($firstOnNextLine, $phpcsFile->eolChar.$phpcsFile->eolChar);
167+
$phpcsFile->fixer->endChangeset();
123168
}
124169
}//end if
125170
}//end if

‎src/Standards/Squiz/Tests/CSS/ClassDefinitionOpeningBraceSpaceUnitTest.css

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,3 +41,68 @@
4141
}
4242

4343
.GUITextBox.container:after {}
44+
45+
.single-line {float: left;}
46+
47+
#opening-brace-on-different-line
48+
49+
50+
{
51+
color: #FFFFFF;
52+
}
53+
54+
#opening-brace-on-different-line-and-inline-style
55+
56+
57+
{color: #FFFFFF;}
58+
59+
@media screen and (max-device-width: 769px) { .everything-on-one-line { float: left; } }
60+
61+
/* Document handling of comments in various places */
62+
.no-space-before-opening-with-comment /* comment*/{
63+
float: left;
64+
}
65+
66+
.space-before-opening-with-comment-on-next-line
67+
/* comment*/ {
68+
float: left;
69+
}
70+
71+
#opening-brace-on-different-line-with-comment-between
72+
/*comment*/
73+
{
74+
padding: 0;
75+
}
76+
77+
.single-line-with-comment { /* comment*/ float: left; }
78+
79+
.multi-line-with-trailing-comment { /* comment*/
80+
float: left;
81+
}
82+
83+
84+
@media screen and (max-device-width: 769px) {
85+
/*comment*/
86+
.comment-line-after-nesting-class-opening {
87+
}
88+
}
89+
90+
@media screen and (max-device-width: 769px) {
91+
92+
/*comment*/
93+
.blank-line-and-comment-line-after-nesting-class-opening {
94+
}
95+
}
96+
97+
@media screen and (max-device-width: 769px) {
98+
99+
100+
101+
/* phpcs:ignore Standard.Category.Sniffname -- for reasons */
102+
.blank-line-and-annotation-after-nesting-class-opening {
103+
}
104+
}
105+
106+
/* Live coding. Has to be the last test in the file. */
107+
.intentional-parse-error {
108+
float: left

‎src/Standards/Squiz/Tests/CSS/ClassDefinitionOpeningBraceSpaceUnitTest.css.fixed

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
.HelpWidgetType-new-bug-title {
2+
float: left;
3+
}
4+
.HelpWidgetType-new-bug-title {
5+
float: left;
6+
}
7+
.HelpWidgetType-new-bug-title {
8+
float: left;
9+
}
10+
.HelpWidgetType-new-bug-title {
11+
float: left;
12+
}
13+
.HelpWidgetType-new-bug-title {
14+
15+
float: left;
16+
}
17+
18+
@media screen and (max-device-width: 769px) {
19+
20+
header #logo img {
21+
max-width: 100%;
22+
}
23+
24+
}
25+
26+
@media screen and (max-device-width: 769px) {
27+
28+
header #logo img {
29+
max-width: 100%;
30+
}
31+
32+
}
33+
34+
@media screen and (max-device-width: 769px) {
35+
36+
header #logo img {
37+
max-width: 100%;
38+
}
39+
40+
}
41+
42+
.GUITextBox.container:after {
43+
}
44+
45+
.single-line {
46+
float: left;}
47+
48+
#opening-brace-on-different-line {
49+
color: #FFFFFF;
50+
}
51+
52+
#opening-brace-on-different-line-and-inline-style {
53+
color: #FFFFFF;}
54+
55+
@media screen and (max-device-width: 769px) {
56+
57+
.everything-on-one-line {
58+
float: left; } }
59+
60+
/* Document handling of comments in various places */
61+
.no-space-before-opening-with-comment /* comment*/ {
62+
float: left;
63+
}
64+
65+
.space-before-opening-with-comment-on-next-line
66+
/* comment*/ {
67+
float: left;
68+
}
69+
70+
#opening-brace-on-different-line-with-comment-between
71+
/*comment*/ {
72+
padding: 0;
73+
}
74+
75+
.single-line-with-comment {
76+
/* comment*/ float: left; }
77+
78+
.multi-line-with-trailing-comment { /* comment*/
79+
float: left;
80+
}
81+
82+
83+
@media screen and (max-device-width: 769px) {
84+
85+
/*comment*/
86+
.comment-line-after-nesting-class-opening {
87+
}
88+
}
89+
90+
@media screen and (max-device-width: 769px) {
91+
92+
/*comment*/
93+
.blank-line-and-comment-line-after-nesting-class-opening {
94+
}
95+
}
96+
97+
@media screen and (max-device-width: 769px) {
98+
99+
/* phpcs:ignore Standard.Category.Sniffname -- for reasons */
100+
.blank-line-and-annotation-after-nesting-class-opening {
101+
}
102+
}
103+
104+
/* Live coding. Has to be the last test in the file. */
105+
.intentional-parse-error {
106+
float: left

‎src/Standards/Squiz/Tests/CSS/ClassDefinitionOpeningBraceSpaceUnitTest.php

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,15 @@ public function getErrorList()
3232
26 => 1,
3333
33 => 1,
3434
43 => 1,
35+
45 => 1,
36+
50 => 1,
37+
57 => 2,
38+
59 => 2,
39+
62 => 1,
40+
73 => 1,
41+
77 => 1,
42+
84 => 1,
43+
97 => 1,
3544
];
3645

3746
}//end getErrorList()

0 commit comments

Comments
 (0)