Phpinspectionsea: Use short ternary instead null coalesace operator

[funivan]

  class ShowPeriod {
  }
  class Film {
    private $showPeriod;
    public function __construct(ShowPeriod $showPeriod = null) {
      $this->showPeriod = $showPeriod !==null ? $showPeriod: new ShowPeriod(); // target line
    }
  }
  $film = new Film();
  var_dump($film);

We get suggestion in the target line. With QF get next code:

    $this->showPeriod = $showPeriod ?? new ShowPeriod();

All is fine BUT we can use short ternary:

    $this->showPeriod = $showPeriod ?: new ShowPeriod();

If our variable can be null|Object and variable exists in current scope than short ternary can be used.
Do not know if this is real enhancement or but.

[Fleshgrinder]

What difference does it make? My guess would actually be that the null coalesce operator is the better choice in such situations, because it does not try to coerce the variables content to a Boolean, but rather checks for null only.

[funivan]

@Fleshgrinder you are correct in this case. But if we use short ternary we can get error on undefined variable. But with constructions like isset, empty, ?? we should never get error on undefined variable.

P.S I saw @rentalhost report issue related to empty, isset and ?? oprator

[kalessil]

I see the point, but both ?? and ?: have side effects. I think it worth to drop this pattern at all.

[Fleshgrinder]

They can be exactly what you want, but at the same time they can be exactly the opposite too. We should rather find the situations in which it is clear that it can be reduced:

if (isset($this->collection[$key]) {
    return $this->collection[$key];
}

return \null;

This would be a clear situation for null coalesce:

return $this->collection[$key] ?? \null;

etc. pp.

[kalessil]

@Fleshgrinder : that's a new pattern, which relies on isset and therefore 100% compliant with null coalescing RFC.

What I mean is this, the last 2 strategies (null comparison and array key exist) are NOT part of the RFC.

To re-cap on the discussion before, the example before after applying hotfix will do the same. BUT if variable not defined anymore we'll never know about this.

This was escalated by @BenMorel in #148, and it seems that we have 2 options here:

• Go ahead with #148
• Stick to RFC, but we are going to loose additional patterns I referenced before.

@BenMorel, @rentalhost, @funivan, @Fleshgrinder: for me both ways are good, so please vote for the way to go.

[rentalhost]

Seems that the ?: for this case (Class|null) is better, because that if in sometime variable doesn't exists (for instance, after refactoring) it'll not be _always-else_ variable and should help to find it easily. The #148 will help with that, futurely, if PS implements that (I still guess that we should wait about 5 years haha).

[kalessil]

@rentalhost : ?: is not an option =) We either implementing #148 or sticking to RFC (null compare, array key exists contexts analysis will be dropped)

You have to choose this time.

[funivan]

Vote for 2. Stick to RFC, but we are going to loose additional patterns I referenced before.

[rentalhost]

@kalessil Ah, right. This is a bit confuse to me (_some little problems with english_), but I still thinks that #148 should be implemented by EA, because PS will implements it _who knows when_. And it should be skipped when PS do that finally.

[BenMorel]

I'm not sure to get all the ins and outs of this discussion, but I'm still all for #148; even though it's now in the roadmap for JetBrains, as @rentalhost said, it might take years before it's implemented (if it's ever implemented). I do think it's a separate issue though, some people seem to be happy that ?? does not care if the variable is set. I usually do care, so I'm for an optional inspection specific to the null coalesce operator on an undefined variable.

[kalessil]

Closing in favor of #148