WordPress.org

Make WordPress Core

Opened 15 months ago

Closed 3 weeks ago

#51736 closed task (blessed) (fixed)

wp_remote_retrieve_header returns an Array for multiple headers

Reported by: robtarr Owned by: johnjamesjacoby
Milestone: 5.9 Priority: normal
Severity: normal Version: 2.7
Component: HTTP API Keywords: has-patch
Focuses: docs Cc:

Description

When calling wp_remote_retrieve_header and specifying a header that has multiple entries in the response this method returns an array. The docs, however, indicate that it should return a string.

I think that this should probably throw an error if the wrong type comes back.

Attachments (2)

51736.diff (799 bytes) - added by felipeelia 5 weeks ago.
The diff covering only the doc change.
51736.2.diff (821 bytes) - added by costdev 3 weeks ago.

Download all attachments as: .zip

Change History (10)

#1 @johnjamesjacoby
4 months ago

  • Focuses docs added
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.9
  • Owner set to johnjamesjacoby
  • Status changed from new to assigned
  • Type changed from enhancement to task (blessed)
  • Version set to 2.7

Hey @robtarr 👋 Good catch!

I'll take care of this, related to #53399.

#2 @hellofromTonya
6 weeks ago

  • Keywords dev-feedback removed

specifying a header that has multiple entries in the response this method returns an array

Are you meaning that the $response['headers'][ $header ] could have an array of header entries for a single header name?

If that's possible, then the question is: Should this function always return only a string data type, i.e. a single header entry of string type?

  • If yes, then additional checks are needed and potentially an error would be thrown when it's not a string.
  • If no, then the code consuming wp_remote_retrieve_header() needs auditing to determine if additional checks are required to properly handle either a string or array. Looking at the code, there are instances where the returned value is passed to a native PHP function which will result in an error or deprecation on PHP 8+.

@johnjamesjacoby is this something on your list before 5.9 Beta 3?

#3 @johnjamesjacoby
6 weeks ago

Are you meaning that the $response['headers'][ $header ] could have an array of header entries for a single header name?

Correct! "could" being the operative word, unfortunately.

It may be a string, but it could also be an array. It all depends on what gets sent back.

is this something on your list before 5.9 Beta 3?

I see this strictly as a documentation change, without additional checks, errors, or auditing – at least for the here and now. As such, anytime during the 5.9 cycle where docs changes are allowed seems OK to me.

#4 @felipeelia
5 weeks ago

Is adding a new parameter to specify the return something you'd consider @johnjamesjacoby? Let's say $type that could be 'raw', 'array', or 'string'.

So, in src/wp-includes/comment.php (by line 2787) we would have the following (returning the first value of an hipotical array returned):

preg_match( '#(image|audio|video|model)/#is', wp_remote_retrieve_header( $response, 'content-type', 'string' )

and in tests/phpunit/tests/http/base.php (test_multiple_location_headers()) this assertion would be still valid:

$this->assertIsArray( wp_remote_retrieve_header( $res, 'location' ) );

I can help crafting a diff/PR if that is helpful. Thanks!

Last edited 5 weeks ago by felipeelia (previous) (diff)

@felipeelia
5 weeks ago

The diff covering only the doc change.

#5 @felipeelia
5 weeks ago

  • Keywords has-patch added; needs-patch removed

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


3 weeks ago

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


3 weeks ago

@costdev
3 weeks ago

#8 @davidbaumwald
3 weeks ago

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

In 52441:

HTTP API: Improve docs for wp_remote_retrieve_header function return value.

This change updates the @return docs for wp_remote_retrieve_header to specify that an array can also be returned if the requested header key has multiple values.

Props robtarr, johnjamesjacoby, felipeelia, hellofromTonya, costdev.
Fixes #51736.

Note: See TracTickets for help on using tickets.