False positive for undefined variable in wp_parse_str and wp_parse_args

[sirbrillig]

False positive for undefined variable in wp_parse_str and wp_parse_args

[sirbrillig]

🤔 That's an interesting situation. There's a few aspects of the code you have above and I'm not 100% certain I understand which one or ones you are pointing to, so I'll do my best to answer, but I apologize in advance if I'm misunderstanding! Please correct me if so!

Even if some functions, like parse_str() (with two arguments), assign values to a variable by reference, the variable must still exist before it's passed to the function. PHP will dynamically create the variable on the fly if it does not exist, but that's also true in the statement echo $ss;, and is precisely the sort of thing that this sniff is designed to catch.

In the example above, the statement wp_parse_str( $q, $ss ) is passing an undefined variable, $ss, as an argument to a function to be assigned a value. The function isn't creating the variable, it's just assigning it by reference. In order to be referenced, it must already exist, so I don't see that as a false positive.

I think that's what you were referring to, but just in case:

There's another behavior of parse_str() which, when called with only one argument, creates variables in the current scope. This is similar to extract(), but I'd argue that either of those uses are very risky behavior since it's quite hard to track variables when you can't see where they're being created. I think that in cases where those patterns are used, it's best to just disable this sniff.

[david-binda]

I think that's what you were referring to, but just in case:

Yeah, correct. Also, thanks for wording the issue clearly.

PHP will dynamically create the variable on the fly if it does not exist, but that's also true in the statement echo $ss;

I personally see a difference in between the two examples. While in the echo $ss example, PHP also dynamically creates the variable, the statement does not work as expected, and is likely a bug in the code.

In the original example, tho, the parse_str( 'foo=bar', $ss ); is, imho, a idiomatic usage of such a function in PHP, and the initial dynamic creation of $ss with NULL value is expected. On the end of the execution of the statement, the type of $ss will be an array. Such an usage is also mentioned in the PHP documentation.

and is precisely the sort of thing that this sniff is designed to catch.
...
think that in cases where those patterns are used, it's best to just disable this sniff.

I can totally see your point and am happy to disable the sniff for such patterns. Disabling the sniff, is from my point of view, a better solution than alternative, rather verbose, notation:

$ss = [];
parse_str( 'foo=bar', $ss );

[sirbrillig]

Such an usage is also mentioned in the PHP documentation.

// Recommended
parse_str($str, $output);
echo $output['first'];  // value

Thanks for linking to that, and for the explanation. I'm starting to think this is sort of like using == for comparison: while often risky, in specific cases it's an idiomatic use of the language which is quite safe and clear.

if ( $_POST['amount'] == 5 ) {
  // ...
}

So while the WordPress coding standard warns us about the above code, if the developer knows what they're doing, it is probably more clear than (int) $_POST['amount'] === 5. The coding standard still warns about it, though, because it's hard to know programmatically if the usage is intentional or unintentional.

In the case of this sniff, parse_str( 'foo=bar', $ss ) seems to be an idiomatic exception to the need to define variables before using them. So if we want to allow that behavior, we'll need some way to determine that the pattern is intentional.

The first thing that comes to mind is a whitelist of functions which can be passed undefined variables without a warning. Apparently this sniff already includes such a feature, as part of the original code that I forked! I would just need to add an option to also include the WordPress functions which follow that pattern. I'll see what I can do.

[sirbrillig]

#71 documents the existing option.

#69 adds a new option to accept WordPress versions, but I need to add to that list.