Introduce IgnoreErrorExtension

[ruudk]

This allows for more flexibility in ignoring errors.

Some use cases:

• Ignore missingType.iterableValue on controller actions:
  Rule: when the method is public and it has #[Route] attribute or the
  class has #[AsController] attribute.
• Ignore should return int but returns int|null on getId for entities.
  Rule: class needs to have #[Entity] attribute.
• Ignore never returns null so it can be removed from the return type
  Rule: method needs to have #[GraphQL\Field] attribute.
• Enforce missingCheckedExceptionInThrows partially, only for specific classes.

This idea came from phpstan/phpstan#11134

Note

@ondrejmirtes Before I continue with adding tests and improving things, I would first like to know if agree with this functionality.

[LoogleCZ]

Hi,

this is exactly what I'm looking for right now as I'm working on Magento 2 projects when I'm bit of forced into following error:

Method Foo\Bar\Plugin\SomeClass::aroundBuildOutputDataArray() has parameter $dataObjectType with no type specified.
         🪪  missingType.parameter

I do not want to ignore all missingType.parameter errors in Method Foo\Bar\Plugin\SomeClass as suggested in comment, but I'd appreciate a way I can determine when the error will be filtered out (In my case, it will be all plugin methods, so class namespace will contain Plugin and method name will start with around/before/after with first argument strongly typed to some existing class).

I'd use this proposed feature or I can use configuration with something like this:

parameters:
	ignoreErrors:
		-
			message: '#^Method [a-zA-Z0-9\\_]+\\Plugin\\[a-zA-Z0-9\\_]+\:\:(before|after|around)[a-zA-Z0-9_-]+\(\) has parameter \$[a-zA-Z0-9_-]* with no type specified#'
			identifier: missingType.parameter
			path: */Plugin/*

And for the requirement to have strongly typed first argument and ignore error based on that - I can then create custom rule to check this condition. Maybe you can do that too in your case.

[ruudk]

@LoogleCZ Yes, with this extension you would be able to filter errors based on their class, namespace, if they use an attribute, if this extend from some base class, etc... Curious what Ondrej thinks about it 😊

[ondrejmirtes]

Yeah, this would be a nice addition. But there's a lot of details that we need to get right (because ignoring errors is one of the most popular and most widely used PHPStan feature!).

Some notes I have after thinking about this:

1. I'd avoid "filter" in the name. This should be "IgnoreErrorsExtension".
2. We have to think about when it makes sense to apply it. There are different shortcomings to different places. For example, does an error first go through ignoreErrors (config) and only after that through the registered extensions?
3. Do we pass Error or RuleError to the extension? I'm leaning towards Error for many reasons. For example, it'd be great if errors, even when eventually filtered out, would still be part of the result cache. Changing an IgnoreErrorsExtension would mean we don't need to invalidate the result cache. Also, ErrorFormatter works with Error.
4. Don't forget to also apply IgnoreErrorsExtension to errors produced by CollectedDataNode rules (in AnalyserResultFinalizer).

[ruudk]

1. I'd avoid "filter" in the name. This should be "IgnoreErrorsExtension".

Sure

2. We have to think about when it makes sense to apply it. There are different shortcomings to different places. For example, does an error first go through ignoreErrors (config) and only after that through the registered extensions?

If we would first want to process the ignoreErrors, then the current place where I call the extension does not make sense. How would we be able to get the Node at the ignoreErrors stage? I think that information is not part of the Error, right?

Is it even possible to put the Node in the Error, and restore that from the result cache?

3. Do we pass Error or RuleError to the extension? I'm leaning towards Error for many reasons. For example, it'd be great if errors, even when eventually filtered out, would still be part of the result cache. Changing an IgnoreErrorsExtension would mean we don't need to invalidate the result cache. Also, ErrorFormatter works with Error.

Sure, we can first call the RuleErrorTransformer and then pass the Error to the filter.

4. Don't forget to also apply IgnoreErrorsExtension to errors produced by CollectedDataNode rules (in AnalyserResultFinalizer).

Will take care of this later once we iron out 2 + 3 😉

[ondrejmirtes]

Yeah, if you need to access Node and Scope and not just stuff that's available on Error then my whole argument about result cache is moot.

[ondrejmirtes]

So to summarize:

1. Rename the extension so that it has "IgnoreError" in the name.
2. Call it after transforming from RuleError to Error.
3. Don't forget about calling in on CollectedDataNode rule errors.

Thank you!

[ruudk]

I pushed the requested changes, please have a look when you have time. When you think it looks OK, I will continue with adding tests and tweaking things.

[ondrejmirtes]

I totally forgot about this! It got lost in my 100+ unread-emails-inbox. But it occured to me it might be a solution to a problem someone's faced so here I am again to give some feedback.

[ondrejmirtes]

Otherwise 👍

[ondrejmirtes]

This looks good!

Now for the tests. I'd be completely fine with just E2E tests in this manner:

phpstan-src/.github/workflows/e2e-tests.yml

Lines 216 to 245 in ee7eb11

	- script: |
	cd e2e/bad-exclude-paths
	cp -r tmp-node-modules node_modules
	OUTPUT=$(../../bin/phpstan analyse -c ignoreNonexistentExcludePath.neon)
	echo "$OUTPUT"
	- script: |
	cd e2e/bad-exclude-paths
	OUTPUT=$(../../bin/phpstan analyse -c ignoreReportUnmatchedFalse.neon)
	echo "$OUTPUT"
	- script: |
	cd e2e/bug-11826
	composer install
	OUTPUT=$(../bashunit -a exit_code "1" "../../bin/phpstan")
	echo "$OUTPUT"
	../bashunit -a contains 'Child process error (exit code 255): PHP Fatal error' "$OUTPUT"
	../bashunit -a contains 'Result is incomplete because of severe errors.' "$OUTPUT"
	- script: |
	cd e2e/bug-11857
	composer install
	../../bin/phpstan
	- script: |
	cd e2e/result-cache-meta-extension
	composer install
	../../bin/phpstan -vvv
	../../bin/phpstan -vvv --fail-without-result-cache
	echo 'modified-hash' > hash.txt
	OUTPUT=$(../bashunit -a exit_code "2" "../../bin/phpstan -vvv --fail-without-result-cache")
	echo "$OUTPUT"
	../bashunit -a matches "Note: Using configuration file .+phpstan.neon." "$OUTPUT"
	../bashunit -a contains 'Result cache not used because the metadata do not match: metaExtensions' "$OUTPUT"

[phpstan-bot]

This pull request has been marked as ready for review.

[ruudk]

@ondrejmirtes It's ready for review 🎉

[ondrejmirtes]

Even if I comment this change, all tests are passing. Please add a collector + CollectedDataNode rule with an error to the E2E test, and ignore it with an extension, so we can verify this code does what it should.

[ruudk]

Good catch! I added the tests. While doing it, I wonder, what the value of this would be. Since the Node points to the CollectedDataNode, that contains all data.

I think in real world scenario's, this is not interesting, but maybe I don't see it yet.

[ruudk]

@ondrejmirtes It's ready again. The failures are unrelated to this PR AFAIK.

[ondrejmirtes]

Thank you! Please contribut the documentation now.

Should be very similar to phpstan/phpstan@3fa1a7c

[ruudk]

🫡 Thanks for the merge, the docs are added here:

• Add IgnoreErrorExtension docs phpstan#12628
• Add link to docs in comment for IgnoreErrorExtension #3833

[ruudk]

With the release of 2.1.7 I immediately added my own IgnoreErrorExtension and it didn't work as expected.

After a lot of trial and error, I noticed it was related to some cache. Even though I did run clear-result-cache it did not call the shouldIgnore method for my error. When I ran phpstan analyze --debug the/file.php it did work.

So that makes me wonder, is there anything we should do in the caching system? When such extension is added, it should invalidate the cache, so that all errors will be passed through the method.

@ondrejmirtes I don't know too much about how this works, but if you have a hint, I can implement / fix it. Maybe it's similar to #3765

[ondrejmirtes]

There's no other cache besides the one deleted by clear-result-cache. Whole analysis runs again and FileAnalyser is called for all analysed files.

First you should look into logic in your new extension, maybe there's a bug in it.

If that doesn't help, please reproduce the issue in a public repository so I can take a look.

[ruudk]

Nevermind. Turns out I forgot to pass --configuration=phpstan.php when running phpstan clear-result-cache. Normally I use an alias to invoke phpstan with the correct configuration. 🙈

[ondrejmirtes]

I don't really approve or endose using .php as main config file format. Although it works, you should prefer .neon which is auto-discovered and this mistake wouldn't happen to you :)

.php is officially supported for generating and reading baselines (for performance reasons). Aside from that, I also sometimes recommend it to conditionally include other .neon files, like here: https://github.com/phpstan/phpstan-src/blob/2.1.x/build/ignore-by-php-version.neon.php

[ruudk]

I'm aware, and you also explained this somewhere else where you said you want to eventually / maybe work on a PHP class configuration format.

For what it's worth: For us, the benefits of using PHP outweigh for yet another config format (PHPStan is the only one using Neon in our project).
We have all our config in PHP (no yaml, no xml, no neon).
It allows PHP CS Fixer / Rector to keep them formatted.
We can use FQCN class/constant references, and import them. Have them clickable, plus it's possible to find usages when you on the other side (natively in PHPStorm, without the need of a plugin).
We can use PHP code for specific use cases too, like our dynamic config finder for PHPStan:

<?php declare(strict_types=1);

use Symfony\Component\Finder\Finder;

$files = Finder::create()->files()->name('*.php')->sortByName()->in(__DIR__ . '/src-dev/PHPStan/config');

return [
    'includes' => [
        __DIR__ . '/phpstan-baseline.php',
        ...array_values(array_map(fn ($file) => $file->getRealPath(), iterator_to_array($files))),
    ],
    // ...

The above works so neat. Simply add a new file to the config directory, and PHPStan picks it up.

[staabm]

btw: I just realized another use-case for this extension type.

we are now able to easily filter errors for 3rd party rules. we no longer need to copy/paste these rules or use patch files to make them work for us, as we need it in our context

[ondrejmirtes]

@staabm I don't get it. You can filter errors by identifiers already in the config.

[staabm]

You can filter errors by identifiers already in the config.

with this error extension we can filter them more fine-grained though.
a recent use-case e.g. was allow a certain error of a 3rd party extension only within context of certain subclasses

[ondrejmirtes]

Yeah, sure :)