- First commit to issue fork.
- π¦πΊAustralia mstrelan
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.
- πΊπΈUnited States smustgrave
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?
- π¦πΊAustralia mstrelan
@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); }