carlosas / phpat

PHP Architecture Tester - Easy architecture testing for PHP :heavy_check_mark:
https://phpat.dev
MIT License
1.09k stars 44 forks source link

Support for PHPStan error identifiers #297

Open NanoSector opened 1 week ago

NanoSector commented 1 week ago

Enhancement description phpat currently reports errors to PHPStan without error identifiers. This means that in order to ignore a phpat error for whatever reason, it is currently required to use phpstan-ignore-next-line which potentially ignores much more than just phpat's output.

Consider the scenario where external dependency usage is blocked everywhere but a certain class. For example, we use the brick/money package for our monetary value support but wrap it in our own Money class for abstraction. Our codebase is covered by fairly broad rules which allow certain dependencies to be used in certain areas; this is generally correct as it is unwanted to allow brick/money in more than just our custom Money class.

This means our custom Money class is currently covered in the following comments:

/** @phpstan-ignore-next-line We need this dependency, but don't want to allow it in the entire layer */

That however disables all of PHPStan's inspections for the given lines. We currently work around this by splitting relevant entries into multiple lines and specifically targeting the lines that contain references to the dependency.

It would be extremely helpful to be able to selectively ignore phpat errors in these scenario's.

Suggested approach or solution PHPStan's rule builder has support for setting error identifiers: https://phpstan.org/blog/using-rule-error-builder

phpat can either set a global error identifier, but it might be more flexible to allow rule authors to specify custom rule identifiers. For example, the architecture rule that allows dependencies above might have its errors identified as app.architecture.disallowedVendorUsage and a rule which enforces a certain naming scheme could have its errors identified as app.architecture.controllerIsNotSuffixedWithController.

This would allow the following, assuming the usage of symfony/framework-bundle is not generally allowed:

/** @phpstan-ignore app.architecture.disallowedVendorUsage (We would like to use AbstractController here for [reasons]) */
final class MyControllerWithoutProperSuffix extends AbstractController

This would cause PHPStan to stop reporting the AbstractController usage, but would still have it report a broken naming convention.

carlosas commented 1 week ago

Hi! I'm already working on it as a part of the phpstan 2 support, in the next major version oh PHPat. The approach im working on is curating the rule names (based on the test method names) to transform them into valid identifiers.

Work in progress :)

carlosas commented 1 week ago

Also remember that you can exclude certain classes in your rules

NanoSector commented 1 week ago

Thanks for the quick response! Glad to hear it's on the agenda. Do you prefer to leave this ticket open, as #293 does not explicitly mention this? Using the test method names as identifiers also works in those scenarios, so sounds good to me.

Also remember that you can exclude certain classes in your rules

This would work for finely scoped rules. This becomes a problem because we have few but broad rules which cover the entire codebase. Adding an exception to these rules would allow the dependency everywhere in the codebase (and would therefore require more finer grained rules to cover this again).

robotomarvin commented 1 week ago

It would be nice to have them configurable (and possible even easier to implement), like in https://github.com/spaze/phpstan-disallowed-calls/blob/main/docs/custom-rules.md