WordPress.org

Make WordPress Core

Opened 10 months ago

Last modified 10 months ago

#40044 new enhancement

A little strange logic in get_header_video_url() function

Reported by: Tkama Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.7.2
Component: Customize Keywords:
Focuses: Cc:

Description

Lets see func code here: https://developer.wordpress.org/reference/functions/get_header_video_url/

function get_header_video_url() {
    $id = absint( get_theme_mod( 'header_video' ) );
    $url = esc_url( get_theme_mod( 'external_header_video' ) );
 
    if ( ! $id && ! $url ) {
        return false;
    }
 
    if ( $id ) {
        // Get the file URL from the attachment ID.
        $url = wp_get_attachment_url( $id );
    }
 
    return esc_url_raw( set_url_scheme( $url ) );
}
// at the bigining
$url = esc_url( get_theme_mod( 'external_header_video' ) );

// at the end
return esc_url_raw( set_url_scheme( $url ) );

May be better to do esc only just before return (variant 1). Or if we dont need it for wp_get_attachment_url() do esc_url_raw() only for it (variant 2)...

And also it seems better to improve all logic, in order it become more clear an faster in some cases...

variant 1:

function get_header_video_url() {
    $id = get_theme_mod( 'header_video' ); // absint( $id ) - no need
 
    if ( $id ) {
        // Get the file URL from the attachment ID.
        $url = wp_get_attachment_url( $id );
    }
    else {
        $url = get_theme_mod( 'external_header_video' );
    }

    if ( ! $url ) {
        return false;
    }
 
    return esc_url( $url );
}

variant 2:

function get_header_video_url() {
    $id = get_theme_mod( 'header_video' ); // absint( $id ) - no need
 
    if ( $id ) {
        // Get the file URL from the attachment ID.
        $url = esc_url_raw( wp_get_attachment_url( $id ) );
    }
    else {
        $url = esc_url( get_theme_mod( 'external_header_video' ) );
    }

    if ( ! $url ) {
        return false;
    }
 
    return $url;
}

Change History (2)

#1 @SergeyBiryukov
10 months ago

  • Component changed from Permalinks to Customize

#2 @Tkama
10 months ago

Oh, now I see that the code is change a littele in 4.7.3 https://core.trac.wordpress.org/browser/trunk/src/wp-includes/theme.php#L1328

But the logic still strange...

And see next to the function the_header_video_url() code: https://core.trac.wordpress.org/browser/trunk/src//wp-includes/theme.php#L1366

function the_header_video_url() {
        $video = get_header_video_url();
        if ( $video ) {
                echo esc_url( $video );
        }
}

Why we esc the value of get_header_video_url() one more time? It's already escaped...

Last edited 10 months ago by Tkama (previous) (diff)
Note: See TracTickets for help on using tickets.