Bug #25753
openAddress PHPStan issues in cac-onboarding and bp-two-factor
0%
Description
As mentioned on the dev call last week, the cac-onboarding fatal error that caused #25655 could have been caught if a PHPStan task was set up.
I ran PHPStan locally for the cac-onboarding and bp-two-factor plugins and am going to push up some changes to address some minor things that PHPStan found.
Updated by Raymond Hoh 23 days ago
In https://github.com/cuny-academic-commons/cac/commit/ff5b38a6f7, I've pinned cac-onboarding with the following changes: https://github.com/cuny-academic-commons/cac-onboarding/compare/5218a45...2b5b67b.
The main error PHPStan found was a group ID type mismatch during invitation acceptance: https://github.com/cuny-academic-commons/cac-onboarding/commit/c814bdc
In https://github.com/cuny-academic-commons/cac/commit/2700bc9e2f, I've addressed some PHPStan issues with bp-two-factor. Mostly PHPDoc fixes for the most part.
I'd like to add both cac-onboarding and bp-two-factor to our PHPStan matrix, but first, I want to ask Jeremy about whether setting up individual PHPStan jobs would be better or not. Right now, whenever a PHP file is changed, we run one PHPStan job that checks all our watched directories. What if we added individual PHPStan jobs for each of our watched directories? So if we made a change to /wp-content/mu-plugins/, that would only run the phpstan:mu-plugins script and none of the other PHPStan ones. Would there be a negative with doing this? If there is, then let's keep the same set up that we have right now and I'll add cac-onboarding and bp-two-factor to the existing matrix.
Updated by Boone Gorges 21 days ago
Following up on yesterday's dev call: I think we generally decided that the risk of separating the calls is probably not worth the effort. There may be cases where an update in one library causes a failure in a separate but related library. So we're tentatively going to leave things as they are, so that any PHP change runs all the tests.
The one potential exception would be if running the full PHPStan test suite either made the build too long-running or if it was causing us to exceed our GitHub Actions budget. I'm not concerned about time, but Jeremy, perhaps you could double-check that we're under our resource threshold.