Remove all of our PHPStan `ignoreErrors` entries AND ignore PHPStan errors caused by upstream changes in core

we probably need to expand , phpstan.neon.dist, ignoreErrors:

To verify the regex i added, you can look at the results of fee19fc8 for the errors.

Discussed this in scrum with the team.

Since this is very much a moving target as core becomes more compliant with PHPStan, I think that it makes sense for us to add ignore rules progressively in order to keep ourselves unblocked.

HOWEVER...

This issue is NOT to be considered fixed until there are NO PHPStan rules being ignored by us. If we need to ignore things as core churns upstream, then fine; we can continually open new merge requests against this issue's fork, and commit them as needed. But eventually, we need to actually fix the problems and remove the ignore rules, and once that's completely done, we can close this issue. I have spoken!

I'm going to start fixing the errors.

I am running into a weird error of directory not being writable, i also think we need to ignore the errors related to tearDown as we are calling the parent it's just that it's in a try-catch which seems reasonable to me and phpstan shouldn't have complained.

This was committed over a month ago… 😇

Nope!

The plan was to leave this open until all the PHPStan ignores are removed from our phpstan.neon file. The idea was to make multiple commits to this same issue over time to remove them progressively, then finally mark the issue as fixed once they were all gone.

Ah. Right.

Well … that was not at all clear based on the issue metadata. I see you wrote it in #6 though!

I thought the goal was to keep this open in case there were more upstream (as in 10.1) changes happening? With the alpha tagged, that should not happen anymore.

Then this issue didn't need review, but work?

There is not summary at all

Updated issue summary

As discussed just now: can you please expand why we still have those 2 ignored PHPStan errors?

We need to justify them. Because we'll need to do that to get it in core too.

You told me one of them appears to be a PHPStan bug. Let's make sure we link to a bug report, so that we can eventually remove it, and we'll be down to a single one!

updated

it doesn't error out on https://phpstan.org/r/1a212fa2-3cba-4c82-80db-7c75a8346f50 i'm missing something i think

#24: TIL about https://phpstan.org/try! Thank you! 😀

Unfortunately there seems to be no performance difference.

Okay so it no longer has a problem with \Drupal\Tests\package_manager\Kernel\PackageManagerKernelTestBase::tearDown
but there still is a problem with the try-catch. So it might be a bug after all.

Unfortunately there seems to be no performance difference.

That's what I expected honestly 😊

but there still is a problem with the try-catch. So it might be a bug after all.

But didn't your https://phpstan.org/r/1a212fa2-3cba-4c82-80db-7c75a8346f50 experiment from #24 prove that it should work? 🤔

Paired with @omkar.podey for an hour. Root cause found: a bug in \PHPStan\Rules\PHPUnit\ShouldCallParentMethodsRule::hasParentClassCall()! 🤪 Omkar will report this upstream and disable PHPStan for this one line until it's fixed upstream.

Created a bug report https://github.com/phpstan/phpstan-phpunit/issues/187

So the conclusion is we want 3351212-test-if-still-errors merged, it solves two things

1. Fix oversight of not calling parent::tearDown()
2. A solid reason for ignoring - "#^Missing call to parent::tearDown.* method#"

Once tests are passing again (after 📌 Since #3361983 was committed to Drupal core, psr/http-message needed to be explicitly required for build tests Needs work lands), this can be committed 👍

🎉

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