Fix PHPCS and PHPStan issues across multiple files#818
Fix PHPCS and PHPStan issues across multiple files#818aslamdoctor wants to merge 7 commits intoWordPress:masterfrom
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
@aslamdoctor U2F.php and fido-u2f-admin you can ignore, we'll remove them in PR #439 completely. |
- Add PHPStan bootstrap file for runtime constants (TWO_FACTOR_DIR, TWO_FACTOR_VERSION) - Add missing properties ($new, $last_used) to Registration class - Fix PHPDoc types for show_two_factor_login, process_provider, authentication_page, rename_link, delete_link, and pack64 - Fix undefined variable bug in wp_ajax_inline_save - Add input validation, sanitization, and wp_unslash for $_POST/$_REQUEST usage - Remove redundant isset($user->ID) checks and always-true conditions - Cast base_convert() result to int for array offset usage
FIDO/U2F files will be removed entirely in PR WordPress#439, so changes to U2F.php and class-two-factor-fido-u2f-admin.php are unnecessary.
77084c1 to
19d592e
Compare
|
Whoops my stuff was outdated. That's what I get for being up and working early on a Sunday. |
|
Thanks for your reviews @georgestephanis. Always highly appreciated!! :) |
|
Can you mark them resolved? I didn't as I wanted to make sure all is ok. |
Tests pass false as $user to authentication methods. Replace the removed isset($user->ID) checks with explicit early return guards to safely handle this case without accessing properties on false.
Rename phpstan-bootstrap.php to constants.php and include it from two-factor.php so TWO_FACTOR_DIR and TWO_FACTOR_VERSION are defined in a single place.
constants.php
Outdated
| @@ -0,0 +1,17 @@ | |||
| <?php | |||
There was a problem hiding this comment.
While having this in a standalone file helps with unifying the source of truth, having another file to update during the release cycle makes it more difficult (version strings are spread across two files) and we would need to update the release documentation to account for this change.
I feel like we should not change the plugin DX just because phpstan can't reach those constants. Is there a way to update the plugin bootstrap/entry file (remove the ABSPATH check) so that phpstan is able to read the constants? Maybe just move the ABSPATH guard to after the constants are defined?
There was a problem hiding this comment.
Good point — having a separate file adds release friction without enough benefit.
Addressed in ac5d59a:
- Moved
TWO_FACTOR_DIRandTWO_FACTOR_VERSIONdefinitions intotwo-factor.phpbefore theABSPATHguard, so PHPStan can discover them directly from the scanned file. Tested it aswell ✅ - Removed
constants.phpentirely — version strings stay in one file. - Removed the
bootstrapFilesentry fromphpstan.dist.neonsince PHPStan (1.7.0+) picks up constants from scanned sources.
| } | ||
|
|
||
| $code = $this->sanitize_code_from_request( 'two-factor-email-code' ); | ||
| if ( ! isset( $user->ID ) || ! $code ) { |
There was a problem hiding this comment.
question: Is phpstan complaining about the isset( $user->ID ) when the $user is false or not WP_User?
There was a problem hiding this comment.
PHPStan at level 0 doesn't flag the isset( $user->ID ) pattern — it would only become an issue at higher levels (accessing a property on false). The early return guards were added as a code quality improvement to make the false handling explicit and keep the @param docblock accurate (WP_User|false). Happy to revert these changes if you'd prefer keeping the original isset() approach.
- Define TWO_FACTOR_DIR and TWO_FACTOR_VERSION in two-factor.php before the ABSPATH check so PHPStan can discover them from scanned files - Remove constants.php to keep version strings in a single file - Bump PHPStan analysis level from 0 to 4 - Exclude FIDO/U2F files from PHPStan analysis - Add phpstan-ignore for Jetpack runtime compatibility check
Summary
TWO_FACTOR_DIR,TWO_FACTOR_VERSION) unreachable during static analysis$new,$last_used) toRegistrationclass inincludes/Yubico/U2F.phpshow_two_factor_login,process_provider,authentication_page,rename_link,delete_link, andpack64wp_ajax_inline_savewhere a non-matching key could be incorrectly updatedsanitize_text_field), andwp_unslashfor$_POST/$_REQUESTusage inclass-two-factor-fido-u2f-admin.phpisset($user->ID)checks and always-true conditionsbase_convert()result to(int)for array offset usage inbase32_encode()Reference #437
Test plan
./vendor/bin/phpstan analyse --memory-limit=1G --no-progress— should pass with 0 errors at level 0phpstan.dist.neonand run analysis — should pass with 0 errorsphpstan.dist.neonand run analysis — should pass with 0 errors./vendor/bin/phpcs --standard=WordPress providers/class-two-factor-fido-u2f-admin.php— should pass with 0 errors/warnings