Phpinspectionsea: [FR] Add inspections for error control operator (@) usages

[Radiergummi]

| Subject | Details |
| :------------- | :---------------------------------------------------------------------------- |
| Issue type | Feature request |
| Plugin | Php Inspections (EA Extended) |
| Language level | PHP 7.3 |

Current behaviour (description/screenshot:)

Conditions like the following trigger no inspections:

if (@$array['key']) {
  // ..
}

While looking innocuous, every time the condition is hit and the key is _not_ set, PHP will write a notice to the error log, complaining about an undefined index. This spams the log, especially for big applications.
Manually swapping these statements is cumbersome (at least for those of us having to refactor legacy code...).

Expected behaviour

An easy fix to the above is to use isset():

if (isset($array['key'])) {
  // ..
}

Hence I think it would be nice to have a quick fix available that transforms conditions like (@<array>[<key>]) or (@<array>[<key>] === <value>) into the appropriate (isset(<array>[<key>])) and (isset(<array>[<key>]) && <array>[<key>] === <value>), respectively.

It might also be beneficial to have additional inspections for bad usages of the error control operator. For most built-in PHP functions that might trigger errors or warnings, some common causes are known and can easily be mitigated (eg., check if a file is readable before attempting to do readfile()).

I'm not capable of coding this myself, but I'd be more than willing to help flesh out the specifics, if I can.

Environment details

PhpStorm 2018.3.3
Build #PS-183.5153.36, built on January 9, 2019
Licensed to * / *
Subscription is active until *
JRE: 1.8.0_152-release-1343-b26 amd64
JVM: OpenJDK 64-Bit Server VM by JetBrains s.r.o
Windows 10 10.0

[funivan]

PhpStorm has built-in inspection Usage of the silence operator but without QFs

One hint to speed up code grooming of the legacy project:
You can use Search structurally action in the IDE. Invoke this action and paste @$a$[$b$]. Also, you can perform replace of these statements with Replace structurally action.

[Radiergummi]

@funivan thank you, that's definitely helpful! What's your opinion on the quick fixes though?

[funivan]

@Radiergummi

1. Fix some code with Replace structurally action
2. Check how many statements (% of them) will be fixed if @kalessil add QF for this inspection?
   In our project, we have a lot of legacy code, but only about 2% (6 statements) can be fixed automatically with at QF. So for me this QF is not needed (IMHO)

[Radiergummi]

I'm in the process of fixing stuff, but the situation is different for my team... Our code is riddled with stuff like -

if (@$x['y'] || @$x['z']) {}

if (@$x['y'] == 42) {}

$z = @$x['y'];

Convincing some of the older devs to resort to isset() was hard on it's own, so if there was a QF to fix the existing code with _alt-enter, enter_, the barrier to actually do that would certainly be lower.
And with "riddled", I mean 2179 structural search matches in just one of several projects.

[andreasschroth]

Hmm, not sure if this should be implemented as the inspection is already available as a built-in one as @funivan already pointed out. Thus, the inspection would be duplicated somehow. Maybe suggest to the PhpStorm team on their Youtrack issue tracker that they implement a QF for their inspection?

OT: I wrote something on Stackoverflow about the @ operator in case they're still not convinced: https://stackoverflow.com/questions/1032161/what-is-the-use-of-the-symbol-in-php/36892995#36892995

[orklah]

Hi everybody,

I don't know if everybody already know this method, but in doubt:

When you have a Replace structurally rule, you can add it to the "Structural Search Inspection" rule in Inspections > General.

When you add a "Search structurally" rule, it flags the error like an inspection and when you add a "Replace structurally"rule, it behave just like a quick fix.

[Radiergummi]

@andreasschroth Yup it might be sensible to add it to their issue tracker, however I've got around 5 issues open since around half a year, so there's not too much hope they actually care about minor stuff like this in the forseeable future (Not to blame JB, there's bigger issues to action on).

@orklah I don't trust a search-replace rule to match all the creative ways some people use the error control operator 😉 However that's useful in other ways, so thank you!

[andreasschroth]

@Radiergummi Yep, I know, they're not very fast. I also still have unfixed issues open over there. But otherwise it would feel like a duplication somehow. Well @kalessil has to decide in the end, but for me it wouldn't add additional benefit to have the same inspection again inside the plugin just with a QF.

[orklah]

@Radiergummi , You're welcome :)

Keep in mind that it's not an automatic process, it's just like an inspection, it relies on someone in your your team typing 'alt+enter' , previewing the replace and then validate.

I add custom inspection like that for a lot of cases so my team can do the fix themselves

[kalessil]

I'm in general not against reinventing the wheel, but I'm pretty sure we should create a FR for PhpStorm team to add the QF.

SSR might be a temporal solution, did it work for you @Radiergummi ?

[Radiergummi]

@kalessil not quite - I tried, but I could not come up with an expression that'd match if conditions... It always replaced the condition body, too. But I'm pretty sure that's just a matter of reading the documentation more thoroughly and homework for me personally.
Would you mind creating the FR? I'd assume your voice has a little more weight than mine for JetBrains.

[orklah]

@Radiergummi
Try something like that:

Search pattern:
if(@$a$[$b$]){$c$}

Replace pattern:
if(isset($a$[$b$])){$c$}

You may need to create more than one SSR to match every depth of your arrays
For example if(@$a$[$b$][$d$]){$c$}, if(@$a$[$b$][$d$][$e$]){$c$} etc...

One case for elseif may be needed too (ternary also?).

[kalessil]

I took time to think if this FR would fit the project scope, and decided to close the issue. Sorry.
Since PhpStorm has built-in inspection, it can spot places for cleanup.