Phpinspectionsea: "null !== $var construction should be used" gets triggered for isset($var)

[hansott]

class SomeClass {
  public function someMethod() {
    // ...
    if ($isPost) {
      // ...
      if (empty($errors)) {
        if ($type === 'text') {
          $var = ...;
        } elseif ($type === 'buttons') {
          $var = ...;
        }
        if (isset($var)) {
          //^^^^^^^^^^^ null !== $var construction should be used instead
        }
      }
    }
  }
}

If I change it too $var !== null:

class SomeClass {
  public function someMethod() {
    // ...
    if ($isPost) {
      // ...
      if (empty($errors)) {
        if ($type === 'text') {
          $var = ...;
        } elseif ($type === 'buttons') {
          $var = ...;
        }
        if ($var !== null) {
          //^^^^^^^^^^^^^ Variable 'var' might have not been defined before
        }
      }
    }
  }
}

If I add both checks:

class SomeClass {
  public function someMethod() {
    // ...
    if ($isPost) {
      // ...
      if (empty($errors)) {
        if ($type === 'text') {
          $var = ...;
        } elseif ($type === 'buttons') {
          $var = ...;
        }
        if (isset($var) && $var !== null) {
          //^^^^^^^^^^^ null !== $var construction should be used instead
          //               ^^^^^^^^^^^^^ It seems like '$var !== null' is already covered by 'isset(...)'
        }
      }
    }
  }
}

I believe this is generated by https://github.com/kalessil/phpinspectionsea/blob/master/src/main/java/com/kalessil/phpStorm/phpInspectionsEA/inspectors/apiUsage/IsNullFunctionUsageInspector.java

Is this the intended behaviour?

And, like always, thanks for this awesome plugin and your hard work. 😉

[kalessil]

Yes, it is =) In this case the root cause is this block: variable needs default value (e.g. null for a perfect fit)

        if ($type === 'text') {
          $var = ...;
        } elseif ($type === 'buttons') {
          $var = ...;
        }

I suggest to refactor it:

        switch ($type) {
            case 'text':
                $var = ...;
                break;
            case 'buttons':
                $var = ...;
                break;
             default:
                $var = null;
                break;
        }

[rentalhost]

@kalessil In this case, I should agree with issuer, once that $var could not be defined in that point and isset() make sense there (althrough is better you generate a default state (eg. $var = null), but is not a real limitation).

To simplify:

function example()
{
    if (epxr()) {
        $var = true;
    }

    return isset($var);
}

Here, if expr() is false, so $var will never be set, so we really should use isset($var), which is a valid fix. As alternative, we can do:

function example()
{
    $var = null; 

    if (epxr()) {
        $var = true;
    }

    return $var !== null;
}

Or even (for this simplification):

function example()
{
    $var = epxr() ? true : null;

    return $var !== null;
}

But in both cases, you are declaring $var as null, which could change the code behaviour - if you real intention is just check if it was declared in some moment.

[kalessil]

The intent is clear, therefore I confirmed that the reported warnings are also intentional.
So we are forcing to not use such constructions, in reliable software all variables states are determined.

[rentalhost]

I think that PHP and JS are the unique languages that allows things like that (check if variable was not defined). And I agree that it is not the best solution. I am divided between "what is good" (declared it as null and avoid isset()) and "what is allowed" (allow isset() usage). Well, I always use "what is good", so...

[kalessil]

Apart from you great points =), we also can not identify those optionally defined variables from IDE API - so we are forced to go for good as well xD