Direct variable-as-array assignment marked as undefined

[sirbrillig]

Direct variable-as-array assignment marked as undefined

[sirbrillig]

That's a good idea!

Making a separate error code also gives folks a more fine-grained control over the types of warnings being displayed. It will probably require a major version bump though since it may break some existing configs that assume all undefined variable warnings have the same sniff code.

[sirbrillig]

Just for my own reference, the specific tasks here are:

1. When marking a variable use as an undefined read, if the variable is followed by []=, use a different sniff code.
2. When marking a variable use as an undefined read, if the variable is followed by []=, consider that variable defined at that point going forward. (This will prevent future uses of the same variable from being reported as undefined.)

I think that will meet the requirements of this feature request.

[sirbrillig]

whether this is a best practice or not is up for debate, but I don't think this specific warning should be thrown

🤔 Thank you for bringing this up! The line between 'best practice' and 'bug' is sometimes subtle when talking about using undefined variables since PHP is quite happy to create variables when they're being used. This is why the notices produced by this sniff are warnings and not errors. It's hard to be sure what the intent is if we are not being explicit.

So here we are considering a statement like $b[] = 'value'; and we need to ask "is this a definition or a use?". $b[] = 'value' is another way of performing an operation on an array. The operation can also be written as array_push($b, 'value'), which would throw a PHP warning, but the former version is a shortcut which both defines and uses the variable at once. Looked at that way, the shortcut should definitely be ignored by this sniff as you're suggesting.

On the other hand, consider this code:

$usersForSubmission = ['abc', 'bbc'];
validateUsers($usersForSubmission);
$usrsForSubmission[] = 'ccb';
submitUsers($usrsForSubmission);

The third statement is perfectly legal PHP but has a typo which could create a very hard-to-find bug. If we allow the shortcut to be ignored by the sniff we might miss that bug. The intention of this sniff is to help prevent those bugs by making sure variables are defined and used explicitly. The extra step of forcing the developer to either unambiguously define a variable as an array or unambiguously ignoring the sniff on that line is needed in order to clarify the intent.

I definitely see where you're coming from, though. If you disagree with the above, can you think of a way we could disambiguate enough to be able to report a warning about the above example?

[jonathan1055]

Yes, when I said "fixed" I meant that we can address the issue reported and discused above. I didn't mean to imply that it was broken. Are you thinking that the new separate code for shorthand definition-and-assignment can be created and implemented first, getting this in for release 3.0, and then later do the work on the logic for what to report as a warning. Or can it all be done in one change?

I've not looked at any of this repository before today, but happy to help with testing / review / feedback when you get anything to show.

[sybrew]

I completely agree with your reasoning!

But, I still want to suggest a way to disambiguate the warning:

1. When $var[] = ''; is used, and only when the $var isn't assigned yet, spawn a new type of warning.
2. When $var or $var[] is called already, do not warn on later $var calls.

To illustrate:

$var[] = 'something'; // warning:
// Variable $var was undefined, did you mean to create it? Use `$var = [ 'value' ]`, instead.
// (VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedArrayVariable)

// (note the use of UndefinedArrayVariable)

$var[] = 'another entry'; // no warning.

Then, when the developer agrees with the assessment, they can fix it, or optionally add a comment:

// phpcs:ignore, VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedArrayVariable -- var creation OK.

What do you think?

[jonathan1055]

Just wondering if there is any progress on this? The reasoning and proposed solution above make perfect sense. This library has now become the one of choice for Drupal Coder module replacing our own in-house one, but we cannot use it until this problem is fixed. Happy to help here with testing if I can.

[sirbrillig]

@jonathan1055 Thanks for the interest! If by "fixed", you mean to create separate error codes for shorthand definition-and-assignment, I have not looked at this yet. However, if we could get a PR in before 3.0 is released, it could be included there, which would be good since it's a breaking change. I'll take a look to see how complex this would be.

[jonathan1055]

This looks good. Thank you very much.

[lkraav]

Should this issue still be pinned in the repo?

[sirbrillig]

Good point. It's been resolved in 3.0. Unpinned!