Remove front page restrictions from new custom header functions

[peterwilsoncc]

Forked from #38639

cc: @bradyvercher, @celloexpressions, @joemcgill
Adding to milestone per original ticket

Original description for this part of the ticket:

Front Page Restriction

I've been looking to incorporate this functionality in the ​Marquee theme and it feels broken if video only loads on the front page. This seems like a restriction that should be left up to the theme and is particularly difficult to workaround if a theme wants video on more than one page.

Browsers should be caching local videos, which should minimize the bandwidth impact a bit. I think we should make the API broadly useful and document best practices instead of trying to enforce onerous restrictions.

[peterwilsoncc]

@bradyvercher following up your request for feedback on the previous ticket.

I am unsure either way, there are forward compatibility concerns with this change so a decision from the component maintainer/lead devs is required.

My forward compatibility concern is that if this changes after a release, themes may be coded in such a way they can't be adapted. This can be mitigated but it's not elegant code.

My inclination is to make this change and allow themes to limit to is_front_page() if they desire. With some encouragement they do so.

[flixos90]

+1 for this.

I think we can leave the is_front_page() check in for a sane default, but that return value should receive a filter so that theme developers who explicitly need the video header in other areas can dig in to adjust it.

A solution that would be even cleaner is to allow an additional argument when registering theme support for custom-header: Something like a has_video_cb could be used, with the default value of is_front_page.

[flixos90]

38738.diff​ implements the theme support approach and sets is_front_page() as the default callback. A function is_header_video_active() is introduced for easy detection.

A theme developer who for instance wants to show the video everywhere would add theme support for custom-header with the argument video-active-callback being either empty or having the value __return_true.

[bradyvercher]

Thanks for the feedback @peterwilsoncc!

@flixos90 is there a functional different between the approach you suggested and adding an argument to the_custom_header_markup() like the one in #38639? It seems to require more code and more effort, so I'd like to understand the benefit.

[flixos90]

@bradyvercher The approach I suggested makes using the header video functionality in the theme a bit easier. You only have to declare the details once when adding theme support, thus it's not necessary to pass a variable to the function. If a theme had multiple areas where it showed the header markup, it would need to pass the variable each time whereas the theme support approach allows to handle it in one central location.
The additional function can also make sense to have available for possible plugins that enhance the header video function in some way.

[joemcgill]

I like the idea behind 38738.diff​ a lot. It does seem like this is something one would want to define when setting up support, rather than worrying about it in templates. I think there might be some confusion though with a function named is_header_video_active() that its purpose would be to determine if the video is set. Maybe something like is_header_video_supported()?

[bradyvercher]

Replying to flixos90:

If a theme had multiple areas where it showed the header markup, it would need to pass the variable each time whereas the theme support approach allows to handle it in one central location.

I think that's an extremely rare edge case, but that does assume you want to use the same behavior each time, so you do lose a little flexibility. In either case, the possibility for overriding the default behavior is there, so I'm cool with either approach.

I can't really think of any other functions that check a template conditional like this that could help influence the naming, but is_header_video_supported() sounds like it would be doing the same thing as current_theme_supports( 'custom-header', 'video' ).

[joemcgill]

Replying to bradyvercher:

I can't really think of any other functions that check a template conditional like this that could help influence the naming, but is_header_video_supported() sounds like it would be doing the same thing as current_theme_supports( 'custom-header', 'video' ).

I don't think the use of a template conditional is as important as the fact that it allows a registered callback at all. For instance, if someone wanted to write their own theme callback that only allowed for video headers on the front-page or a set of custom landing pages, they could register a callback like mytheme_show_video_headers_callback(), which does something like:

function mytheme_show_video_headers_callback() {
    if ( is_front_page() || is_page( 'page-slug' ) ) {
        return true;
    }

    return false; 
}

I agree with you that the naming is a bit clunky, though. I wonder if there is a better place to include the logic from the proposed is_header_video_active() rather than including another function. Perhaps something we could check in has_header_video() depending on context?

[bradyvercher]

I was mainly just trying to think of another similar function to use as inspiration for naming. A registered callback, an argument, or even a filter would all allow for defining a custom callback, the only difference is how they're registered and when they get called.

[flixos90]

Replying to joemcgill:

I agree with you that the naming is a bit clunky, though. I wonder if there is a better place to include the logic from the proposed is_header_video_active() rather than including another function. Perhaps something we could check in has_header_video() depending on context?

I think a dedicated function makes sense. has_header_video() should only check if a header video is set, not whether it should show on the current template. I certainly agree the naming could be improved; though regarding your improvement I also agree with @bradyvercher hthat is_header_video_supported() sounds more like it would check for general support.

[peterwilsoncc]

While a callback is clean, a filter seems more WordPressy.

function is_header_video_active() {
        return apply_filters( 'is_header_video_active', true );
}

Should additional arguments be added in a future release, the logic is built in to WP hooks already.

I'll put together an alternative once I'm across the existing patch.

[peterwilsoncc]

In 38738.2.diff​

• new function is_header_video_active() to determine if video should be shown
• function always returns false if custom-header, video isn't supported by the theme
• new filter is_header_video_active to allow themes and plugins to modify the output
• defaults to allowing video on is_front_page() only.

Removed from original patch:

• custom callback when setting up theme support

Left unanswered: naming things.

[flixos90]

Replying to peterwilsoncc:

• function always returns false if custom-header, video isn't supported by the theme

Good catch, that totally makes sense to me.

While a callback is clean, a filter seems more WordPressy.

Why not go the clean way? :) I think since this feature is theme-related, it should happen through theme support, especially since we already have similar callbacks to add to custom-header support. Maybe we can have both, the theme support, but still keep the filter you proposed (to filter the callback return value) in case a plugin needs to override for a special case.

[peterwilsoncc]

Replying to flixos90:

Why not go the clean way? :) I think since this feature is theme-related, it should happen through theme support, especially since we already have similar callbacks to add to custom-header support.

I like hooks because they're an established way of managing callback in a forward compatible manner.

Given there are other callbacks in the feature, I'm happy to go with a callback.

In 38738.3.diff​:

• reinstate the callback
• maintain the hook
• tests

[bradyvercher]

@peterwilsoncc Unfortunately, calling add_theme_support( 'custom-header' ) doesn't work well in tests since it defines constants that end up poisoning subsequent tests. I tried working around that in #38639 by assigning the custom header args directly to the $_wp_theme_features global, but I'm not sure if there's a better way.

[peterwilsoncc]

Replying to bradyvercher:
Good catch, I'll look over add_theme_support() and see if backing up the global before running the tests will prevent poisoning the remaining tests.

[davidakennedy]

I like the idea of removing the restrictions and putting that in the hands of themers.

If this does happen though, Twenty Seventeen will need to keep things as they are with the video only on the front page because of the current design implementation. So this serves as a reminder to me make those changes if this gets committed. :)

[flixos90]

Replying to davidakennedy:

If this does happen though, Twenty Seventeen will need to keep things as they are with the video only on the front page because of the current design implementation. So this serves as a reminder to me make those changes if this gets committed. :)

I'm not a 100% sure, but it might very well be possible that no changes would be necessary on the Twenty Seventeen end of things, since is_front_page() would still be the default value for the new implementation.

[peterwilsoncc]

In 38738.4.diff​:

• refreshed following [39227]
• backs up theme support global before tests, restores following.

[peterwilsoncc]

I think we have enough consensus to put this in.

I've written to many iterations of this to review, labelling as such.

[flixos90]

The patch looks good to me. In 38738.5.diff​ I cleared up the unit tests a bit:

• only one assertion per test
• restoring the $_wp_theme_features global happens before the assertion so that a failing assertion doesn't pollute further tests

I agree this is good to go in.

[davidakennedy]

@flixos90 Ah, thanks. I'll still keep an eye on things, just in case.

[bradyvercher]

It wasn't readily apparent with the tests in 38738.5.diff​ since they're only testing new video arguments, but they still poisoned subsequent tests. Backing up the global and restoring doesn't fix the issue and shouldn't be necessary.

Any time add_theme_support( 'custom-header' ) is called with any of the following arguments, it ​defines constants that will persist through subsequent tests: header-text, width, height, default-text-color, and default-image.

When custom header support is registered with '__jit' => true, it defines all of the header constants, which prevents any of the values from being updated/tested in subsequent tests.

In 38738.6.diff​ I pulled in and updated the tests I wrote for #38639. I also moved the tests from 38738.5.diff​ over to the new customHeader.php test case and updated those to not call add_theme_support( 'custom-header' ) to prevent header constants from ever being defined.

It doesn't look like the patch keeps binary files, so the video file will need to be ​downloaded from this repo and saved to tests/phpunit/data/images/video.mp4 to make the tests pass.

There are also some changes in _remove_theme_support() to prevent notices from being thrown when removing custom header support.

[flixos90]

Replying to bradyvercher:

It wasn't readily apparent with the tests in 38738.5.diff​ since they're only testing new video arguments, but they still poisoned subsequent tests. Backing up the global and restoring doesn't fix the issue and shouldn't be necessary.

The tests 38738.6.diff​ are looking great to me. Sorry I somehow overlooked the constants being defined. Regarding the global though, I think we should make sure it is reset to its original value afterwards, I think this should be the case in any unit tests where a global is modified as this can also poison subsequent tests.

[bradyvercher]

Replying to flixos90:

Sorry I somehow overlooked the constants being defined. Regarding the global though, I think we should make sure it is reset to its original value afterwards, I think this should be the case in any unit tests where a global is modified as this can also poison subsequent tests.

No worries! The global should be reset in the tearDown() method when it calls remove_theme_support( 'custom-header' );. Here's where that happens in core: ​https://github.com/WordPress/WordPress/blob/8e6753ceadab17e132baf53f52f3e7fa8cc76798/wp-includes/theme.php#L2338

[peterwilsoncc]

There's a bunch of other tests in 38738.6.diff​ for the customiser and other tickets.

Are you able to strip them out so I can focus only on the tests for this ticket prior to the RC tomorrow? I will speed up getting the /src changes in.

I'll happily work through the additional tests post RC.

[bradyvercher]

38738.7.diff​ removes all the new tests, but keeps the test case and general approach for testing header args without poisoning subsequent tests.

[joemcgill]

38738.7.diff​ looks good to me, and I'm starting to come around to the idea of is_header_video_active as the name of the conditional function. The comment on the @param in the docs for the filter in that function should be split into separate lines, but that can be handled in the commit.

The last thing to note is that is_header_video_active() (or whatever we end up with) should be used as the active_callback in the customizer (see: #38778). If this lands first, we can update the patch on that ticket, but if that ticket lands first, we'll want to make the update here.

[peterwilsoncc]

@joemcgill thanks for looking over.

Requires refresh following [39237].

[joemcgill]

38738.8.diff​ refreshes 38738.7.diff​ following [39237] and updates the inline doc for the filter of is_header_video_active() as described in comment #28.

[peterwilsoncc]

In 39240:

Themes: Remove front page restriction from video header functions.

Adds a callback for determining when video headers should be displayed in themes supporting custom headers. By default, video headers are only displayed on the front page of a site.

Theme authors may set a custom callback by passing 'video-active-callback' => 'mytheme_video_active_callback' as an argument. The default callback is is_front_page().

This introduces the new function is_header_video_active() - returns true on pages that should display video headers. The return value can be filtered using the new filter of the same name.

Props flixos90, bradyvercher, peterwilsoncc, joemcgill.
Fixes #38738.

[westonruter]

In 40379:

Customize: Use is_header_video_active() as active_callback for external_header_video control instead of is_front_page().

Use the same active_callback as was supplied previously for the header_video control in [39240] where this instance was missed.

Amends [39240].
Props pratikshrestha.
See #38738.
Fixes #40308.

[swissspidy]

In 40384:

Customize: Use is_header_video_active() as active_callback for external_header_video control instead of is_front_page().

Use the same active_callback as was supplied previously for the header_video control in [39240] where this instance was missed.

Amends [39240].
Props pratikshrestha.
See #38738.
Fixes #40308.

Merges [40379] to the 4.7 branch.