WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 months ago

#25747 closed defect (bug) (fixed)

The `http_api_debug` hook isn't called for all HTTP requests

Reported by: johnbillion Owned by: johnbillion
Milestone: 5.3 Priority: normal
Severity: minor Version: 2.7
Component: HTTP API Keywords: has-patch
Focuses: Cc:
PR Number:

Description (last modified by johnbillion)

In WP_Http::_dispatch_request(), the http_api_debug action isn't called if there are no available transports for the request. This means we miss a situation that is ripe for debugging.

Edit: There's more. See my comments below.

Attachments (3)

25747.diff (1.1 KB) - added by johnbillion 6 years ago.
25747.2.diff (4.7 KB) - added by johnbillion 5 years ago.
25747.3.diff (1.8 KB) - added by johnbillion 6 months ago.

Download all attachments as: .zip

Change History (11)

@johnbillion
6 years ago

#1 @johnbillion
6 years ago

25747.diff (best viewed without whitespace changes) alters _dispatch_request() so it doesn't return the WP_Error immediately. It then gets passed to the http_api_debug hook as expected.

Anything that's hooking into this hook will need to ensure it handles boolean false as the $class parameter, as well as a string.

#2 @johnbillion
6 years ago

  • Keywords has-patch added
  • Version set to 3.2

#3 @johnbillion
6 years ago

  • Description modified (diff)
  • Keywords needs-patch added; has-patch removed
  • Summary changed from The `http_api_debug` hook isn't called when there are no available transports to The `http_api_debug` hook isn't called for all HTTP requests

There are actually several places where an HTTP request's return value doesn't trigger the http_api_debug action. None of the following responses trigger it:

  • Anything short-circuiting requests with the pre_http_request filter (here).
  • Invalid URLs (here).
  • Blocked requests (here).
  • Unwritable directories when streaming (here).

We might need a little refactoring here, or a helper method to trigger this action.

#4 @wonderboymusic
5 years ago

  • Milestone changed from Awaiting Review to Future Release

Bump

@johnbillion
5 years ago

#5 @johnbillion
5 years ago

  • Keywords needs-testing needs-unit-tests has-patch added; needs-patch removed

25747.2.diff is a partial patch that I've had sitting around for ages. Probably needs some more work. And tests.

#8 @johnbillion
6 months ago

  • Keywords needs-patch added; needs-testing has-patch removed
  • Milestone set to Future Release
  • Owner set to johnbillion
  • Status changed from new to accepted
  • Version changed from 3.2 to 2.7

@johnbillion
6 months ago

#9 @johnbillion
6 months ago

  • Keywords has-patch added; needs-unit-tests needs-patch removed
  • Milestone changed from Future Release to 5.3

25747.3.diff introduces a much simpler patch than prior ones. It calls the http_api_debug hook on all erroneous responses. There's no need to call the hook for errors being returned by pre_http_request because it's already catchable via that filter.

In addition, I've changed the error code for the error returned when requests are blocked so that loggers, debuggers, etc can differentiate between an unexpected error and an expected one.

#10 @johnbillion
5 months ago

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

In 45504:

HTTP API: Ensure the http_api_debug hook is fired for all responses.

This means that logging and debugging functionality can access the error code and error message for erroneous responses under all conditions. The hook is not fired when a request is short-circuited with the pre_http_request filter, because this filter itself can be used in that situation.

Fixes #25747

Note: See TracTickets for help on using tickets.