Skip to content
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.

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

Code Modernization: Avoid passing null to parameter 2 of http_build_query() #1800

Closed

Conversation

anomiex
Copy link

@anomiex anomiex commented Nov 2, 2021

This fixes the "Deprecated: http_build_query(): Passing null to parameter #2 ($numeric_prefix) 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. The correct value for the $numeric_prefix parameter to http_build_query() is an empty string.

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


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.

…query()`

This fixes the "Deprecated: http_build_query(): Passing null to parameter WordPress#2 ($numeric_prefix) 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. The correct value for the `$numeric_prefix` parameter to `http_build_query()` is an empty string.

Trac ticket: https://core.trac.wordpress.org/ticket/53635
@@ -342,7 +342,7 @@ protected function setup_handle($url, $headers, $data, $options) {
$data = '';
}
elseif (!is_string($data)) {
$data = http_build_query($data, null, '&');
$data = http_build_query($data, '', '&');
Copy link

Choose a reason for hiding this comment

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

Requests is an upstream library. This change was made in WordPress/Requests#500 so just need to have a new version of it set and added to WP.

I'd reckon that it can be removed from this PR and rely on the upstream reference being pulled in.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @kraftbj. The fix in this file would be made in the Requests library. I know that @jrfnl has been working on PHP 8.1 compatibility fixes for it. Flagging her here.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the ping @hellofromtonya and yes, @kraftbj and @hellofromtonya are correct.
You can find the repo for Requests here: https://github.com/WordPress/Requests/ and all the PHP 8.1 related PRs here: https://github.com/WordPress/Requests/pulls?q=is%3Apr+php+8.1

All the Requests related fixes which were lined up in WP PR #1798, #1799 and this PR, have already been applied in Requests.

Work is under way to get it ready for a new release and we're hoping to release before the WP 5.9 RC deadline, but as this release will be a major (2.0), we're trying to still get some more changes in which could be considered breaking changes (stricter input validation) as otherwise those would need to wait until the next major.

If you'd like to get involved with Requests or want to help us get it ready for the next release, please know there is a dedicated channel in the WP Slack #core-http-api, so feel free to ask questions on how to get started etc there.

Once the release is out, an update of the WP code can be expected forthwith. A test with the most pertinent breaking changes in Requests 2.0 has already been done, so all that's really needed is for that PR to be updated with the latest changes from Requests itself.

Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. Please revert the change in the Requests file as Requests is an external library.

@anomiex
Copy link
Author

anomiex commented Nov 2, 2021

Hmm. Hopefully you get the library updated sooner rather than later, as these errors are blocking our own work on PHP 8.1 compatibility.

@kraftbj
Copy link

kraftbj commented Nov 2, 2021

Hmm. Hopefully you get the library updated sooner rather than later, as these errors are blocking our own work on PHP 8.1 compatibility.

The project was recently moved into the WordPress organization, so the Core group with access (not me!) can cut a new version when needed. At worst, I would expect it in time for the 5.9 Beta 1 (Nov 16 ETA).

https://core.trac.wordpress.org/changeset/50842

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

The two changes currently remaining in this PR are good to go. This is the correct fix for this issue (and has also been applied elsewhere in the same manner).

@hellofromtonya
Copy link
Contributor

Committed in changeset https://core.trac.wordpress.org/changeset/52019.

@anomiex anomiex deleted the fix/php-8.1-null-to-http_build_query branch November 5, 2021 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants