WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 6 months ago

#39512 closed defect (bug) (fixed)

has_header_video() should be filtered

Reported by: wpweaver Owned by: SergeyBiryukov
Milestone: 4.7.3 Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: has-patch fixed-major
Focuses: Cc:

Description

function has_header_video() is totally dependent on having a video URL set by the Header Media menu.

The returned value of the function should be filtered. It is easily possible to use 'header_video_settings' to replace the 'videoUrl' setting. But the fact that has_header_video() is used various places by the header video code, it makes it difficult to supply an alternate video if there is no get_theme_mod( 'external_header_video' ) set.

It is perfectly reasonable to want to use the Header Video script with 'header_video_settings', but having has_video_header() fail makes it really hard to do. If the result was filtered, then the plugin/theme could provide an alternative videoURL even when the 'external_header_video' has not been set.

All of the other critical elements have filters, but not having one for has_header_video() makes all of the other filters somewhat incomplete. The filter should obviously be named 'has_header_video'.

Attachments (2)

39512.diff (640 bytes) - added by sanket.parmar 7 months ago.
Adding a has_header_video filter.
39512.2.diff (786 bytes) - added by celloexpressions 7 months ago.
Filter get_header_video_url().

Download all attachments as: .zip

Change History (16)

#1 @celloexpressions
7 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.7.2

Makes sense; moving to 4.7.2 for investigation since this in introduced in 4.7. Are you able to create a patch to add the filter @wpweaver?

@sanket.parmar
7 months ago

Adding a has_header_video filter.

#2 @sanket.parmar
7 months ago

  • Keywords has-patch added; needs-patch removed

#3 @wpweaver
7 months ago

Sorry, the fix would be totally trivial, but I'm not really familiar enough with the system to provide patches.

If I understand correctly, the has-patch tag means someone else did it. Thank you.

#4 @SergeyBiryukov
7 months ago

Maybe get_header_video_url() should be filterable instead?

#5 follow-up: @SergeyBiryukov
7 months ago

But the fact that has_header_video() is used various places by the header video code, it makes it difficult to supply an alternate video if there is no get_theme_mod( 'external_header_video' ) set.

FWIW, get_theme_mod() is filterable, so it should be possible to set an alternative video URL using theme_mod_external_header_video filter.

#6 in reply to: ↑ 5 @celloexpressions
7 months ago

  • Keywords needs-patch added; has-patch removed

Replying to SergeyBiryukov:

But the fact that has_header_video() is used various places by the header video code, it makes it difficult to supply an alternate video if there is no get_theme_mod( 'external_header_video' ) set.

FWIW, get_theme_mod() is filterable, so it should be possible to set an alternative video URL using theme_mod_external_header_video filter.

While it's possible to filter from the get_theme_mod, I think it makes sense to introduce another filter here. However, I actually agree that a filter on get_header_video_url() would be better than on has_header_video, since that offers the most flexibility.

#7 @wpweaver
7 months ago

I only read code as deep as has_header_video. Filtering get_header_video_url could work, but the logic would be a bit more complicated. The current source of the url is either an internal attachment ID or a url to an external link. Currently, either one of these could generate the ultimate url, but the filter needs to be able to provide a url even if the theme_mod values are null.

The logic of just where to apply the filter, and on which elements is fairly non-trivial in get_header_video_url, but certainly solvable. How to do it in has_header_video is simple.

But putting the filter in get_header_video_url would also make the_header_video_url work as expected, so the filter really does belong on get_header_video_url, but it will require new logic for the attachment vs external video source to make a filter work.

Maybe like this?

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 );
	}
	$url = apply_filters('get_header_video_url', $url);
	if ( ! $url )
		return false;
	return esc_url_raw( set_url_scheme( $url ) );
}
Last edited 7 months ago by wpweaver (previous) (diff)

@celloexpressions
7 months ago

Filter get_header_video_url().

#8 @celloexpressions
7 months ago

  • Keywords has-patch added; needs-patch removed

I think rearranging the conditional placement after the filter should work. Does 39512.2.diff look right?

#9 @SergeyBiryukov
7 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 40045:

Customize: Introduce get_header_video_url filter for the return value of get_header_video_url().

Props sanket.parmar, celloexpressions.
Fixes #39512.

#10 @SergeyBiryukov
7 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.7.3 consideration.

I used @since 4.8.0 in [40045], just in case it doesn't make it into 4.7.3 (we haven't had enhancements in minor releases since 3.0 I think, but that's going to change now).

If it does make it into 4.7.3, we can change the @since entry in trunk as well.

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


7 months ago

#12 @celloexpressions
6 months ago

This seems like an obvious 4.7.x candidate to me since it adds a filter on a function added in 4.7 that should have been filtered from the beginning.

#13 @dd32
6 months ago

In 40086:

Customize: Update the introduced version in the docs for the get_header_video_url filter to 4.7.3.

See #39512.

#14 @dd32
6 months ago

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

In 40087:

Customize: Introduce get_header_video_url filter for the return value of get_header_video_url().

Props sanket.parmar, celloexpressions, SergeyBiryukov.
Merges [40045], [40086] to the 4.7 branch.
Fixes #39512.

Note: See TracTickets for help on using tickets.