Skip to content

PHP 8.1: WP_User::get_data_by(): fix passing null to non-nullable deprecation #2133

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.

Sign up for GitHub

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

Jump to bottom
Open
hellofromtonya wants to merge 5 commits into
base: trunk
Choose a base branch
from
Open

PHP 8.1: WP_User::get_data_by(): fix passing null to non-nullable deprecation #2133

hellofromtonya wants to merge 5 commits into from

Conversation

@hellofromtonya
Copy link
Contributor

@hellofromtonya hellofromtonya commented Jan 10, 2022 •
edited
Loading

The PHP native trim() function expects to be passed a string and it is not a nullable parameter. Passing null to it will result in a trim(): Passing null to parameter #1 ($string) of type string is deprecated notice on PHP 8.1.

In the WP_User::get_data_by() static method, the value given is expected to be int or string. However no input validation was done for fields other than the id field.

This PR adds input validation for the field $value to ensure it is a scalar type and if no, bails out returning false. Why is_scalar()? For backwards-compatible, the native behavior of trim() is retained as it coerces non-string scalar types into a string.

This PR also adds a new test class specifically for WP_User::get_data_by() static method.

Trac ticket: https://core.trac.wordpress.org/ticket/55656
Trac ticket: https://core.trac.wordpress.org/ticket/54730

References:

Notes:

This PR resolves these tests:

8) WP_Test_REST_Users_Controller::test_update_item_no_change
trim(): Passing null to parameter #1 ($string) of type string is deprecated

/var/www/src/wp-includes/class-wp-user.php:211
/var/www/src/wp-includes/pluggable.php:105
/var/www/src/wp-includes/user.php:1798
/var/www/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php:728
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:1141
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:988
/var/www/tests/phpunit/includes/spy-rest-server.php:67
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:1610

9) WP_Test_REST_Users_Controller::test_update_item_en_US_locale
trim(): Passing null to parameter #1 ($string) of type string is deprecated

/var/www/src/wp-includes/class-wp-user.php:211
/var/www/src/wp-includes/pluggable.php:105
/var/www/src/wp-includes/user.php:1798
/var/www/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php:728
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:1141
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:988
/var/www/tests/phpunit/includes/spy-rest-server.php:67
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:1714

10) WP_Test_REST_Users_Controller::test_update_item_empty_locale
trim(): Passing null to parameter #1 ($string) of type string is deprecated

/var/www/src/wp-includes/class-wp-user.php:211
/var/www/src/wp-includes/pluggable.php:105
/var/www/src/wp-includes/user.php:1798
/var/www/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php:728
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:1141
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:988
/var/www/tests/phpunit/includes/spy-rest-server.php:67
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:1739

11) WP_Test_REST_Users_Controller::test_update_item_username_attempt
trim(): Passing null to parameter #1 ($string) of type string is deprecated

/var/www/src/wp-includes/class-wp-user.php:211
/var/www/src/wp-includes/pluggable.php:105
/var/www/src/wp-includes/user.php:1798
/var/www/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php:728
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:1141
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:988
/var/www/tests/phpunit/includes/spy-rest-server.php:67
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:1768

12) WP_Test_REST_Users_Controller::test_update_item_existing_nicename
trim(): Passing null to parameter #1 ($string) of type string is deprecated

/var/www/src/wp-includes/class-wp-user.php:211
/var/www/src/wp-includes/pluggable.php:105
/var/www/src/wp-includes/user.php:1798
/var/www/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php:728
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:1141
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:988
/var/www/tests/phpunit/includes/spy-rest-server.php:67
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:1793

13) WP_Test_REST_Users_Controller::test_update_user_role
trim(): Passing null to parameter #1 ($string) of type string is deprecated

/var/www/src/wp-includes/class-wp-user.php:211
/var/www/src/wp-includes/pluggable.php:105
/var/www/src/wp-includes/user.php:1798
/var/www/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php:728
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:1141
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:988
/var/www/tests/phpunit/includes/spy-rest-server.php:67
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:1852

14) WP_Test_REST_Users_Controller::test_update_user_multiple_roles
trim(): Passing null to parameter #1 ($string) of type string is deprecated

/var/www/src/wp-includes/class-wp-user.php:211
/var/www/src/wp-includes/pluggable.php:105
/var/www/src/wp-includes/user.php:1798
/var/www/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php:728
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:1141
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:988
/var/www/tests/phpunit/includes/spy-rest-server.php:67
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:1873

15) WP_Test_REST_Users_Controller::test_update_user_role_invalid_privilege_deescalation
trim(): Passing null to parameter #1 ($string) of type string is deprecated

/var/www/src/wp-includes/class-wp-user.php:211
/var/www/src/wp-includes/pluggable.php:105
/var/www/src/wp-includes/user.php:1798
/var/www/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php:728
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:1141
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:988
/var/www/tests/phpunit/includes/spy-rest-server.php:67
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:1917

16) WP_Test_REST_Users_Controller::test_update_user_role_invalid_role
trim(): Passing null to parameter #1 ($string) of type string is deprecated

/var/www/src/wp-includes/class-wp-user.php:211
/var/www/src/wp-includes/pluggable.php:105
/var/www/src/wp-includes/user.php:1798
/var/www/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php:728
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:1141
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:988
/var/www/tests/phpunit/includes/spy-rest-server.php:67
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:1977

17) WP_Test_REST_Users_Controller::test_update_item_only_roles_as_site_administrator
trim(): Passing null to parameter #1 ($string) of type string is deprecated

/var/www/src/wp-includes/class-wp-user.php:211
/var/www/src/wp-includes/pluggable.php:105
/var/www/src/wp-includes/user.php:1798
/var/www/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php:728
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:1141
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:988
/var/www/tests/phpunit/includes/spy-rest-server.php:67
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:2072

18) WP_Test_REST_Users_Controller::test_update_item_including_roles_and_other_params
trim(): Passing null to parameter #1 ($string) of type string is deprecated

/var/www/src/wp-includes/class-wp-user.php:211
/var/www/src/wp-includes/pluggable.php:105
/var/www/src/wp-includes/user.php:1798
/var/www/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php:728
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:1141
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:988
/var/www/tests/phpunit/includes/spy-rest-server.php:67
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:2094

19) WP_Test_REST_Users_Controller::test_user_roundtrip_as_editor
trim(): Passing null to parameter #1 ($string) of type string is deprecated

/var/www/src/wp-includes/class-wp-user.php:211
/var/www/src/wp-includes/pluggable.php:105
/var/www/src/wp-includes/user.php:1798
/var/www/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php:728
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:1141
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:988
/var/www/tests/phpunit/includes/spy-rest-server.php:67
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:2172
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:2217

20) WP_Test_REST_Users_Controller::test_user_roundtrip_as_editor_html
trim(): Passing null to parameter #1 ($string) of type string is deprecated

/var/www/src/wp-includes/class-wp-user.php:211
/var/www/src/wp-includes/pluggable.php:105
/var/www/src/wp-includes/user.php:1798
/var/www/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php:728
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:1141
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:988
/var/www/tests/phpunit/includes/spy-rest-server.php:67
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:2172
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:2268

21) WP_Test_REST_Users_Controller::test_user_roundtrip_as_superadmin
trim(): Passing null to parameter #1 ($string) of type string is deprecated

/var/www/src/wp-includes/class-wp-user.php:211
/var/www/src/wp-includes/pluggable.php:105
/var/www/src/wp-includes/user.php:1798
/var/www/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php:728
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:1141
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:988
/var/www/tests/phpunit/includes/spy-rest-server.php:67
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:2172
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:2304

22) WP_Test_REST_Users_Controller::test_user_roundtrip_as_superadmin_html
trim(): Passing null to parameter #1 ($string) of type string is deprecated

/var/www/src/wp-includes/class-wp-user.php:211
/var/www/src/wp-includes/pluggable.php:105
/var/www/src/wp-includes/user.php:1798
/var/www/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php:728
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:1141
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:988
/var/www/tests/phpunit/includes/spy-rest-server.php:67
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:2172
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:2333

23) WP_Test_REST_Users_Controller::test_get_additional_field_registration
trim(): Passing null to parameter #1 ($string) of type string is deprecated

/var/www/src/wp-includes/class-wp-user.php:211
/var/www/src/wp-includes/pluggable.php:105
/var/www/src/wp-includes/user.php:1798
/var/www/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php:728
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:1141
/var/www/src/wp-includes/rest-api/class-wp-rest-server.php:988
/var/www/tests/phpunit/includes/spy-rest-server.php:67
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:2764

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@hellofromtonya
Add test case and nonscalar value test.
b6c979d
@hellofromtonya
Added tests for scalar field values.
f484df5
@hellofromtonya
Add tests for id and ID fields.
02ebc52
@hellofromtonya
Validates field value input is scalar.
d19ef72 
The value should be string or integer. If the field is 'id' or
'ID', checks already exist for numeric ID; else, `false` is
returned.

For the user email, login, or slug/nicename, these values are
expected to be a string. The `trim()` for these field values
results in the following:

* string: trims
* non-string scalar: coerces to string
* `null`: empty string and on PHP 8.1, a deprecation notice
* non-scalar: Warning on PHP 5.6 or TypeError on PHP 8+
@hellofromtonya hellofromtonya requested a review from jrfnl January 10, 2022 20:31
@hellofromtonya hellofromtonya self-assigned this Jan 10, 2022
@anomiex anomiex mentioned this pull request Feb 14, 2022
Closed
costdev
costdev requested changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

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

Some changes suggested for consistency with the test suite and readability.

Test coverage from other tests in the suite appear to have most lines covered. However, neither existing tests nor this test class currently cover the default branch of switch ( $field ) {. See Codecov - L237 or 241 of the PR. It would be good to cover this for completeness.

This should cover it:

public function test_unaccepted_field() {
	$this->assertFalse( WP_User::get_data_by( 'user_url', 'http://tacos.com' ) );
}

tests/phpunit/tests/user/wp-user/getUserBy.php

$user = WP_User::get_data_by( 'id', $user_id );
$this->assertInstanceOf( 'stdClass', $user, 'User is not an instance of stdClass' );
$this->assertSame( self::$user_id, (int) $user->ID, 'User ID does not match the user generated by the factory' );
Copy link
Contributor

@costdev costdev Feb 15, 2022

Choose a reason for hiding this comment

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

This casts the $actual value to int rather than the $expected value to string. If the test is right, then the $actual type should not be corrected in the test, but in source.

However, if the $actual type is string and must be maintained for BC, I would suggest casting self::$user_id to string either in the assertion, or just once in wpSetUpBeforeClass() to save additional casts in other test methods (such as in test_alias_of_id).

This has been a common occurrence in the test suite, and we may benefit from casting the factory's $user_id to string since this has to be maintained for BC anyway.

tests/phpunit/tests/user/wp-user/getUserBy.php
public function test_alias_of_id() {
$user = WP_User::get_data_by( 'ID', self::$user_id );
$this->assertInstanceOf( 'stdClass', $user, 'User is not an instance of stdClass' );
$this->assertSame( self::$user_id, (int) $user->ID, 'User ID does not match the user generated by the factory' );
Copy link
Contributor

@costdev costdev Feb 15, 2022

Choose a reason for hiding this comment

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

See above comment about $actual value being cast to int.

tests/phpunit/tests/user/wp-user/getUserBy.php
return array(
'id' => array(
'field' => 'id',
'value' => null,
Copy link
Contributor

@costdev costdev Feb 15, 2022

Choose a reason for hiding this comment

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

I wonder if, given the subject of the test method, the $value here and in the next dataset might be better as 'REPL_USER_ID' for clarity in the test suite.

As null is not considered scalar by is_scalar() Docs, it initially looked to me as though null was being tested in test_scalar(). 'REPL_USER_ID' will continue to be overwritten by self::$user_id, so this is just for clarity when reading the tests.

@hellofromtonya hellofromtonya force-pushed the fix/php81/trim-nonnullable-WP_User-get_data_by branch from 4853a93 to 139be5e Compare February 15, 2022 22:06
@hellofromtonya hellofromtonya changed the title PHP 8.1: WP_User::get_data_by(): fix passing null to non-nullable deprecation (Trac 54730) PHP 8.1: WP_User::get_data_by(): fix passing null to non-nullable deprecation May 2, 2022
This was referenced Jul 12, 2023
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@costdev costdev costdev requested changes

@jrfnl jrfnl Awaiting requested review from jrfnl

Assignees

@hellofromtonya hellofromtonya

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hellofromtonya @costdev