Skip to content

Commit eb3454f

Browse files
jrfnljrfnl
authored
Merge pull request #558 from rodrigoprimo/test-coverage-unnecessary-string-concat
Generic/UnnecessaryStringConcat: improve code coverage
2 parents 5932287 + 0b4f97e commit eb3454f

File tree

5 files changed

+97
-63
lines changed

5 files changed

+97
-63
lines changed

‎src/Standards/Generic/Sniffs/Strings/UnnecessaryStringConcatSniff.php

Lines changed: 43 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -70,54 +70,58 @@ public function register()
7070
*/
7171
public function process(File $phpcsFile, $stackPtr)
7272
{
73-
// Work out which type of file this is for.
7473
$tokens = $phpcsFile->getTokens();
75-
if ($tokens[$stackPtr]['code'] === T_STRING_CONCAT) {
76-
if ($phpcsFile->tokenizerType === 'JS') {
77-
return;
78-
}
79-
} else {
80-
if ($phpcsFile->tokenizerType === 'PHP') {
81-
return;
82-
}
74+
75+
if ($tokens[$stackPtr]['code'] === T_STRING_CONCAT && $phpcsFile->tokenizerType === 'JS') {
76+
// JS uses T_PLUS for string concatenation, not T_STRING_CONCAT.
77+
return;
78+
} else if ($tokens[$stackPtr]['code'] === T_PLUS && $phpcsFile->tokenizerType === 'PHP') {
79+
// PHP uses T_STRING_CONCAT for string concatenation, not T_PLUS.
80+
return;
8381
}
8482

8583
$prev = $phpcsFile->findPrevious(T_WHITESPACE, ($stackPtr - 1), null, true);
8684
$next = $phpcsFile->findNext(T_WHITESPACE, ($stackPtr + 1), null, true);
87-
if ($prev === false || $next === false) {
85+
if ($next === false) {
86+
return;
87+
}
88+
89+
if (isset(Tokens::$stringTokens[$tokens[$prev]['code']]) === false
90+
|| isset(Tokens::$stringTokens[$tokens[$next]['code']]) === false
91+
) {
92+
// Bow out as at least one of the two tokens being concatenated is not a string.
8893
return;
8994
}
9095

91-
if (isset(Tokens::$stringTokens[$tokens[$prev]['code']]) === true
92-
&& isset(Tokens::$stringTokens[$tokens[$next]['code']]) === true
96+
if ($tokens[$prev]['content'][0] !== $tokens[$next]['content'][0]) {
97+
// Bow out as the two strings are not of the same type.
98+
return;
99+
}
100+
101+
// Before we throw an error for PHP, allow strings to be
102+
// combined if they would have < and ? next to each other because
103+
// this trick is sometimes required in PHP strings.
104+
if ($phpcsFile->tokenizerType === 'PHP') {
105+
$prevChar = substr($tokens[$prev]['content'], -2, 1);
106+
$nextChar = $tokens[$next]['content'][1];
107+
$combined = $prevChar.$nextChar;
108+
if ($combined === '?'.'>' || $combined === '<'.'?') {
109+
return;
110+
}
111+
}
112+
113+
if ($this->allowMultiline === true
114+
&& $tokens[$prev]['line'] !== $tokens[$next]['line']
93115
) {
94-
if ($tokens[$prev]['content'][0] === $tokens[$next]['content'][0]) {
95-
// Before we throw an error for PHP, allow strings to be
96-
// combined if they would have < and ? next to each other because
97-
// this trick is sometimes required in PHP strings.
98-
if ($phpcsFile->tokenizerType === 'PHP') {
99-
$prevChar = substr($tokens[$prev]['content'], -2, 1);
100-
$nextChar = $tokens[$next]['content'][1];
101-
$combined = $prevChar.$nextChar;
102-
if ($combined === '?'.'>' || $combined === '<'.'?') {
103-
return;
104-
}
105-
}
106-
107-
if ($this->allowMultiline === true
108-
&& $tokens[$prev]['line'] !== $tokens[$next]['line']
109-
) {
110-
return;
111-
}
112-
113-
$error = 'String concat is not required here; use a single string instead';
114-
if ($this->error === true) {
115-
$phpcsFile->addError($error, $stackPtr, 'Found');
116-
} else {
117-
$phpcsFile->addWarning($error, $stackPtr, 'Found');
118-
}
119-
}//end if
120-
}//end if
116+
return;
117+
}
118+
119+
$error = 'String concat is not required here; use a single string instead';
120+
if ($this->error === true) {
121+
$phpcsFile->addError($error, $stackPtr, 'Found');
122+
} else {
123+
$phpcsFile->addWarning($error, $stackPtr, 'Found');
124+
}
121125

122126
}//end process()
123127

‎src/Standards/Generic/Tests/Strings/UnnecessaryStringConcatUnitTest.1.inc

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
<?php
2+
$x = 'My '.'string';
3+
$x = 'My '. 1234;
4+
$x = 'My '.$y.' test';
5+
6+
echo $data['my'.'index'];
7+
echo $data['my'. 4];
8+
echo $data['my'.$x];
9+
echo $data[$x.$y.'My'.'String'];
10+
11+
$code = '$actions = array();'."\n";
12+
$code = "$actions = array();"."\n";
13+
14+
// No errors for these because they are needed in some cases.
15+
$code = ' ?'.'>';
16+
$code = '<'.'?php ';
17+
18+
$string = 'This is a really long string. '
19+
. 'It is being used for errors. '
20+
. 'The message is not translated.';
21+
22+
$shouldBail = 1 + 1;
23+
24+
$shouldNotTrigger = 'My' . /* comment */ 'string';
25+
$shouldNotTrigger = 'My' /* comment */ . 'string';
26+
27+
// phpcs:set Generic.Strings.UnnecessaryStringConcat allowMultiline true
28+
$string = 'Multiline strings are allowed '
29+
. 'when setting is enabled.';
30+
// phpcs:set Generic.Strings.UnnecessaryStringConcat allowMultiline false
31+
32+
// phpcs:set Generic.Strings.UnnecessaryStringConcat error false
33+
$throwWarning = 'My' . 'string';
34+
// phpcs:set Generic.Strings.UnnecessaryStringConcat error true

‎src/Standards/Generic/Tests/Strings/UnnecessaryStringConcatUnitTest.2.inc

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
<?php
2+
3+
// Intentional parse error (only empty tokens after T_STRING_CONCAT).
4+
// This should be the only test in this file.
5+
// Testing that the sniff is *not* triggered.
6+
7+
$parseError = 'String' .

‎src/Standards/Generic/Tests/Strings/UnnecessaryStringConcatUnitTest.inc

Lines changed: 0 additions & 21 deletions
This file was deleted.

‎src/Standards/Generic/Tests/Strings/UnnecessaryStringConcatUnitTest.php

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ final class UnnecessaryStringConcatUnitTest extends AbstractSniffUnitTest
3333
public function getErrorList($testFile='')
3434
{
3535
switch ($testFile) {
36-
case 'UnnecessaryStringConcatUnitTest.inc':
36+
case 'UnnecessaryStringConcatUnitTest.1.inc':
3737
return [
3838
2 => 1,
3939
6 => 1,
@@ -65,11 +65,21 @@ public function getErrorList($testFile='')
6565
* The key of the array should represent the line number and the value
6666
* should represent the number of warnings that should occur on that line.
6767
*
68+
* @param string $testFile The name of the file being tested.
69+
*
6870
* @return array<int, int>
6971
*/
70-
public function getWarningList()
72+
public function getWarningList($testFile='')
7173
{
72-
return [];
74+
switch ($testFile) {
75+
case 'UnnecessaryStringConcatUnitTest.1.inc':
76+
return [
77+
33 => 1,
78+
];
79+
80+
default:
81+
return [];
82+
}
7383

7484
}//end getWarningList()
7585

0 commit comments

Comments
 (0)