Make WordPress Core

Opened 3 years ago

Closed 11 months ago

Last modified 11 months ago

#52622 closed defect (bug) (fixed)

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 (21)

This ticket was mentioned in PR #1036 on WordPress/wordpress-develop by sjoerdlinders.


3 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
3 years ago

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

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

  • Keywords reporter-feedback added

#5 in reply to: ↑ 3 @sjoerdlinders
3 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 3 years ago by sjoerdlinders (previous) (diff)

#6 @sjoerdlinders
3 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
3 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
3 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 follow-up: @hellofromTonya
3 years 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
3 years ago

  • Keywords php8 added

#11 @jrf
3 years 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
3 years ago

  • Milestone changed from 5.9 to 6.0

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


2 years ago

#14 @costdev
2 years ago

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

#15 @peterwilsoncc
2 years 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
14 months ago

  • Keywords php80 added; php8 removed
  • Milestone changed from Future Release to 6.3

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


11 months ago

#18 @oglekler
11 months ago

  • Keywords dev-feedback added

This ticket was discussed in the recent bug scrub.

The ticket should be moved into future release if there will be no activity.

But the patch is quite small, and it looks like it needs dev-feedback and backend testing to figure out if this proposed default values are correct.

Props to @Clorith

Last edited 11 months ago by oglekler (previous) (diff)

#19 @SergeyBiryukov
11 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 56128:

HTTP API: Declare a few default parameters in WP_Http_Curl and WP_Http_Streams.

This resolves Undefined array key PHP warnings when trying to access any of these values in WP_Http_Curl::request() or WP_Http_Streams::request():

  • $parsed_args['decompress']
  • $parsed_args['stream']
  • $parsed_args['filename']

Follow-up to [10410], [11236], [13274], [17555], [37428], [42766], [44346].

Props sjoerdlinders, hellofromTonya, jrf, oglekler, Clorith, SergeyBiryukov.
Fixes #52622.

@SergeyBiryukov commented on PR #1036:


11 months ago
#20

Thanks for the PR! Merged in r56128.

#21 in reply to: ↑ 9 @SergeyBiryukov
11 months ago

  • Keywords dev-feedback removed

Replying to hellofromTonya:

Do you agree with the default values?

Thanks for the investigation! These defaults look reasonable to me and are now committed in [56128].

It appears that the WP_Http_Curl and WP_Http_Streams classes are no longer used by WordPress core, so I have created a follow-up ticket to deprecate them: #58705.

Note: See TracTickets for help on using tickets.