Opened 2 years ago
Last modified 2 months ago
#52622 reviewing defect (bug)
Fix PHP Warning on PHP7.2 in class-wp-http-curl.php
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.3 | Priority: | normal |
Severity: | normal | Version: | 5.6.2 |
Component: | HTTP API | Keywords: | has-patch needs-unit-tests php80 |
Focuses: | administration, coding-standards | Cc: |
Description
Added the default settings parameter 'decompress' with the default value 'false'.
<?php $defaults = array( 'method' => 'GET', 'timeout' => 5, 'redirection' => 5, 'httpversion' => '1.0', 'blocking' => true, 'headers' => array(), 'body' => null, 'cookies' => array(), 'decompress' => false, );
If this array element is not pre-defined it will result in a warning on line 316:
<?php if ( true === $parsed_args['decompress'] && ...
Change History (16)
This ticket was mentioned in PR #1036 on WordPress/wordpress-develop by sjoerdlinders.
2 years ago
#1
- Keywords has-patch added; needs-patch removed
#2
@
2 years ago
- Milestone changed from Awaiting Review to 5.8
- Owner set to SergeyBiryukov
- Status changed from new to reviewing
#3
follow-up:
↓ 5
@
2 years ago
@sjoerdlinders Out of interest and to document this issue properly:
- What is the warning you see ?
- How can the issue be reproduced ?
- Can this change be safeguarded by adding a unit test which would demonstrate the warning without the fix ?
#5
in reply to:
↑ 3
@
2 years ago
Replying to jrf:
- What is the warning you see ?
E_NOTICE => Undefined index: decompress
wp-includes/class-wp-http-curl.php (line 315)
if ( true === $parsed_args['decompress'] && ...
This notice is caught by the file 'wp-admin/admin-header.php' (line 197)
$error_get_last = error_get_last();
and will add the following 'php-error' class in the admin body tag: (line 205)
$admin_body_class .= ' php-error';
which will show extra header space on the admin page, without the actual notice issue itself.
- How can the issue be reproduced ?
On a WordPress Multisite (WPMU) setup this notice will be given when you switch to an other domain.
#6
@
2 years ago
Second change:
Added the default settings parameter 'filename' with a default empty string value .
$defaults = array(
'method' => 'GET',
'timeout' => 5,
'redirection' => 5,
'httpversion' => '1.0',
'blocking' => true,
'headers' => array(),
'body' => null,
'cookies' => array(),
'decompress' => false,
'filename' => '',
);
If this array element is not pre-defined it will result in a E_NOTICE 'Undefined index: filename' on line 306:
'filename' => $parsed_args['filename'],
This can be reproduces and will show exactly the same result problems as discribed in my reply #comment:5
(https://github.com/WordPress/wordpress-develop/pull/1036/)
#7
@
2 years ago
- Keywords reporter-feedback removed
Third and final change:
Added the default settings parameter 'stream' with a default null value.
$defaults = array(
'method' => 'GET',
'timeout' => 5,
'redirection' => 5,
'httpversion' => '1.0',
'blocking' => true,
'headers' => array(),
'body' => null,
'cookies' => array(),
'decompress' => false,
'stream' => null,
'filename' => '',
);
If this array element is not pre-defined it will result in a E_NOTICE 'Undefined index: stream' on line 299:
if ( $parsed_args['stream'] ) {
This can be reproduces and will show exactly the same result problems as discribed in my reply #comment:5
(https://github.com/WordPress/wordpress-develop/pull/1036/)
#8
@
2 years ago
- Keywords needs-testing needs-unit-tests added
- Milestone changed from 5.8 to 5.9
Today is 5.8 Beta 1. Ran out of time for this one to land. Punting to 5.9.
#9
@
19 months ago
- Keywords needs-testing removed
Hmm interesting that these parameters are not part of the defaults or at minimum checked before usage within the WP_Http_Curl::request()
method.
Looking at this deeper to ensure no backwards-compatibility (BC) breaks:
- None of these are pre-defined defaults
- Default value? In PHP 5.6 to PHP 8.1, a missing key evaluates to
null
. See in action here https://3v4l.org/qORqI.
Currently each is null
if not defined. Hmm, does that value make sense? Expect for filename
, the other two are used in conditional expressions as booleans.
'decompress'
is checked for a stricttrue
. Default:false
.'stream'
is used for as a truthy check. Default:false
'filename'
is used infopen()
, [which requires a non-nullable string](https://www.php.net/manual/en/function.fopen.php). An empty string makes sense as its default value. But hold on, it's also used to set the'filename'
in the$response
array. Current value then (when not defined) isnull
in the response. Default then should stay asnull
to avoid a BC-break. Then thefopen()
can be guarded as there's no reason to invokefopen()
without a filename to attempt to open.
Also want to note that in PHP 8.0+, each of these will throw a Warning: Undefined array key
.
What about tests? Guess what?! There are no tests for WP_Http_Curl
class. It'll need a testing strategy for curl.
I'm thinking of this approach:
- Get consensus on the default value for each of these missing arguments.
- Get those committed.
- Then in a separate ticket:
- Add tests
- Do validation. For example, there's no need to pass a null or empty string to
fopen()
as literally there would be nothing to open.
@SergeyBiryukov @jrf Do you agree with the default values?
<?php 'decompress' => false, 'stream' => false, 'filename' => null,
If yes, I'd advocate this get committed to ship with 5.9. Do you agree?
#11
@
19 months ago
I've only had a quick look, but I think punting this to WP 6.0 would be the better choice.
- It's very late in the day for WP 5.9.
- It looks like the class is making direct Curl requests instead of using the Requests library.
- No tests, so no safeguard we're not breaking something.
Does sound like a good ticket to have a more in-depth discussion about, possibly in the livestream @hellofromTonya and me do regularly ?
This ticket was mentioned in Slack in #core by costdev. View the logs.
14 months ago
#14
@
14 months ago
This ticket was discussed in tonight's bug scrub. Has there been any progress with the discussion off-Trac?
Added the default settings parameter 'decompress' with the default value 'false'.
If this array element is not pre-defined it will result in a warning on line 316:
if ( true === $parsed_argsdecompress? && ...
Trac ticket: https://core.trac.wordpress.org/ticket/52622