phpspec / prophecy

Highly opinionated mocking framework for PHP 5.3+
MIT License
8.53k stars 240 forks source link

PHPStan enhancements #612

Closed Wirone closed 7 months ago

Wirone commented 7 months ago

Details in commits' messages.

Strict rules and bleeding edge were enabled without any fixes because I need your feedback if you agree to enable them. If yes, I suggest merging this as-is and I can try to work on it in subsequent PR and shrink the baseline a little 🙂. Removed from this PR.

stof commented 7 months ago

is the PHP baseline as strict conversion of the existing neon baseline or are there new errors as well ? The conversion makes it impossible to determine that in the review.

Also, does phpstan automatically detects the PHP format when using --regenerate-baseline now ? Last time I checked, using a PHP baseline required passing the filename explicitly which makes it harder to contributors. As our baseline is quite small, I'm not sure we need to convert it to PHP format which is not the default.

Also, regarding the phpstan.neon file, I'm not sure it makes sense to move to .dist and ignoring phpstan.neon. I don't see a use case for using a custom phpstan configuration locally (and if you have such temporary use case, you can still use a custom need)

Wirone commented 7 months ago
  1. As stated in the commits' messages, new errors were dumped to baseline because of strict types package and bleeding edge (each commit shows what new errors occurred).
  2. Unfortunately, it's required to pass baseline's file name explicitly. I suggest adding phpstan and phpstan:baseline Composer scripts that would make it transparent for developers. You can see it here, it works great because you can change the underlying command and usage is still the same.
  3. This is approach officially suggested by PHPStan's documentation (it was pointed in the commit's message). Since phpstan.dist.neon is read by default with lower priority, for people that don't make custom config it's 100% transparent. For people who want customise config phpstan.neon is also read by default, but with higher priority. So in the end everyone can just run phpstan analyse and proper config will be used without the need to pass it as a CLI option. It is especially helpful for setting parallelisation params or editorUrl that enables navigation from CLI to IDE and when you use Docker (so each dev can set it as needed).
stof commented 7 months ago

As our baseline is not big enough so that parsing the neon files becomes the botteneck of running the analysis, I would suggest keeping it in the default neon format to make things easier for contributors so that --regenerate-baseline works out of the box.

Even if we add composer scripts, this won't solve it for contributors that don't know about them and run the phpstan command they know.

stof commented 7 months ago

Regarding strict rules and bleeding edge, I'm not OK to just enable them with everything in the baseline. The question then is how much of this is fixable (considering that we still need to support multiple PHP versions and that we do have legitimate use cases for == for instance). But some of those errors are indeed worth fixing (for instance the bunch of Token implementations that document their scoring method as returning bool|int instead of the int|false return type of the interface)

Wirone commented 7 months ago

OK, I'll remove strict types and bleeding edge and will provide them in separate PRs with fixes. And will revert NEON format of the baseline 👍.

Wirone commented 7 months ago

As our baseline is not big enough so that parsing the neon files becomes the botteneck of running the analysis (...)

@stof PHP format was introduced mostly because of NEON performance, but I pointed other argument in favour of it (in the commit's message) - it's possible to navigate from baseline to the file containing error in a single click in IDE. Nice little perk 😉.

stof commented 7 months ago

@Wirone then try to convince the phpstan maintainer to make the PHP format a first-class citizen for --regenerate-baseline called without argument, so that it would detect the format of the existing baseline. Once that is the case, it would remove my objection.