Skip to content

Commit 2ec00a3

Browse files
rebeccahumrebeccahum
committed
Account for concatenation in option names
1 parent 3f1dbf8 commit 2ec00a3

File tree

3 files changed

+191
-24
lines changed

3 files changed

+191
-24
lines changed

‎WordPressVIPMinimum/Sniffs/Functions/OptionsRaceConditionSniff.php‎

Lines changed: 142 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco
5454
$tokens = $phpcsFile->getTokens();
5555

5656
if ( $tokens[ $stackPtr ]['content'] !== $this->delete_option ) {
57+
// delete_option is not first function found.
5758
$stackPtr = $phpcsFile->findNext(
5859
[ T_STRING ],
5960
$stackPtr + 1,
@@ -78,23 +79,58 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco
7879
return; // Not a function call, bail.
7980
}
8081

81-
$delete_option_semicolon = $phpcsFile->findNext(
82-
[ T_SEMICOLON ],
83-
$tokens[ $delete_option_scope_start ]['parenthesis_closer'] + 1,
82+
$delete_option_name = $phpcsFile->findNext(
83+
Tokens::$emptyTokens,
84+
$delete_option_scope_start + 1,
8485
null,
85-
false
86+
true
8687
);
8788

88-
$delete_option_key = $phpcsFile->findNext(
89+
$delete_option_concat = $phpcsFile->findNext(
8990
Tokens::$emptyTokens,
90-
$delete_option_scope_start + 1,
91+
$delete_option_name + 1,
9192
null,
9293
true
9394
);
9495

96+
$delete_option_name = $this->trim_strip_quotes( $tokens[ $delete_option_name ]['content'] );
97+
98+
$is_delete_option_concat = $tokens[ $delete_option_concat ]['code'] === T_STRING_CONCAT;
99+
if ( $is_delete_option_concat ) {
100+
// If option name is concatenated, we need to build it out.
101+
$delete_option_concat = $phpcsFile->findNext(
102+
Tokens::$emptyTokens,
103+
$delete_option_concat + 1,
104+
null,
105+
true
106+
);
107+
108+
while ( $delete_option_concat < $tokens[ $delete_option_scope_start ]['parenthesis_closer'] ) {
109+
$delete_option_name .= $this->trim_strip_quotes( $tokens[ $delete_option_concat ]['content'] );
110+
111+
$delete_option_concat = $phpcsFile->findNext(
112+
array_merge( Tokens::$emptyTokens, [ T_STRING_CONCAT ] ),
113+
$delete_option_concat + 1,
114+
null,
115+
true
116+
);
117+
}
118+
}
119+
120+
$delete_option_scope_end = $phpcsFile->findNext(
121+
[ T_SEMICOLON ],
122+
$tokens[ $delete_option_scope_start ]['parenthesis_closer'] + 1,
123+
null,
124+
false
125+
);
126+
127+
if ( ! $delete_option_scope_end ) {
128+
return; // Something went wrong with the syntax.
129+
}
130+
95131
$add_option = $phpcsFile->findNext(
96132
Tokens::$emptyTokens,
97-
$delete_option_semicolon + 1,
133+
$delete_option_scope_end + 1,
98134
null,
99135
true
100136
);
@@ -126,15 +162,55 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco
126162
return; // Not a function call, bail.
127163
}
128164

129-
$add_option_inside_if_option_key = $phpcsFile->findNext(
165+
$add_option_inside_if_option_name = $phpcsFile->findNext(
130166
Tokens::$emptyTokens,
131167
$add_option_inside_if_scope_start + 1,
132168
null,
133169
true
134170
);
135171

136-
if ( $add_option_inside_if_option_key && $this->is_same_option_key( $tokens, $add_option_inside_if_option_key, $delete_option_key ) ) {
137-
$phpcsFile->addWarning( $message, $add_option_inside_if_option_key, 'OptionsRaceCondition' );
172+
$add_option_inside_if_concat = $phpcsFile->findNext(
173+
Tokens::$emptyTokens,
174+
$add_option_inside_if_option_name + 1,
175+
null,
176+
true
177+
);
178+
179+
$add_option_inside_if_option_name = $this->trim_strip_quotes( $tokens[ $add_option_inside_if_option_name ]['content'] );
180+
181+
if ( $is_delete_option_concat && $tokens[ $add_option_inside_if_concat ]['code'] === T_STRING_CONCAT ) {
182+
$add_option_inside_if_concat = $phpcsFile->findNext(
183+
array_merge( Tokens::$emptyTokens, [ T_STRING_CONCAT ] ),
184+
$add_option_inside_if_concat + 1,
185+
null,
186+
true
187+
);
188+
189+
$add_option_inside_if_scope_end = $phpcsFile->findNext(
190+
[ T_COMMA ],
191+
$add_option_inside_if_concat + 1,
192+
null,
193+
false
194+
);
195+
196+
if ( ! $add_option_inside_if_scope_end ) {
197+
return; // Something went wrong.
198+
}
199+
200+
while ( $add_option_inside_if_concat < $add_option_inside_if_scope_end ) {
201+
$add_option_inside_if_option_name .= $this->trim_strip_quotes( $tokens[ $add_option_inside_if_concat ]['content'] );
202+
203+
$add_option_inside_if_concat = $phpcsFile->findNext(
204+
array_merge( Tokens::$emptyTokens, [ T_STRING_CONCAT ] ),
205+
$add_option_inside_if_concat + 1,
206+
null,
207+
true
208+
);
209+
}
210+
}
211+
212+
if ( $this->is_same_option_key( $delete_option_name, $add_option_inside_if_option_name ) ) {
213+
$phpcsFile->addWarning( $message, $add_option_inside_if_scope_start, 'OptionsRaceCondition' );
138214
}
139215

140216
// Walk ahead out of IF control structure.
@@ -180,16 +256,54 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco
180256
return; // Not a function call, bail.
181257
}
182258

183-
// Check if it's the same option being deleted earlier.
184-
$add_option_key = $phpcsFile->findNext(
259+
$add_option_name = $phpcsFile->findNext(
185260
Tokens::$emptyTokens,
186261
$add_option_scope_start + 1,
187262
null,
188263
true
189264
);
190265

191-
if ( $this->is_same_option_key( $tokens, $add_option_key, $delete_option_key ) ) {
192-
$phpcsFile->addWarning( $message, $add_option_key, 'OptionsRaceCondition' );
266+
$add_option_concat = $phpcsFile->findNext(
267+
Tokens::$emptyTokens,
268+
$add_option_name + 1,
269+
null,
270+
true
271+
);
272+
273+
$add_option_name = $this->trim_strip_quotes( $tokens[ $add_option_name ]['content'] );
274+
if ( $is_delete_option_concat && $tokens[ $add_option_concat ]['code'] === T_STRING_CONCAT ) {
275+
$add_option_concat = $phpcsFile->findNext(
276+
Tokens::$emptyTokens,
277+
$add_option_concat + 1,
278+
null,
279+
true
280+
);
281+
282+
$add_option_scope_end = $phpcsFile->findNext(
283+
[ T_COMMA ],
284+
$add_option_concat + 1,
285+
null,
286+
false
287+
);
288+
289+
if ( ! $add_option_scope_end ) {
290+
return; // Something went wrong.
291+
}
292+
293+
while ( $add_option_concat < $add_option_scope_end ) {
294+
$add_option_name .= $this->trim_strip_quotes( $tokens[ $add_option_concat ]['content'] );
295+
296+
$add_option_concat = $phpcsFile->findNext(
297+
array_merge( Tokens::$emptyTokens, [ T_STRING_CONCAT ] ),
298+
$add_option_concat + 1,
299+
null,
300+
true
301+
);
302+
}
303+
}
304+
305+
if ( $this->is_same_option_key( $delete_option_name, $add_option_name ) ) {
306+
$phpcsFile->addWarning( $message, $add_option_scope_start, 'OptionsRaceCondition' );
193307
return;
194308
}
195309
}
@@ -209,14 +323,22 @@ protected function processTokenOutsideScope( File $phpcsFile, $stackPtr ) {
209323
/**
210324
* Check if option is the same.
211325
*
212-
* @param array $tokens List of PHPCS tokens.
213-
* @param int $first_option Stack position of first option.
214-
* @param int $second_option Stack position of second option to match against.
326+
* @param string $option_name Option name.
327+
* @param string $option_name_to_compare Option name to compare against.
215328
*
216329
* @return false
217330
*/
218-
private function is_same_option_key( $tokens, $first_option, $second_option ) {
219-
return $tokens[ $first_option ]['code'] === $tokens[ $second_option ]['code'] &&
220-
$tokens[ $first_option ]['content'] === $tokens[ $second_option ]['content'];
331+
private function is_same_option_key( $option_name, $option_name_to_compare ) {
332+
return $option_name === $option_name_to_compare;
333+
}
334+
335+
/**
336+
* Trim whitespace and strip quotes surrounding an arbitrary string.
337+
*
338+
* @param string $string The raw string.
339+
* @return string String without whitespace or quotes around it.
340+
*/
341+
public function trim_strip_quotes( $string ) {
342+
return trim( preg_replace( '`^([\'"])(.*)\1$`Ds', '$2', $string ) );
221343
}
222344
}

‎WordPressVIPMinimum/Tests/Functions/OptionsRaceConditionUnitTest.inc‎

Lines changed: 43 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,9 @@ function test_same_option_key() {
2727
add_option( $key, 'data' ); // Warning.
2828
}
2929

30-
function test_same_option_key_var_assignment2() {
31-
$delete = delete_option( 'option_key' );
32-
add_option( 'option_key', [ 'stuff', '123' ] ); // Warning.
30+
function test_same_option_key_double_quoted_string() {
31+
$delete = delete_option( "option_{$id}" );
32+
add_option( "option_{$id}", [ 'stuff', '123' ] ); // Warning.
3333
}
3434

3535
function test_same_option_key_string() {
@@ -103,10 +103,49 @@ function concurrent_option_writes_with_one_if_different_key() {
103103
$test = add_option( 'test_option', [ 1, 2, 3 ] ); // Warning.
104104
}
105105

106-
function concurrent_option_writes_with_one_if_same_key() {
106+
function test_concurrent_option_writes_with_if() {
107107
$test = delete_option( 'test_option' );
108108
if ( $test ) {
109109
add_option( 'test_option', [] ); // Warning.
110110
}
111111
$test = add_option( 'test_option', [ 1, 2, 3 ] ); // Warning.
112112
}
113+
114+
function test_different_quotes() {
115+
delete_option( 'foobar' );
116+
add_option( "foobar", 'baz' ); // Warning.
117+
}
118+
119+
function test_spacing_inside_option_name() {
120+
delete_option( 'foobar' );
121+
add_option( ' foobar ', 'baz' ); // Warning.
122+
}
123+
124+
function test_concat_same_string_var() {
125+
delete_option( 'foo_' . $bar );
126+
add_option( 'foo_' . $bar, 'baz' ); // Warning.
127+
}
128+
129+
function test_concat_different_string_var() {
130+
delete_option( 'foo_' . $ba );
131+
add_option( 'foo_' . $bar, 'baz' ); // OK - Not same option name.
132+
}
133+
134+
function test_inside_if_concat_option_name( $test ) {
135+
$test = delete_option( "foo_" . $bar );
136+
if ( $test ) {
137+
add_option( 'foo_' . $bar, [ 1, 2, 3 ] ); // Warning.
138+
}
139+
}
140+
141+
function test_long_concat() {
142+
delete_option( 'foo_' . $bar . '_' . $id );
143+
add_option( "foo_" . $bar . '_' . $id, 'baz' ); // Warning.
144+
}
145+
146+
function test_long_concat_inside_if() {
147+
$test = delete_option( 'foo_' . $bar . '_' . $id );
148+
if ( $test ) {
149+
$option_added = add_option( "foo_" . $bar . '_' . $id, 'baz' ); // Warning.
150+
}
151+
}

‎WordPressVIPMinimum/Tests/Functions/OptionsRaceConditionUnitTest.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,12 @@ public function getWarningList() {
4747
103 => 1,
4848
109 => 1,
4949
111 => 1,
50+
116 => 1,
51+
121 => 1,
52+
126 => 1,
53+
137 => 1,
54+
143 => 1,
55+
149 => 1,
5056
];
5157
}
5258

0 commit comments

Comments
 (0)