Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positive for undefined variable in wp_parse_str and wp_parse_args #67

Closed
david-binda opened this issue Feb 7, 2019 · 4 comments
Closed

Comments

@david-binda
Copy link

david-binda commented Feb 7, 2019

An example from WordPress phpunit tests producing false-positive reports of undefined variables for both functional lines:

// $q is defined above.
wp_parse_str( $q, $ss );
$this->assertEquals( $ss, wp_parse_args( $q ) );

While in the first case, the $ss is not being defined at the point of wp_parse_str call, it's being created and populated inside the function, in the second case, the $ss has already been populated.

Should be easy to fix the lint issues by defining $ss as an empty array prior this code, but the notation in the example feels like a commonly used idiom in PHP (not only for wp_parse_args, but also for parse_str and other functions with similar variable creation capability).

@sirbrillig
Copy link
Owner

🤔 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
Copy link
Author

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
Copy link
Owner

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
Copy link
Owner

#71 documents the existing option.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants