Make WordPress Core

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: sjoerdlinders's profile sjoerdlinders Owned by: sergeybiryukov's profile SergeyBiryukov
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

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

#2 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 5.8
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#3 follow-up: @jrf
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 ?

#4 @desrosj
2 years ago

  • Keywords reporter-feedback added

#5 in reply to: ↑ 3 @sjoerdlinders
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.

Last edited 2 years ago by sjoerdlinders (previous) (diff)

#6 @sjoerdlinders
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 @sjoerdlinders
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 @hellofromTonya
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 @hellofromTonya
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 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?

#10 @hellofromTonya
19 months ago

  • Keywords php8 added

#11 @jrf
19 months ago

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 ?

#12 @hellofromTonya
19 months ago

  • Milestone changed from 5.9 to 6.0

This ticket was mentioned in Slack in #core by costdev. View the logs.


14 months ago

#14 @costdev
14 months ago

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

#15 @peterwilsoncc
14 months ago

  • Milestone changed from 6.0 to Future Release

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?

#16 @hellofromTonya
2 months ago

  • Keywords php80 added; php8 removed
  • Milestone changed from Future Release to 6.3
Note: See TracTickets for help on using tickets.