Code Modernization: Avoid checking if a null email address exists in WP_REST_Users_Controller.

[anomiex]

This fixes a "Deprecated: trim(): Passing null to parameter #1 ($string) of type string is deprecated" warning on PHP 8.1.

PHP has started requiring that built-in method arguments that are not explicitly declared as nullable may no longer be passed null. In this case it seems to make more sense to avoid calling WordPress's own email_exists() function with null, as that method is declared as taking only strings and a null email is never going to exist anyway.

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

---

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]

I'm not seeing a test scenario for a null, '', or invalid email in the existing phpunit tests. Would be good to add these tests as part of this fix.

[anomiex]

FYI, you are in fact running into the deprecation this is fixing in your existing tests. See for example https://github.com/WordPress/wordpress-develop/runs/4083354600?check_suite_focus=true#step:20:403

And a test that would try to pass an empty string as an email address won't actually reach this code, as that gets rejected as "invalid email address" during parameter validation. I can still add such a test if you insist, but it wouldn't actually test the code being changed here.

[jrfnl]

I looked at that failing test before and my assessment at the time was that this is an issue with the way the test is set up, not with the actual code.

self::$user is not being correctly mocked in the wpSetUpBeforeClass() method in the WP_Test_REST_Users_Controller class which leads to the null for the email address.
This is not a situation which should ever occur in "real life" and should therefore not be solved in the src code, but by improving the test.

[anomiex]

You may want to check again. The problem isn't whether the user has an email address set, it's whether the incoming REST request specifies a value for the 'email' parameter (i.e. to update the email).

You should be able to prove that to yourself easily enough: Add something like

if ( null === $request['email'] ) return new WP_Error( 'email_is_null', 'Email is null', array( 'status' => '500' ) );

just before the email_exists() call, then try to make a REST request to update the description of an existing user that does have an email address.

[anomiex]

I see that you've closed https://core.trac.wordpress.org/ticket/53635 without addressing the issue this PR fixes. What's the way forward now?

[hellofromtonya]

I see that you've closed https://core.trac.wordpress.org/ticket/53635 without addressing the issue this PR fixes. What's the way forward now?

@anomiex Trac ticket https://core.trac.wordpress.org/ticket/54730 is now open for continuing PHP 8.x compatibility fixes during the 6.0 cycle. I updated the description in this PR to point at the new ticket.

[anomiex]

Looks like #2133 will fix this too, by making the called functions no longer object to being passed null. If that gets merged, this can be closed.

[hellofromtonya]

Looks like #2133 will fix this too, by making the called functions no longer object to being passed null. If that gets merged, this can be closed.

Once #2133 gets merged, then yes, it will resolve this PR's deprecation notice without further changes. Plus #2133 resolves the issue at the lower level, i.e. where the deprecation notice is triggered. I think overall this PR can be closed in favor of #2133.

Note: There's an improvement opportunity, though out of scope for the purpose of this PR:

• Guard email_exists() to invoke get_user_by() only when a string type is passed to email_exists(), i.e. providing a teeny tiny micro-optimization when a non-string is passed. (This tweak deserves a separate PR and tests.)