WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years 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 2 years ago.
Adding a has_header_video filter.
39512.2.diff (786 bytes) - added by celloexpressions 2 years ago.
Filter get_header_video_url().

Download all attachments as: .zip

Change History (16)

#1 @celloexpressions
2 years 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
2 years ago

Adding a has_header_video filter.

#2 @sanket.parmar
2 years ago

  • Keywords has-patch added; needs-patch removed

#3 @wpweaver
2 years 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
2 years ago

Maybe get_header_video_url() should be filterable instead?

#5 follow-up: @SergeyBiryukov
2 years 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
2 years 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
2 years 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.

Version 0, edited 2 years ago by wpweaver (next)

@celloexpressions
2 years ago

Filter get_header_video_url().

#8 @celloexpressions
2 years 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
2 years 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
2 years 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.


2 years ago

#12 @celloexpressions
2 years 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
2 years 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
2 years 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.