Skip to content

Commit 0fff3b5

Browse files
committed
Squiz/EmbeddedPhp: bug fix - content intended indent calculation
The intended content indent calculation contained a pretty nasty bug, though it is unlikely to have been hit much in real code bases. What was happening: * The sniff would try to find the first whitespace token on the line containing the stack pointer. If found, the size of the whitespace was taken as the required indent. This part was working correctly. * If no whitespace token was found, the sniff would try and find the first inline HTML token. If an inline HTML token was found, the leading whitespace in the inline HTML token was taken as the indent. This, again, is correct. * However, now it gets iffy... what if no whitespace or inline HTML token was found on the line containing the open tag ? I.e. the situation where the open tag _is_ the first token on the line. This situation was not accounted for by the sniff. The `File::findFirstOnLine()` method would return `false` in that case, which means that the indent calculation would end up like this: ```php $indent = (strlen($tokens[false]['content']) - strlen(ltrim($tokens[false]['content']))); ``` Now, `false` is not a valid array key, so this would end up being juggled to `0`, meaning the length of the leading whitespace for the first token in the **file** was being used. Fixed now. Includes various additional unit tests safeguarding the fix and one (in a separate file) demonstrating the situation where the bug would have had a real life impact.
1 parent efb9e44 commit 0fff3b5

File tree

6 files changed

+119
-3
lines changed

6 files changed

+119
-3
lines changed

src/Standards/Squiz/Sniffs/PHP/EmbeddedPhpSniff.php

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -147,10 +147,13 @@ private function validateMultilineEmbeddedPhp($phpcsFile, $stackPtr, $closingTag
147147
}
148148
}//end if
149149

150-
$first = $phpcsFile->findFirstOnLine(T_WHITESPACE, $stackPtr);
150+
$indent = 0;
151+
$first = $phpcsFile->findFirstOnLine(T_WHITESPACE, $stackPtr);
151152
if ($first === false) {
152-
$first = $phpcsFile->findFirstOnLine(T_INLINE_HTML, $stackPtr);
153-
$indent = (strlen($tokens[$first]['content']) - strlen(ltrim($tokens[$first]['content'])));
153+
$first = $phpcsFile->findFirstOnLine(T_INLINE_HTML, $stackPtr);
154+
if ($first !== false) {
155+
$indent = (strlen($tokens[$first]['content']) - strlen(ltrim($tokens[$first]['content'])));
156+
}
154157
} else {
155158
$indent = ($tokens[($first + 1)]['column'] - 1);
156159
}

src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.1.inc

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,35 @@ Safeguard that blank lines between a PHP tag and a scope closer are not touched
199199
}
200200
?>
201201

202+
<!--
203+
Safeguard correct content indent calculation.
204+
-->
205+
<?php
206+
echo 'indent is correct - first on line for open tag is open tag';
207+
?>
208+
209+
<?php
210+
echo 'indent is correct - first on line for open tag is inline HTML without content';
211+
?>
212+
<div><?php
213+
echo 'indent is correct - first on line for open tag is inline HTML with content';
214+
?><?php
215+
echo 'indent is correct - first on line for open tag is whitespace';
216+
?>
217+
218+
<?php
219+
echo 'indent is incorrect - first on line for open tag is open tag';
220+
?>
221+
222+
<?php
223+
echo 'indent is incorrect - first on line for open tag is inline HTML';
224+
?>
225+
<div><?php
226+
echo 'indent is correct - first on line for open tag is inline HTML with content';
227+
?><?php
228+
echo 'indent is incorrect - first on line for open tag is whitespace';
229+
?>
230+
202231
<?php
203232
// This test case file MUST always end with an unclosed long open PHP tag (with this comment) to prevent
204233
// the tests running into the "last PHP closing tag excepted" condition breaking tests.

src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.1.inc.fixed

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,39 @@ Safeguard that blank lines between a PHP tag and a scope closer are not touched
201201
}
202202
?>
203203

204+
<!--
205+
Safeguard correct content indent calculation.
206+
-->
207+
<?php
208+
echo 'indent is correct - first on line for open tag is open tag';
209+
?>
210+
211+
<?php
212+
echo 'indent is correct - first on line for open tag is inline HTML without content';
213+
?>
214+
<div>
215+
<?php
216+
echo 'indent is correct - first on line for open tag is inline HTML with content';
217+
?>
218+
<?php
219+
echo 'indent is correct - first on line for open tag is whitespace';
220+
?>
221+
222+
<?php
223+
echo 'indent is incorrect - first on line for open tag is open tag';
224+
?>
225+
226+
<?php
227+
echo 'indent is incorrect - first on line for open tag is inline HTML';
228+
?>
229+
<div>
230+
<?php
231+
echo 'indent is correct - first on line for open tag is inline HTML with content';
232+
?>
233+
<?php
234+
echo 'indent is incorrect - first on line for open tag is whitespace';
235+
?>
236+
204237
<?php
205238
// This test case file MUST always end with an unclosed long open PHP tag (with this comment) to prevent
206239
// the tests running into the "last PHP closing tag excepted" condition breaking tests.
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
<div>Inline HTML with indent to demonstrate the bug in the indent calculation.</div>
2+
<?php
3+
// This test case file MUST always start with a long open PHP tag set (with this comment) to prevent
4+
// the tests running into the "first PHP open tag excepted" condition breaking the tests.
5+
// Tests related to that "first PHP open tag excepted" condition should go in separate files.
6+
?>
7+
<!--
8+
Content indent calculation should not look at first token in the stack, but only at the line containing the PHP open tag for the block.
9+
-->
10+
<?php
11+
echo 'indent is correct - first on line for open tag is open tag';
12+
?>
13+
<?php
14+
echo 'indent is incorrect - first on line for open tag is inline HTML';
15+
?>
16+
17+
<?php
18+
// This test case file MUST always end with an unclosed long open PHP tag (with this comment) to prevent
19+
// the tests running into the "last PHP closing tag excepted" condition breaking tests.
20+
// Tests related to that "last PHP closing tag excepted" condition should go in separate files.
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
<div>Inline HTML with indent to demonstrate the bug in the indent calculation.</div>
2+
<?php
3+
// This test case file MUST always start with a long open PHP tag set (with this comment) to prevent
4+
// the tests running into the "first PHP open tag excepted" condition breaking the tests.
5+
// Tests related to that "first PHP open tag excepted" condition should go in separate files.
6+
?>
7+
<!--
8+
Content indent calculation should not look at first token in the stack, but only at the line containing the PHP open tag for the block.
9+
-->
10+
<?php
11+
echo 'indent is correct - first on line for open tag is open tag';
12+
?>
13+
<?php
14+
echo 'indent is incorrect - first on line for open tag is inline HTML';
15+
?>
16+
17+
<?php
18+
// This test case file MUST always end with an unclosed long open PHP tag (with this comment) to prevent
19+
// the tests running into the "last PHP closing tag excepted" condition breaking tests.
20+
// Tests related to that "last PHP closing tag excepted" condition should go in separate files.

src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.php

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,14 @@ public function getErrorList($testFile='')
8585
180 => 2,
8686
181 => 1,
8787
189 => 1,
88+
212 => 1,
89+
214 => 2,
90+
219 => 1,
91+
223 => 1,
92+
225 => 1,
93+
226 => 1,
94+
227 => 2,
95+
228 => 1,
8896
];
8997

9098
case 'EmbeddedPhpUnitTest.2.inc':
@@ -169,6 +177,9 @@ public function getErrorList($testFile='')
169177
case 'EmbeddedPhpUnitTest.21.inc':
170178
return [12 => 2];
171179

180+
case 'EmbeddedPhpUnitTest.22.inc':
181+
return [14 => 1];
182+
172183
default:
173184
return [];
174185
}//end switch

0 commit comments

Comments
 (0)