Skip to content

fix: Refactor FileAnalyser instantiation with named arguments #42

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 14, 2025

Conversation

thimarsola
Copy link
Contributor

This pull request includes a few updates to the PHPStanAnalyser.php file to enhance dependency injection and improve code readability. The most important changes include the addition of a new import and the refactoring of the FileAnalyser instantiation to use named parameters.

Dependency Injection and Code Readability Improvements:

  • src/PHPStanAnalyser.php: Added the IgnoreErrorExtensionProvider import to support the new dependency injection.
  • public static function make(Container $container, array $rules, array $collector in src/PHPStanAnalyser.php: Refactored the FileAnalyser instantiation to use named parameters, including the new ignoreErrorExtensionProvider dependency.

Xnip2025-03-05_23-27-20

Updated the `FileAnalyser` constructor usage to leverage named arguments for improved clarity and maintainability. Added `IgnoreErrorExtensionProvider` as a dependency to align with updated requirements.
Copy link

@nick-lai nick-lai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@unisys12
Copy link

Oh my! This PR just ended several hours of debugging for me. I have a project that I have been working on for a few weeks and have been using PestPHP and the type-coverage plugin. I wanted to experiment with one of the new Laravel 12 starter kits, so I created a new project and imported all my backend code and tests into the new project. Installed all the required dev packages for testing (over 80 assertions at the moment). All tests pass, as expected, but I get this error when running type coverage. So many rabbit holes! It's obvious what the problem was, but we all know your not suppose to change vendor code. I came here to fork the project start picking at it, but checked open PR's first.

Thank you @thimarsola !

@LucaPipolo
Copy link

@thimarsola @nick-lai any chances to have this merged anytime soon? Thanks for your great work! 🚀

@thimarsola
Copy link
Contributor Author

@LucaPipolo I'm waiting for the approval workflow to proceed with the completion of this Pull Request.

@nick-lai
Copy link

I encountered the same issue and, upon quickly searching the repository's issues and PRs, found that @thimarsola had already created this PR.

At that time, @nunomaduro might have been busy preparing for LaraconIN, so I assisted in reviewing the PR. After testing, I confirmed that this PR resolved the issue I faced, so I approved it and commented, "Looks good to me," awaiting further confirmation and merge from the maintainers.

cc @owenvoke

@nick-lai
Copy link

For those currently encountering this issue, there is a temporary mitigation. Until this PR is merged, you can lock the upstream dependency phpstan/phpstan to version 2.1.6 by running:

composer require phpstan/phpstan:2.1.6 --dev

Because phpstan/phpstan v2.1.7 has made the following changes:
https://github.com/phpstan/phpstan-src/pull/3783/files#diff-e5565fad48542a95d917f9eba387b39a4db2c79adcca361d7db06390ef9cf4bfR20-R22

@owenvoke
Copy link
Member

We should likely require a minimum of ^2.1.7 in Composer. However, I assume this also breaks backwards compatibility with 1.x?

@nick-lai
Copy link

It is indeed necessary to adjust the minimum compatible version of PHPStan in composer.json, as follows:

"require": {
"php": "^8.2",
"phpstan/phpstan": "^1.12.10|^2.1.4",
"tomasvotruba/type-coverage": "^1.0.0|^2.0.2",
"pestphp/pest-plugin": "^3.0.0"
},

    "require": {
        "php": "^8.2",
-       "phpstan/phpstan": "^1.12.10|^2.1.4",
+       "phpstan/phpstan": "^2.1.7",
        "tomasvotruba/type-coverage": "^1.0.0|^2.0.2",
        "pestphp/pest-plugin": "^3.0.0"
    },

However, I believe the main issue is that PHPStan did not adhere to the Semver guidelines when moving from 2.1.6 to 2.1.7. The changes to FileAnalyser in phpstan/phpstan-src#3783 can be seen as breaking changes (perhaps PHPStan should consider releasing version 3.0.0?). This affects all downstream libraries that depend on PHPStan, and I'm not sure whether PHPStan will promise to following Semver in phpstan/phpstan-src.

@nunomaduro nunomaduro merged commit ad01d4c into pestphp:3.x Mar 14, 2025
2 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants