Project

General

Profile

Actions

Bug #25753

open

Address PHPStan issues in cac-onboarding and bp-two-factor

Added by Raymond Hoh 23 days ago. Updated 8 days ago.

Status:
New
Priority name:
Normal
Assignee:
Category name:
WordPress Plugins
Target version:
Start date:
2026-09-28
Due date:
% Done:

0%

Estimated time:
Deployment actions:

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.

Actions #1

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.

Actions #2

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.

Actions #3

Updated by Boone Gorges 8 days ago

  • Target version changed from 2.7.13 to 2.7.14
Actions

Also available in: Atom PDF