WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#37097 closed defect (bug) (fixed)

Requests: Return array from WP_Http::request for compatibility

Reported by: rmccue Owned by: rmccue
Milestone: 4.6 Priority: highest omg bbq
Severity: normal Version: 4.6
Component: HTTP API Keywords: has-patch
Focuses: Cc:
PR Number:

Description

As we noticed in #33055, a lot of people are checking is_array directly on the response from this function. This means that our compatibility shim with ArrayAccess breaks.

Let's switch this back to an array, but keep the new object around. I want to encourage the WP_HTTP_Response object as much as possible.

Attached patch changes format back to an array, now with a new http_response key containing the WP_HTTP_Requests_Response object. It also removes the ArrayAccess implementation as we no longer need it.

Attachments (2)

37097.diff (4.0 KB) - added by rmccue 3 years ago.
Initial patch to switch back to array
37097.2.diff (4.7 KB) - added by rmccue 3 years ago.
Remove @see and add unit test

Download all attachments as: .zip

Change History (8)

@rmccue
3 years ago

Initial patch to switch back to array

#1 follow-up: @Shelob9
3 years ago

I see that on L358 you are adding $requests_response to the array that is returned. I wonder if it makes sense to add a property to the class for last request's response and put it there. We could use that like we use $wpdb->last_result and similar properties like last_error in that class.

#2 in reply to: ↑ 1 @rmccue
3 years ago

Replying to Shelob9:

I wonder if it makes sense to add a property to the class for last request's response and put it there.

I'm not sure what benefit this would provide over the http_api_debug hook? wpdb has to keep stuff around for technical reasons, I'd prefer to avoid persisting state where we don't need to.

#3 @jamescollins
3 years ago

Running latest trunk (r37720), I was getting the following when visiting wp-admin/update-core.php?force-check=1 :

Warning: Invalid argument supplied for foreach() in wp-includes/update.php on line 315
Warning: Invalid argument supplied for foreach() in wp-includes/update.php on line 325
Notice: Undefined index: translations in wp-includes/update.php on line 332

After applying 37097.diff, the errors no longer occur.

#4 @ocean90
3 years ago

  • Priority changed from normal to highest omg bbq

#5 @ocean90
3 years ago

  • Summary changed from Return array from WP_Http::request for compatibility to Requests: Return array from WP_Http::request for compatibility

@rmccue
3 years ago

Remove @see and add unit test

#6 @rmccue
3 years ago

  • Owner set to rmccue
  • Resolution set to fixed
  • Status changed from new to closed

In 37989:

HTTP API: Switch back to returning an array.

The array-compatibility object we started returning in r37428 unfortunately isn't enough like an array. In particular, is_array() checks fail, despite the object implementing ArrayAccess. Mea culpa.

This moves the WP_HTTP_Response object to a new http_response key in the array, and changes the value back to an actual array.

Fixes #37097.
See #33055.

Note: See TracTickets for help on using tickets.