A little strange logic in get_header_video_url() function

[Tkama]

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;
}

[Tkama]

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...

[dlh]

It does look like the external_header_video theme mod needs to be retrieved only when there's no $id.

The use of esc_url() on the raw theme mod also seems premature since it's not known whether the value will be used for display, although there could be some other reason it's present.

I would say escaping on output in the_header_video_url() is correct, though, since that is for-sure for display, and there's no guarantee that the value will have been escaped beforehand.

[dlh]

40044.diff​ attempts to fix the unnecessary call to get_theme_mod() when there's a header_video ID. It also removes the early escaping on the value for the reason mentioned in comment:3, but, again, that might be a misunderstanding.

[dlh]

(Whoops!)

[davidbaumwald]

@SergeyBiryukov The latest patch still applies cleanly. Are you still reviewing this one for inclusion in 5.4?

[davidbaumwald]

This ticket still needs a review, and with 5.4 Beta 1 landing tomorrow, this is being moved to Future Release. If any maintainer or committer feels this can be included in 5.4 or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

[SergeyBiryukov]

In 47267:

Customize: Avoid unnecessary get_theme_mod() call and premature escaping in get_header_video_url().

The result is still escaped with esc_url_raw() for retrieval, and with esc_url() for display in the_header_video_url().

Props dlh, Tkama.
Fixes #40044.