Fix PHP Warning on PHP7.2 in class-wp-http-curl.php

[sjoerdlinders]

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'] && ...

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

[jrf]

@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 ?

[sjoerdlinders]

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.

[sjoerdlinders]

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/)

[sjoerdlinders]

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/)

[hellofromTonya]

Today is 5.8 Beta 1. Ran out of time for this one to land. Punting to 5.9.

[hellofromTonya]

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 strict true. Default: false.
• 'stream' is used for as a truthy check. Default: false
• 'filename' is used in fopen(), [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) is null in the response. Default then should stay as null to avoid a BC-break. Then the fopen() can be guarded as there's no reason to invoke fopen() 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:

1. Get consensus on the default value for each of these missing arguments.
2. Get those committed.
3. 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?

[jrf]

I've only had a quick look, but I think punting this to WP 6.0 would be the better choice.

1. It's very late in the day for WP 5.9.
2. It looks like the class is making direct Curl requests instead of using the Requests library.
3. 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 ?

[costdev]

This ticket was discussed in tonight's bug scrub. Has there been any progress with the discussion off-Trac?

[peterwilsoncc]

I've moved this to future release given there hasn't been any activity during the current release cycle.

I think the answer is no but to be sure: does this need Requests 2?