Phpinspectionsea: Report null coalesce operator usage on undefined variable

[BenMorel]

The Null Coalesce Operator, aka ??, is a nice way to replace this kind of code:

echo ($comment === null) ? 'none' : $comment;

with:

echo $comment ?? 'none';

But IMHO, it also has a not-so-nice side effect: it allows for undefined variables to pass without a notice. This is great when used with arrays, such as $config['host'] ?? 'example.com', but when used with plain variables, I would personally consider it a defect in my code if the variable I'm testing is not defined, even though it's valid PHP.

Therefore, it would be nice to have an optional inspection for this.

To summarize, this would be considered valid code:

$comment = (whatever);
echo $comment ?? 'none';

This would be valid as well:

$config = [];
echo $config['host'] ?? 'example.com'; // even though the 'host' key is not defined

But this would trigger an inspection report:

echo $comment ?? 'none'; // when $comment has not been defined

What do you think?

[rentalhost]

The _null coalesce operator_ is something like isset($variable) && $variable !== null ? $variable : $else. Then it should check first if isset($variable) and it is a valid PHP code. Create an inspection about "variable does not exists" maybe is not cool because you really could expect that it could or not exists, mainly if do you uses templates engine.

Code example:

return view('home', [ 'title' => 'Hello you!' ]); // Without message, still valid.
return view('home', [ 'title' => 'Hello you!', 'message' => 'John, you are fine?' ]);

View:

{{ $title }}
{{ $message ?? 'You are fine?' }}

[BenMorel]

Thanks for sharing your opinion on this. I know it is valid PHP code, and as you rightly pointed out, it may be useful in some cases. In my coding habits however, the "variable does not exist" is a really strong indication of a possible bug.

That's why I suggested this inspection to be optional!

[rentalhost]

Yeah, maybe it could be applied on cases where it definitevely could not exists (in function, methods), but be disabled on "global scope" (like views/templates).

[BenMorel]

That could be an idea as well. In my case, a project-wise configuration is OK, but I can understand other people might want it to be applied only to local scope!

[kalessil]

PhpStorm has already an inspection called "Undefined variable", which would spot the case in echo $comment ?: 'none';. And perhaps it worth making a feature request (additional option in the inspection) in
in PhpStorm backlog for doing the same check in case of ??.

[BenMorel]

From my past experience with JetBrains, they're quite slow to react to such feature requests, so I was hoping this plugin would implement it faster.

Is there any rule for what kind of inspection belongs to PHPStorm, and what kind belongs to this plugin?

[kalessil]

The only rule is the Pragmatism with 3 main statements:

• Fixing bugs first (=> frequent plugin updates);
• Not duplicating PhpStorm functionality on OSS terms (strict types, runtime errors prevention and etc);
• Custom development on commercial terms (business as usual);

[rentalhost]

I guess that this features is different of implemented ?: because the ?? simulates a isset(), then variable could really don't be defined on views (_optionally defined_, in case), but need to be defined on functions and methods.

[kalessil]

Yes, that's clear ;)

My point is that basic logic for finding unused variables is already there, so making a FR to JB makes more sense.

[rentalhost]

Should be better, really. But JB focus is a bit confuse. Then maybe you can do it first while they don't fixes that. I can open a ticket there about this topic later.

[BenMorel]

I've opened around 100 issues with JB over the years, so I have somewhat of an experience with how they respond to feature requests. We can open it, but it may stay open for years before someone picks it up.

[funivan]

If developers use ?? or isset then they thinks in the following way: Variable can be undefined/uninitialized So how to check if developer just make typo and use undefined variable or he already know about this situation?

PhpStorm default inspection Undefined variable works fine and ignore isset (andd ??). There is no way to detect correct usage of isset with defined/undefined variables.

If you want to check if variables are always initialized then you definitely should use
echo ($comment === null) ? 'none' : $comment;
Yes, this is not so nice way. But this is right way.
Null Coalesce Operator is not you friend in your situation.

| it allows for undefined variables to pass without a notice

Yes. And this is correct. This is what ?? should do.

[BenMorel]

@funivan I don't quite agree with you on this one. If I understand you well, your reasoning is the following:

there's an alternative syntax that works for the OP:
echo ($comment === null) ? 'none' : $comment;
so he can just use this and not use ?? at all. We don't need an extra inspection.

If they had followed the same reasoning, the voters of the RFC could have said:

there's already an alternative syntax to the proposed ?? operator:
echo isset($comment) ? $comment : 'none';
so developers can use this. We don't need an extra operator.

But they did vote for it (at least 91% of them), and implement it. They did it because it's a nice shortcut, that makes code more readable and understandable.

And I agree with this. It's useful. It's concise. It's good that it doesn't complain on undefined variables, for example when used on array keys. And it may even be good for some developers when used on global variables, @rentalhost gave a good example of this when used in views.

My point is, I am only ever using this operator in closed scopes, when an undefined variable is a sign of something seriously wrong. But I don't want to avoid using this otherwise nice and concise operator just for this reason.

That's why an optional inspection would be good for me, and hopefully, for others.

The inspection could even be configured in 3 different ways:

• disabled (default)
• enabled in closed scope
• enabled everywhere

What do you think?

[funivan]

I thinks this inspection should be designed by JB team.
One of the main argument: there is standard inspection undefined variable. And if we want to create similar one we should copy all the logic but without spiking isset and ?? operator constructions.

Anyway i`m against this feature.

[kalessil]

Final resolution: this needs to be addressed to JB.

[BenMorel]

Reported as WI-35211. Thanks for your time.

[kalessil]

Picking up instead of the closed #225

[kalessil]

Implemented (47caa076e57b0a578517de1f669e93ac3f11a79a, fresh binary pushed as well) as a probable bugs inspection.

EDIT: only in method/function context

[kalessil]

@BenMorel : can you give a try to the plugin version from master?

[funivan]

@kalessil I will try too :)

[kalessil]

@funivan : great =)

[BenMorel]

Works like a charm, well done and thank you! 👍

[funivan]

@kalessil test it with production code. Inspection works fine.

[kalessil]

@funivan super, now we really well equipped with NCO related inspections.