Make WordPress Core

Changeset 51626


Ignore:
Timestamp:
08/17/2021 12:14:20 AM (3 years ago)
Author:
SergeyBiryukov
Message:

Code Modernization: Check the return type of parse_url() in download_url().

As per the PHP manual:

If the component parameter is omitted, an associative array is returned.
If the component parameter is specified, parse_url() returns a string (or an int, in the case of PHP_URL_PORT) instead of an array. If the requested component doesn't exist within the given URL, null will be returned.

Reference: PHP Manual: parse_url(): Return Values

This commit adds three unit tests for download_url():

  • The first test is "girl-scouting" to make sure that the code up to the point where the error is expected is tested.
  • The second test exposed a PHP 8.1 basename(): Passing null to parameter #1 ($path) of type string is deprecated error due to the call to parse_url() returning null when the component requested does not exist in the passed URL.
  • The output of the call to parse_url() stored in the $url_path variable is used in more places in the function logic. The third test exposes a second PHP 8.1 deprecation notice, this time for substr(): Passing null to parameter #1 ($string) of type string is deprecated.

This commit also removes duplicate parse_url() calls. Neither $url nor $url_filename are changed between when they are first received/defined and when they are re-used, so there is no need to repeat the function calls.

Follow-up to [51606], [51622].

Props jrf, hellofromTonya, SergeyBiryukov.
See #53635.

Location:
trunk
Files:
2 edited

Legend:

Unmodified
Added
Removed
  • trunk/src/wp-admin/includes/file.php

    r51300 r51626  
    11271127    }
    11281128
    1129     $url_filename = basename( parse_url( $url, PHP_URL_PATH ) );
     1129    $url_path     = parse_url( $url, PHP_URL_PATH );
     1130    $url_filename = '';
     1131    if ( is_string( $url_path ) && '' !== $url_path ) {
     1132        $url_filename = basename( $url_path );
     1133    }
    11301134
    11311135    $tmpfname = wp_tempnam( $url_filename );
     
    12131217
    12141218            $signature_url = false;
    1215             $url_path      = parse_url( $url, PHP_URL_PATH );
    1216 
    1217             if ( '.zip' === substr( $url_path, -4 ) || '.tar.gz' === substr( $url_path, -7 ) ) {
     1219
     1220            if ( is_string( $url_path ) && ( '.zip' === substr( $url_path, -4 ) || '.tar.gz' === substr( $url_path, -7 ) ) ) {
    12181221                $signature_url = str_replace( $url_path, $url_path . '.sig', $url );
    12191222            }
     
    12441247
    12451248        // Perform the checks.
    1246         $signature_verification = verify_file_signature( $tmpfname, $signature, basename( parse_url( $url, PHP_URL_PATH ) ) );
     1249        $signature_verification = verify_file_signature( $tmpfname, $signature, $url_filename );
    12471250    }
    12481251
  • trunk/tests/phpunit/tests/admin/includesFile.php

    r48937 r51626  
    7878        return 5;
    7979    }
     80
     81    /**
     82     * Verify that a WP_Error object is returned when invalid input is passed as the `$url` parameter.
     83     *
     84     * @covers ::download_url
     85     * @dataProvider data_download_url_empty_url
     86     *
     87     * @param mixed $url Input URL.
     88     */
     89    public function test_download_url_empty_url( $url ) {
     90        $error = download_url( $url );
     91        $this->assertWPError( $error );
     92        $this->assertSame( 'http_no_url', $error->get_error_code() );
     93        $this->assertSame( 'Invalid URL Provided.', $error->get_error_message() );
     94    }
     95
     96    /**
     97     * Data provider.
     98     *
     99     * @return array
     100     */
     101    public function data_download_url_empty_url() {
     102        return array(
     103            'null'         => array( null ),
     104            'false'        => array( false ),
     105            'integer 0'    => array( 0 ),
     106            'empty string' => array( '' ),
     107            'string 0'     => array( '0' ),
     108        );
     109    }
     110
     111    /**
     112     * Test that PHP 8.1 "passing null to non-nullable" deprecation notice
     113     * is not thrown when the `$url` does not have a path component.
     114     *
     115     * @ticket 53635
     116     * @covers ::download_url
     117     */
     118    public function test_download_url_no_warning_for_url_without_path() {
     119        $result = download_url( 'https://example.com' );
     120
     121        $this->assertIsString( $result );
     122        $this->assertNotEmpty( $result ); // File path will be generated, but will never be empty.
     123    }
     124
     125    /**
     126     * Test that PHP 8.1 "passing null to non-nullable" deprecation notice
     127     * is not thrown when the `$url` does not have a path component,
     128     * and signature verification via a local file is requested.
     129     *
     130     * @ticket 53635
     131     * @covers ::download_url
     132     */
     133    public function test_download_url_no_warning_for_url_without_path_with_signature_verification() {
     134        add_filter(
     135            'wp_signature_hosts',
     136            static function( $urls ) {
     137                $urls[] = 'example.com';
     138                return $urls;
     139            }
     140        );
     141        $error = download_url( 'https://example.com', 300, true );
     142
     143        /*
     144         * Note: This test is not testing the signature verification itself.
     145         * There is no signature available for the domain used in the test,
     146         * which is why an error is expected and that's fine.
     147         * The point of the test is to verify that the call to `verify_file_signature()`
     148         * is actually reached and that no PHP deprecation notice is thrown
     149         * before this point.
     150         */
     151        $this->assertWPError( $error );
     152        $this->assertSame( 'signature_verification_no_signature', $error->get_error_code() );
     153    }
    80154}
Note: See TracChangeset for help on using the changeset viewer.