Fix PHPCS and PHPStan issues across multiple files#818
Fix PHPCS and PHPStan issues across multiple files#818aslamdoctor wants to merge 3 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. |
masteradhoc
requested review from
georgestephanis,
kasparsd and
masteradhoc
and removed request for
kasparsd and
masteradhoc
| <?php | ||
| /** | ||
| * PHPStan bootstrap file. | ||
| * | ||
| * Defines constants that are set at runtime in two-factor.php | ||
| * but unreachable during static analysis because of the ABSPATH guard. | ||
| */ | ||
|
|
||
| define( 'TWO_FACTOR_DIR', __DIR__ . '/' ); | ||
| define( 'TWO_FACTOR_VERSION', '0.15.0' ); |
There was a problem hiding this comment.
I almost wonder if this would be good to rename more generally and include from the main plugin file so these aren't needing to be maintained in multiple places?
I could be overthinking this though.
- 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.
aslamdoctor
force-pushed
the
437-php-wpcs-fixes
branch
from
77084c1 to
19d592e
Compare
| public function validate_authentication( $user ) { | ||
| $code = $this->sanitize_code_from_request( 'two-factor-email-code' ); | ||
| if ( ! isset( $user->ID ) || ! $code ) { | ||
| if ( ! $code ) { |
There was a problem hiding this comment.
A number of these extra conditionals that you're clearing are there to account for unit tests that pass in false to authentication page methods --
Are we sure they're safe to remove in the testing context as well as the real-world use context?
There was a problem hiding this comment.
Good catch! Tests do pass false as $user to these methods (e.g. test_authentication_page_no_user), so removing isset($user->ID) without a replacement guard would cause errors when $code is truthy or the resend param is set.
I've added explicit early return guards (if ( \! $user ) return false;) to both pre_process_authentication() and validate_authentication(), matching the pattern already used in authentication_page(). Also updated the @param docs to WP_User|false.
|
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.
3 participants
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