Problem/Motivation

PHPStan baseline is currently skipping multiple Call to an undefined method errors.

Proposed resolution

Fix RecursiveExtensionFilterIterator errors, clean up the baseline.

CommentFileSizeAuthor
#6 3317223-2.patch1.46 KBlongwave
#4 3317223-4-test.patch792 bytesmondrake
#2 3317223-2.patch1.46 KBmondrake

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new 3317223-2.patch1.46 KB
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented 
+++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterIterator.php
@@ -136,7 +136,7 @@ public function accept() {
+    if ($this->current()->isDir()) {

How did this work before? Do we have no test coverage for it? Is it dead code?

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Needs work
StatusFileSize
new 3317223-4-test.patch792 bytes

Let's see.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

Per #4, it’s not dead code and we have test coverage.

We are traversing a tree here, via an iterator and a filter, and I suspect the filter is wrapping the object and magically passing over calls to its methods (note: I never used these features).

So PHPstan here is both wrong (the isDir method executes all good) and right (the method is not defined explicitly).

IMHO the readibility and understandability of code improves after this change though.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 3317223-2.patch1.46 KB

Aha, found the clue in the PHP docs: https://www.php.net/manual/en/class.iteratoriterator.php

Note:
This class permits access to methods of the inner iterator via the __call magic method.

I guess this might be worth raising with PHPStan. But I also agree that fixing it improves readability, so reuploading #2 and marking RTBC.

  • catch committed ecb9ad9 on 10.0.x 
    Issue #3317223 by mondrake, longwave: Fix...
  • catch committed 23b444a on 10.1.x
    Issue #3317223 by mondrake, longwave: Fix...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Agreed with all the reasoning here - it's a slight improvement even if it's not necessary, so why not.

Committed/pushed to 10.1.x and cherry-picked to 10.0.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.