Fix PHPStan L2 error "Variable $foo in PHPDoc tag @var does not exist. "

It looks like we're getting to the point where PHPStan will start changing a bit our habits...

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -813,12 +813,12 @@ protected function doDelete($entities) {
   -    /** @var \Drupal\Core\Entity\ContentEntityInterface $revision */
        if ($revision = $this->loadRevision($revision_id)) {
          // Prevent deletion if this is the default revision.
          if ($revision->isDefaultRevision()) {
            throw new EntityStorageException('Default revision can not be deleted');
          }
   +      /** @var \Drupal\Core\Entity\ContentEntityInterface $revision */
   

   It's rather strange that the PHPDoc follows the variable initialization. Have you tried to init the variable outside the if?

        /** @var \Drupal\Core\Entity\ContentEntityInterface $revision */
        $revision = $this->loadRevision($revision_id);
        if ($revision) {
          // Prevent deletion if this is the default revision.
          if ($revision->isDefaultRevision()) {
            throw new EntityStorageException('Default revision can not be deleted');
          }
   

   or, if it's necessary to let PHPStan know about the type of a variable after it has been defined, you could use assert() like e.g.

        $revision = $this->loadRevision($revision_id);
        assert($revision instanceof ContentEntityInterface);
        if ($revision) {
          // Prevent deletion if this is the default revision.
          if ($revision->isDefaultRevision()) {
            throw new EntityStorageException('Default revision can not be deleted');
          }
   

   BTW, the if after the assert at that point becomes redundant.

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityUntranslatableFieldsConstraintValidator.php
   @@ -95,11 +95,12 @@ public function validate($entity, Constraint $constraint) {
   -    /** @var \Drupal\Core\Entity\ContentEntityInterface $original */
        if (isset($entity->original)) {
   +      /** @var \Drupal\Core\Entity\ContentEntityInterface $original */
          $original = $entity->original;
        }
        else {
   +      /** @var \Drupal\Core\Entity\ContentEntityInterface $original */
          $original = $this->entityTypeManager
   

   this feels wrong... in the second case $original definitely is not a content entity...

3. +++ b/core/lib/Drupal/Core/Form/ConfigFormBaseTrait.php
   @@ -37,7 +37,6 @@ trait ConfigFormBaseTrait {
   -    /** @var \Drupal\Core\Config\ConfigFactoryInterface $config_factory */
   

   here we probably should use an assert somewhere later... what happens if method_exists($this, 'configFactory') is false?

... and so on ...

We're not the first... https://github.com/phpstan/phpstan/issues/7934

As I said, IMHO we should reconsider our habits...

Wouldn't the entire method be better like

  /**
   * {@inheritdoc}
   */
  public function deleteRevision($revision_id) {
    /** @var \Drupal\Core\Entity\ContentEntityInterface $revision */
    $revision = $this->loadRevision($revision_id);

    // @todo throw an exception if there is no revision at this point.
    if (!$revision) {
      return;
    }

    // Prevent deletion if this is the default revision.
    if ($revision->isDefaultRevision()) {
      throw new EntityStorageException('Default revision can not be deleted');
    }

    $this->invokeFieldMethod('deleteRevision', $revision);
    $this->doDeleteRevisionFieldItems($revision);
    $this->invokeHook('revision_delete', $revision);
  }

for sure, this means this is no longer a low hanging fruit...

PHPStan really drives code to be more declarative.

I think #10 makes sense.

Updated steps to reproduce. Rerolled as an MR. Hiding patches.

While I agree it would be nice to use early returns and not assign variables inside if statements, I think it makes sense why phpstan behaves the way it does. If the condition is falsey then the variable will not match the type we're hinting. We can only hint it once we the condition evaluates to true.

Happy to work on refactoring the methods but I fear it would need to be split many more times to satisfy reviewers.

Not sure I follow, just looking at the first file core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php isn't it correct there as that's where $revision is set?

@smustgrave

Well loadRevision can return null, so in that case the @var doc would be wrong. But that is kind of moot because $revision is not used outside the if statement. Either way, phpstan doesn't like it the way it is.

It's worth also noting that loadRevision returns a RevisionableInterface, but invokeFieldMethod and doDeleteRevisionFieldItems expect a ContentEntityInterface, so we need to tell phpstan that we've got the latter.

To avoid the @var tag we could write it like this:

  /**
   * {@inheritdoc}
   */
  public function deleteRevision($revision_id) {
    $revision = $this->loadRevision($revision_id);

    // @todo throw an exception if there is no revision at this point.
    if (!$revision instanceof ContentEntityInterface) {
      return;
    }

    // Prevent deletion if this is the default revision.
    if ($revision->isDefaultRevision()) {
      throw new EntityStorageException('Default revision can not be deleted');
    }

    $this->invokeFieldMethod('deleteRevision', $revision);
    $this->doDeleteRevisionFieldItems($revision);
    $this->invokeHook('revision_delete', $revision);
  }

So not 100% sure how to review this one

@smustgrave did you try the steps in the IS? Try that against 11.x and again against this branch and you should see the diff of errors that are resolved. We could nitpick if the @var tags have moved to the right places, but if phpstan is happy now then I think that is satisfactory.

Also here are some updated steps to avoid touching the baseline file, I like to use --error-format=raw to get one error per line for easier grepping.

./vendor/bin/phpstan analyse -c core/phpstan.neon.dist --level=2 --error-format=raw | grep "in PHPDoc tag @var does not exist."

That currently gives me 27 errors on 11.x and 0 in this branch.

Looking good. I left a couple of suggestions.

Responded to one, hopefully that helps to explain why they are where they are. I don't intend to make further updates, so feel free to try move things around and run the commands. If things still pass feel free to push those changes.

I performed the manual check as described in the issue summary on my suggestions. The suggestions also passed linting. So I pushed them here.

So my only hesitation is the @var that's moving inside the if statements now, makes it harder to read IMHO. Do we care though?

The change doesn't bother me much since I kind of mentally filter them out. I suppose that means I assume they're for machine consumption. That's a subjective idea, so differing opinions are relevant. But for what it's worth I think it's ok.

I suppose the alternative would be to move assignments out of the conditions.

#25:

So my only hesitation is the @var that's moving inside the if statements now, makes it harder to read IMHO. Do we care though?

I think it probably doesn't take very long to get used to this style, but it looks a bit funky to me as well.

#26:

The change doesn't bother me much since I kind of mentally filter them out. I suppose that means I assume they're for machine consumption. That's a subjective idea, so differing opinions are relevant. But for what it's worth I think it's ok.

I suppose the alternative would be to move assignments out of the conditions.

I think moving the assignments out of the conditions would probably need to make them look like @var Thing|null $foo in a lot of places and that decreases the value of these in my opinion.

I did not manually test this, leaning on #24 for that.

A bunch of questions here, I only reviewed the first few pages.

Found there were a few annotations we can simply delete because they could be inferred. Some of these were due to improvements in core, some may have been due to improvements in phpstan-drupal and finally some may have never been needed in the first place.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased. MenuTreeStorageSchemaUpdateTest was deleted due to the pre-D12 old update function removals.

believe feedback has been addressed for this one, no additional feedback.