#40044 closed enhancement (fixed)
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: |
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)
Change History (12)
#3
@
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.
#4
@
5 years ago
- Milestone changed from Awaiting Review to 5.4
- Owner set to SergeyBiryukov
- Status changed from new to reviewing
#5
@
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.
#7
@
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
@
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.
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#L1366Why we esc the value of
get_header_video_url()
one more time? It's already escaped...