[wp-trac] [WordPress Trac] #52622: Fix PHP Warning on PHP7.2 in class-wp-http-curl.php

[WordPress Trac]

#52622: Fix PHP Warning on PHP7.2 in class-wp-http-curl.php
-------------------------------------+-------------------------------------
 Reporter:  sjoerdlinders            |       Owner:  SergeyBiryukov
     Type:  defect (bug)             |      Status:  reviewing
 Priority:  normal                   |   Milestone:  5.9
Component:  HTTP API                 |     Version:  5.6.2
 Severity:  normal                   |  Resolution:
 Keywords:  has-patch needs-unit-    |     Focuses:  administration,
  tests                              |  coding-standards
-------------------------------------+-------------------------------------
Changes (by hellofromTonya):

 * keywords:  has-patch needs-testing needs-unit-tests => has-patch needs-
     unit-tests


Comment:

 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
 <?php
         'decompress'  => false,
         'stream'      => false,
         'filename'    => null,
 }}}

 If yes, I'd advocate this get committed to ship with 5.9. Do you agree?

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/52622#comment:9>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform