WordPress.org

Make WordPress Core

Opened 21 months ago

Closed 21 months ago

Last modified 13 months ago

#43329 closed enhancement (fixed)

download_url function should return the body in response on error

Reported by: nextendweb Owned by: SergeyBiryukov
Milestone: 5.1 Priority: normal
Severity: normal Version:
Component: Filesystem API Keywords:
Focuses: Cc:
PR Number:

Description

Our non-wordpress.org plugin uses custom url to fetch the update file from our server which needs authentication. So, when the downloading of the update file fails in WordPress we would like to get the response as it is a REST message which contains the exact error message what we should present to the user.

download_url function use wp_safe_remote_get with filename argument which stores the body of the response.

<?php
function download_url( $url, $timeout = 300 ) {
        //WARNING: The file is not automatically deleted, The script must unlink() the file.
        if ( ! $url )
                return new WP_Error('http_no_url', __('Invalid URL Provided.'));

        $url_filename = basename( parse_url( $url, PHP_URL_PATH ) );

        $tmpfname = wp_tempnam( $url_filename );
        if ( ! $tmpfname )
                return new WP_Error('http_no_file', __('Could not create Temporary file.'));

        $response = wp_safe_remote_get( $url, array( 'timeout' => $timeout, 'stream' => true, 'filename' => $tmpfname ) );

        if ( is_wp_error( $response ) ) {
                unlink( $tmpfname );
                return $response;
        }

        if ( 200 != wp_remote_retrieve_response_code( $response ) ){
                unlink( $tmpfname );
                return new WP_Error( 'http_404', trim( wp_remote_retrieve_response_message( $response ) ) );
        }

        $content_md5 = wp_remote_retrieve_header( $response, 'content-md5' );
        if ( $content_md5 ) {
                $md5_check = verify_file_md5( $tmpfname, $content_md5 );
                if ( is_wp_error( $md5_check ) ) {
                        unlink( $tmpfname );
                        return $md5_check;
                }
        }

        return $tmpfname;
}

If the wp_safe_remote_get fails we are unable to access to the response body as the $tmpfname file is unlinked and does not read into the response object.

<?php
        if ( is_wp_error( $response ) ) {
                unlink( $tmpfname );
                return $response;
        }

        if ( 200 != wp_remote_retrieve_response_code( $response ) ){
                unlink( $tmpfname );
                return new WP_Error( 'http_404', trim( wp_remote_retrieve_response_message( $response ) ) );
        }

It would be better if WordPress could read the content of the temporary file and parse it as a normal response body in the previous two if statement.

Attachments (2)

43329.diff (2.6 KB) - added by soulseekah 21 months ago.
with tests
43329.2.diff (2.6 KB) - added by soulseekah 21 months ago.
typo in tests filter cleanup section

Download all attachments as: .zip

Change History (10)

@soulseekah
21 months ago

with tests

#1 @SergeyBiryukov
21 months ago

  • Component changed from General to HTTP API
  • Milestone changed from Awaiting Review to 5.0

#2 @soulseekah
21 months ago

This patch tucks up to 1kb (altered by filter) into the data of the error, so data['code'] and data['body'] will contain the response code and up to 1km of the body.

Useful for debugging purposes.

Special thanks to @campusboy1987 @mihdan @SergeyBiryukov and others in the live coding stream of this ticket.

Godspeed.

Last edited 21 months ago by SergeyBiryukov (previous) (diff)

#3 @nextendweb
21 months ago

I was able to find the solution for my issue without this modification, so this feature is not required for me. But I'm think it makes the HTTP api better.

My solution was:

<?php
    add_filter('upgrader_pre_download', 'n_upgrader_pre_download', 10, 3);

    n_upgrader_pre_download($reply, $package, $upgrader) {
        $needle = my_url_to_match();
        if (substr($package, 0, strlen($needle)) == $needle) {
            add_filter('http_response', 'n_http_response', 10, 3);
        }

        return $reply;
    }

    function n_http_response($response, $r, $url) {

        $needle = my_url_to_match();
        if (substr($url, 0, strlen($needle)) == $needle && (200 != wp_remote_retrieve_response_code($response) || is_wp_error($response))) {

            if (isset($response['filename']) && file_exists($response['filename'])) {

                $body = @file_get_contents($response['filename']);
                // Here I have the body of the error response

            }
        }

        return $response;
    }

@soulseekah
21 months ago

typo in tests filter cleanup section

#4 @johnbillion
21 months ago

  • Version 4.9.4 deleted

This looks like a good idea. What's the purpose of trimming the response (to 1kb by default)?

#5 @soulseekah
21 months ago

We don't want to risk loading up a potentially large file into memory. 1kb was chosen as it would offer enough hints as to what is going wrong (if anything).

Imagine a server spewing out 100MB or random data with a 418 I'm a teapot code. Downloading such a URL will result in an memory limit being hit in PHP probably.

Thus we just read as much as is useful into the error body.

Any other ideas?

Ideally, we'd want the file to stick around just in case and then be unlinked. But the only solution that seems viable is hooking into shutdown, which I think is over engineering.

#6 @SergeyBiryukov
21 months ago

  • Component changed from HTTP API to Filesystem API

#7 @SergeyBiryukov
21 months ago

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

In 42773:

Filesystem API: Allow download_url() to return the response code and body on error as an additional WP_Error object data.

The error response body size is limited to 1 KB by default to avoid taking up too much memory. The size can be increased using download_url_error_max_body_size filter.

Props soulseekah, campusboy1987, mihdan, SergeyBiryukov.
Fixes #43329.

#8 @johnbillion
13 months ago

  • Milestone changed from 5.0 to 5.1
Note: See TracTickets for help on using tickets.