WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 months ago

#40044 reviewing enhancement

A little strange logic in get_header_video_url() function

Reported by: Tkama Owned by: SergeyBiryukov
Milestone: 5.4 Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: has-patch
Focuses: Cc:
PR Number:

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

Attachments (1)

40044.diff (546 bytes) - added by dlh 2 months ago.

Download all attachments as: .zip

Change History (7)

#1 @SergeyBiryukov
3 years ago

  • Component changed from Permalinks to Customize

#2 @Tkama
3 years 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 3 years ago by Tkama (previous) (diff)

#3 @dlh
2 months ago

  • Keywords needs-patch added
  • Version changed from 4.7.2 to 4.7

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
2 months ago

#4 @SergeyBiryukov
2 months ago

  • Milestone changed from Awaiting Review to 5.4
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#5 @dlh
2 months ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from 5.4 to Awaiting Review

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.

#6 @dlh
2 months ago

  • Milestone changed from Awaiting Review to 5.4

(Whoops!)

Note: See TracTickets for help on using tickets.