Make WordPress Core

Opened 8 years ago

Closed 5 years ago

Last modified 5 years ago

#40044 closed enhancement (fixed)

A little strange logic in get_header_video_url() function

Reported by: tkama's profile Tkama Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.4 Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: has-patch
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;
}

Attachments (1)

40044.diff (546 bytes) - added by dlh 5 years ago.

Download all attachments as: .zip

Change History (12)

#1 @SergeyBiryukov
8 years ago

  • Component changed from Permalinks to Customize

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

#3 @dlh
5 years 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
5 years ago

#4 @SergeyBiryukov
5 years ago

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

#5 @dlh
5 years 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
5 years ago

  • Milestone changed from Awaiting Review to 5.4

(Whoops!)

#7 @davidbaumwald
5 years ago

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

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


5 years ago

#9 @davidbaumwald
5 years ago

  • Milestone changed from 5.4 to Future Release

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.

#10 @SergeyBiryukov
5 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

#11 @SergeyBiryukov
5 years ago

  • Milestone changed from Future Release to 5.4
Note: See TracTickets for help on using tickets.