Skip to content

Commit 3172a21

Browse files
jrfnljrfnl
committed
Squiz/EmbeddedPhp: sniff for short open tags
As things were, the `Squiz.PHP.EmbeddedPhp` sniff would only check the formatting for code embedded using long PHP open tags, while short PHP open tags are pretty common for embedded PHP. This commit adds support for examining short PHP open tags to the sniff. Notes: * The error codes used are the same for both type of open tags, with one exception: for single line embedded statements, it is checked that there is a semi-colon at the end of the statement. The "statement" in an embedded snippet using short open tags may just be a single variable and in _some_ lines of thinking it is acceptable to leave out the semi-colon in that case. To accommodate this, the error code for the missing semi-colon differentiates between whether the statement uses long open tags `NoSemicolon` (same as before) or short open tags `ShortOpenEchoNoSemicolon`. This will allows for selectively excluding the `ShortOpenEchoNoSemicolon` error code. * I've considered enforcing that short open tags always are always used as a single-line statement, or adding an option to enforce this, but have decided against this at this time. After all, this sniff is about _consistent_ formatting, single line stays single line and multi-line is made consistently/properly multi-line. It could be considered future scope to add a separate sniff to enforce short open echo tag sets to always be single line when they only contain a single statement. Fixes 27
1 parent 1e64373 commit 3172a21

12 files changed

+450
-14
lines changed

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

Lines changed: 32 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,10 @@ class EmbeddedPhpSniff implements Sniff
2424
*/
2525
public function register()
2626
{
27-
return [T_OPEN_TAG];
27+
return [
28+
T_OPEN_TAG,
29+
T_OPEN_TAG_WITH_ECHO,
30+
];
2831

2932
}//end register()
3033

@@ -69,7 +72,7 @@ private function validateMultilineEmbeddedPhp($phpcsFile, $stackPtr, $closingTag
6972
{
7073
$tokens = $phpcsFile->getTokens();
7174

72-
$prevTag = $phpcsFile->findPrevious(T_OPEN_TAG, ($stackPtr - 1));
75+
$prevTag = $phpcsFile->findPrevious($this->register(), ($stackPtr - 1));
7376
if ($prevTag === false) {
7477
// This is the first open tag.
7578
return;
@@ -262,7 +265,9 @@ private function validateMultilineEmbeddedPhp($phpcsFile, $stackPtr, $closingTag
262265
}
263266
}
264267

265-
if ($tokens[$firstContentAfterBlock]['code'] === T_OPEN_TAG) {
268+
if ($tokens[$firstContentAfterBlock]['code'] === T_OPEN_TAG
269+
|| $tokens[$firstContentAfterBlock]['code'] === T_OPEN_TAG_WITH_ECHO
270+
) {
266271
// Next token is a PHP open tag which will also have thrown an error.
267272
// Prevent both fixers running in the same loop by making sure the token is "touched" during this loop.
268273
// This prevents a stray new line being added between the close and open tags.
@@ -273,7 +278,7 @@ private function validateMultilineEmbeddedPhp($phpcsFile, $stackPtr, $closingTag
273278
}//end if
274279
}//end if
275280

276-
$next = $phpcsFile->findNext(T_OPEN_TAG, ($closingTag + 1));
281+
$next = $phpcsFile->findNext($this->register(), ($closingTag + 1));
277282
if ($next === false) {
278283
return;
279284
}
@@ -333,18 +338,30 @@ private function validateInlineEmbeddedPhp($phpcsFile, $stackPtr, $closeTag)
333338
}
334339

335340
// Check that there is one, and only one space at the start of the statement.
336-
// The open tag token always contains a single space after it.
337-
$leadingSpace = 1;
341+
$leadingSpace = 0;
342+
if ($tokens[$stackPtr]['code'] === T_OPEN_TAG) {
343+
// The long open tag token in a single line tag set always contains a single space after it.
344+
$leadingSpace = 1;
345+
}
346+
338347
if ($tokens[($stackPtr + 1)]['code'] === T_WHITESPACE) {
339-
$leadingSpace = ($tokens[($stackPtr + 1)]['length'] + 1);
348+
$leadingSpace += $tokens[($stackPtr + 1)]['length'];
340349
}
341350

342351
if ($leadingSpace !== 1) {
343352
$error = 'Expected 1 space after opening PHP tag; %s found';
344353
$data = [$leadingSpace];
345354
$fix = $phpcsFile->addFixableError($error, $stackPtr, 'SpacingAfterOpen', $data);
346355
if ($fix === true) {
347-
$phpcsFile->fixer->replaceToken(($stackPtr + 1), '');
356+
if ($tokens[$stackPtr]['code'] === T_OPEN_TAG) {
357+
$phpcsFile->fixer->replaceToken(($stackPtr + 1), '');
358+
} else if ($tokens[($stackPtr + 1)]['code'] === T_WHITESPACE) {
359+
// Short open tag with too much whitespace.
360+
$phpcsFile->fixer->replaceToken(($stackPtr + 1), ' ');
361+
} else {
362+
// Short open tag without whitespace.
363+
$phpcsFile->fixer->addContent($stackPtr, ' ');
364+
}
348365
}
349366
}
350367

@@ -357,7 +374,12 @@ private function validateInlineEmbeddedPhp($phpcsFile, $stackPtr, $closeTag)
357374
&& $tokens[$prev]['code'] !== T_SEMICOLON
358375
) {
359376
$error = 'Inline PHP statement must end with a semicolon';
360-
$fix = $phpcsFile->addFixableError($error, $stackPtr, 'NoSemicolon');
377+
$code = 'NoSemicolon';
378+
if ($tokens[$stackPtr]['code'] === T_OPEN_TAG_WITH_ECHO) {
379+
$code = 'ShortOpenEchoNoSemicolon';
380+
}
381+
382+
$fix = $phpcsFile->addFixableError($error, $stackPtr, $code);
361383
if ($fix === true) {
362384
$phpcsFile->fixer->addContent($prev, ';');
363385
}
@@ -374,7 +396,7 @@ private function validateInlineEmbeddedPhp($phpcsFile, $stackPtr, $closeTag)
374396
$data = [$statementCount];
375397
$phpcsFile->addError($error, $stackPtr, 'MultipleStatements', $data);
376398
}
377-
}
399+
}//end if
378400
}//end if
379401

380402
$trailingSpace = 0;

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
<?php
22
// Not embedded. Do not check here.
3-
echo 'hello';
3+
echo 'Long open tag test case file';
44
?>
55
<html>
66
<head>

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
<?php
22
// Not embedded. Do not check here.
3-
echo 'hello';
3+
echo 'Long open tag test case file';
44
?>
55
<html>
66
<head>

‎src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.2.inc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
<!--
2-
Test file safeguarding that if a file only contains single line embedded statements with a complete set of tags,
2+
Test file safeguarding that if a file only contains single line embedded statements with a complete set of long open tags,
33
these will always be examined and that the special casing of the first/last PHP block does not apply.
44
-->
55
<?php if ($var) { ?>

‎src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.2.inc.fixed‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
<!--
2-
Test file safeguarding that if a file only contains single line embedded statements with a complete set of tags,
2+
Test file safeguarding that if a file only contains single line embedded statements with a complete set of long open tags,
33
these will always be examined and that the special casing of the first/last PHP block does not apply.
44
-->
55
<?php if ($var) { ?>

‎src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.3.inc‎

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
<?php
2+
// This test case file MUST always start with a long open PHP tag set (with this comment) to prevent
3+
// the tests running into the "first PHP open tag excepted" condition breaking the tests.
4+
// Tests related to that "first PHP open tag excepted" condition should go in separate files.
5+
6+
// This test case file only deals with SHORT OPEN ECHO TAGS.
7+
?>
8+
<html>
9+
<head>
10+
<title><?= $title ?></title>
11+
<script><?= $script; ?></script>
12+
</head>
13+
<body>
14+
<?=
15+
$body;
16+
?>
17+
hello
18+
<?=
19+
$moreBody;
20+
?>
21+
<?= 'one'; ?>
22+
<?= 'two'; ?>
23+
<?= 'three;' ?>
24+
<?= 'fourA'; echo 'fourB;'; ?>
25+
<?= 'fiveA'; echo 'fiveB;'; ?>
26+
27+
<?=
28+
29+
$start - $indent + $end;
30+
31+
?>
32+
<?=
33+
34+
$blankLines;
35+
36+
?>
37+
38+
<?=
39+
$closerNeedsOwnLine; ?>
40+
<?= $openerNeedsOwnLine;
41+
?>
42+
43+
<?=
44+
'hi';
45+
?>
46+
47+
<?=
48+
'hi';
49+
?>
50+
51+
<a onClick="Javascript: set_hidden_field('<?= $link_offset - $num_per_page; ?>'); set_hidden_field('process_form', '0'); submit_form(); return false;">
52+
53+
<strong><?=
54+
'foo';
55+
?></strong>
56+
57+
</body>
58+
</html>
59+
60+
<?= 'okay'; // Something. ?>
61+
<?= 'too much space before close'; // Something. ?>
62+
<?= 'no space before close'; // Something.?>
63+
64+
<?= 'okay'; /* translators: okay */ ?>
65+
<?= 'oops' /* translators: no space before close */?>
66+
<?= 'oops' /* translators: too much space before close */ ?>
67+
68+
<?= 'okay'; // phpcs:ignore Standard.Category.Sniff -- reason. ?>
69+
<?= 'too much space before close'; // phpcs:ignore Standard.Category.Sniff -- reason. ?>
70+
<?= 'no space before close'; // phpcs:ignore Standard.Category.Sniff -- reason.?>
71+
72+
<!--
73+
Make sure the "content after opener" fixer does not leave trailing space behind.
74+
-->
75+
<?= $openerNeedsOwnLine;
76+
?>
77+
78+
<!--
79+
Make sure the "content before closer" fixer does not leave trailing space behind.
80+
-->
81+
<?=
82+
$closerNeedsOwnLine; ?>
83+
84+
<!--
85+
Make sure the "content after closer" fixer does not leave trailing space behind.
86+
-->
87+
<?=
88+
$closerNeedsOwnLine;
89+
?> </div>
90+
91+
<?=
92+
$closerNeedsOwnLine;
93+
?> <?= $i; ?>
94+
95+
<!--
96+
Safeguard fixing when there is no whitespace between the open tag and the contents.
97+
-->
98+
<?=$var ?>
99+
<?=$var;
100+
?>
101+
102+
// Safeguard fixing when there is no whitespace between the close tag and the contents.
103+
<?= $var?>
104+
<?=
105+
$var;?>
106+
107+
<!--
108+
Make sure the fixer does not add stray new lines when there are consecutive PHP blocks.
109+
-->
110+
<?=
111+
'something'
112+
?><?=
113+
'embedded';
114+
?> <?=
115+
'embedded';
116+
?><?=
117+
'embedded';
118+
?>
119+
120+
<?php
121+
// This test case file MUST always end with an unclosed long open PHP tag (with this comment) to prevent
122+
// the tests running into the "last PHP closing tag excepted" condition breaking tests.
123+
// Tests related to that "last PHP closing tag excepted" condition should go in separate files.

‎src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.3.inc.fixed‎

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
<?php
2+
// This test case file MUST always start with a long open PHP tag set (with this comment) to prevent
3+
// the tests running into the "first PHP open tag excepted" condition breaking the tests.
4+
// Tests related to that "first PHP open tag excepted" condition should go in separate files.
5+
6+
// This test case file only deals with SHORT OPEN ECHO TAGS.
7+
?>
8+
<html>
9+
<head>
10+
<title><?= $title; ?></title>
11+
<script><?= $script; ?></script>
12+
</head>
13+
<body>
14+
<?=
15+
$body;
16+
?>
17+
hello
18+
<?=
19+
$moreBody;
20+
?>
21+
<?= 'one'; ?>
22+
<?= 'two'; ?>
23+
<?= 'three;'; ?>
24+
<?= 'fourA'; echo 'fourB;'; ?>
25+
<?= 'fiveA'; echo 'fiveB;'; ?>
26+
27+
<?=
28+
$start - $indent + $end;
29+
?>
30+
<?=
31+
$blankLines;
32+
?>
33+
34+
<?=
35+
$closerNeedsOwnLine;
36+
?>
37+
<?=
38+
$openerNeedsOwnLine;
39+
?>
40+
41+
<?=
42+
'hi';
43+
?>
44+
45+
<?=
46+
'hi';
47+
?>
48+
49+
<a onClick="Javascript: set_hidden_field('<?= $link_offset - $num_per_page; ?>'); set_hidden_field('process_form', '0'); submit_form(); return false;">
50+
51+
<strong>
52+
<?=
53+
'foo';
54+
?>
55+
</strong>
56+
57+
</body>
58+
</html>
59+
60+
<?= 'okay'; // Something. ?>
61+
<?= 'too much space before close'; // Something. ?>
62+
<?= 'no space before close'; // Something. ?>
63+
64+
<?= 'okay'; /* translators: okay */ ?>
65+
<?= 'oops'; /* translators: no space before close */ ?>
66+
<?= 'oops'; /* translators: too much space before close */ ?>
67+
68+
<?= 'okay'; // phpcs:ignore Standard.Category.Sniff -- reason. ?>
69+
<?= 'too much space before close'; // phpcs:ignore Standard.Category.Sniff -- reason. ?>
70+
<?= 'no space before close'; // phpcs:ignore Standard.Category.Sniff -- reason. ?>
71+
72+
<!--
73+
Make sure the "content after opener" fixer does not leave trailing space behind.
74+
-->
75+
<?=
76+
$openerNeedsOwnLine;
77+
?>
78+
79+
<!--
80+
Make sure the "content before closer" fixer does not leave trailing space behind.
81+
-->
82+
<?=
83+
$closerNeedsOwnLine;
84+
?>
85+
86+
<!--
87+
Make sure the "content after closer" fixer does not leave trailing space behind.
88+
-->
89+
<?=
90+
$closerNeedsOwnLine;
91+
?>
92+
</div>
93+
94+
<?=
95+
$closerNeedsOwnLine;
96+
?>
97+
<?= $i; ?>
98+
99+
<!--
100+
Safeguard fixing when there is no whitespace between the open tag and the contents.
101+
-->
102+
<?= $var; ?>
103+
<?=
104+
$var;
105+
?>
106+
107+
// Safeguard fixing when there is no whitespace between the close tag and the contents.
108+
<?= $var; ?>
109+
<?=
110+
$var;
111+
?>
112+
113+
<!--
114+
Make sure the fixer does not add stray new lines when there are consecutive PHP blocks.
115+
-->
116+
<?=
117+
'something'
118+
?>
119+
<?=
120+
'embedded';
121+
?>
122+
<?=
123+
'embedded';
124+
?>
125+
<?=
126+
'embedded';
127+
?>
128+
129+
<?php
130+
// This test case file MUST always end with an unclosed long open PHP tag (with this comment) to prevent
131+
// the tests running into the "last PHP closing tag excepted" condition breaking tests.
132+
// Tests related to that "last PHP closing tag excepted" condition should go in separate files.

‎src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.4.inc‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
<!--
2+
Test file safeguarding that if a file only contains single line embedded statements with a complete set of short open tags,
3+
these will always be examined and that the special casing of the first/last PHP block does not apply.
4+
-->
5+
<?= $paragraph1; ?>
6+
<?= $paragraph2 ?>
7+
<?= $paragraph3; ?>

0 commit comments

Comments
 (0)