download_url() always returns error_code 'http_404' for any non-200 responses

[hinnerk]

Expected:
Return WP_Error code "http_406" if a 406 occurs while downloading the URL.
Return WP_Error code "http_500" if a 500 occurs while downloading the URL.
...

Actually doing it wrong:
download_url() always returns WP_Error code "http_404" on any non-200 status.

object(WP_Error)#11262 (3) {
  ["errors"]=>
  array(1) {
    ["http_404"]=>
    array(1) {
      [0]=>
      string(14) "Not Acceptable"
    }
  }
  ["error_data"]=>
  array(1) {
    ["http_404"]=>
    array(2) {
      ["code"]=>
      int(406)
      ["body"]=>
      string(0) ""
    }
  }
  ["additional_data":protected]=>
  array(0) {
  }
}

see https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/file.php#L1214

<?php
if ( 200 !== $response_code ) {
    // [...]
    return new WP_Error( 'http_404', trim( wp_remote_retrieve_response_message( $response ) ), $data );
}

[vedantgandhi28]

Hi @hinnerk I have added a patch that addresses the above issue. Can you have a look at it once and review it? Thank you

[hinnerk]

Hi @vedantgandhi28 Thanks a lot for the patch, looks great, straightforward and correct!

Trac Ticket - https://core.trac.wordpress.org/ticket/60564

download_url() returns a WP_Error with the code http_404 for every non-200 response. A 403, 406 or 500 is indistinguishable from a real 404 unless the caller digs the status code out of get_error_data().

This PR names the error after the actual status code (http_403, http_500, ...), the same convention Site Health already uses for its own HTTP errors ('http_' . wp_remote_retrieve_response_code() in class-wp-site-health.php). It builds on #8187 by @Vedant-Gandhi and adds:

• a fallback to the historical http_404 when the response carries no status code at all (wp_remote_retrieve_response_code() returns '' for a pre_http_request short-circuit without a response array — the one-liner would have produced http_);
• a @since note in the docblock;
• unit tests: the existing 418 test now also asserts the error code, plus a data provider over 403/404/406/500/503 and a test for the no-status-code fallback. All fail on trunk except the two http_404 cases.

Backward compatibility: only callers comparing get_error_code() against the literal 'http_404' are affected, and for those the old code was wrong for every status but 404. Nothing in core does this; the error message and get_error_data() (code, body) are unchanged. Probably worth a line in the 7.2 misc dev note.

[hinnerk]

Picking this up again, as the one-line patch has been sitting without tests for a while.

I have opened ​PR 13602 which builds on @vedantgandhi28's patch and adds what a commit would need:

• The error code now reflects the actual status code (http_403, http_500, ...). This is the same convention Site Health already uses: 'http_' . wp_remote_retrieve_response_code() in class-wp-site-health.php.
• A response without a status code keeps the historical http_404. wp_remote_retrieve_response_code() returns '' when a pre_http_request filter short-circuits with a response that has no response array, and the original patch would have produced the code http_ in that case.
• A @since note in the docblock.
• Unit tests: the existing test_download_url_non_200_response_code() (418) now also asserts the error code, and there is a new data provider over 403/404/406/500/503 plus a test for the no-status-code fallback. All of them fail on trunk except the two http_404 cases.

On backward compatibility: the error message and get_error_data() (code, body) are unchanged. Only code that compares get_error_code() against the literal 'http_404' is affected, and for that code the old behaviour was wrong for every status except an actual 404. Nothing in core branches on it (the upgrader, media_sideload_image() and the REST attachments controller all pass the error through). I have added needs-dev-note so it gets a line in the misc dev note.

Could this get a milestone? It is a small HTTP API fix with tests, and it makes error handling around download_url() in plugins a lot less awkward.