Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#38738 closed defect (bug) (fixed)

Remove front page restrictions from new custom header functions

Reported by: peterwilsoncc's profile peterwilsoncc Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: Themes Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

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.

Attachments (8)

38738.diff (1.5 KB) - added by flixos90 7 years ago.
38738.2.diff (1.6 KB) - added by peterwilsoncc 7 years ago.
38738.3.diff (3.7 KB) - added by peterwilsoncc 7 years ago.
38738.4.diff (4.0 KB) - added by peterwilsoncc 7 years ago.
38738.5.diff (6.0 KB) - added by flixos90 7 years ago.
38738.6.diff (14.5 KB) - added by bradyvercher 7 years ago.
38738.7.diff (5.9 KB) - added by bradyvercher 7 years ago.
38738.8.diff (3.6 KB) - added by joemcgill 7 years ago.

Download all attachments as: .zip

Change History (42)

#1 @peterwilsoncc
7 years ago

  • Keywords needs-patch added

@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.

#2 @flixos90
7 years ago

+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
7 years ago

#3 @flixos90
7 years ago

  • Keywords has-patch added; needs-patch removed

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.

#4 @bradyvercher
7 years ago

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.

#5 @flixos90
7 years ago

@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.

#6 @joemcgill
7 years ago

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()?

#7 @bradyvercher
7 years ago

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' ).

#8 follow-up: @joemcgill
7 years ago

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?

Version 0, edited 7 years ago by joemcgill (next)

#9 @bradyvercher
7 years ago

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.

#10 in reply to: ↑ 8 @flixos90
7 years ago

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.

#11 @peterwilsoncc
7 years ago

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.

Last edited 7 years ago by peterwilsoncc (previous) (diff)

#12 follow-up: @peterwilsoncc
7 years ago

  • Keywords needs-unit-tests added

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.

#13 in reply to: ↑ 12 @flixos90
7 years ago

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.

#14 @peterwilsoncc
7 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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

#15 follow-up: @bradyvercher
7 years ago

@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.

#16 in reply to: ↑ 15 @peterwilsoncc
7 years ago

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.

#17 follow-up: @davidakennedy
7 years ago

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. :)

#18 in reply to: ↑ 17 @flixos90
7 years ago

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.

#19 @peterwilsoncc
7 years ago

In 38738.4.diff:

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

#20 @peterwilsoncc
7 years ago

  • Keywords 2nd-opinion added

I think we have enough consensus to put this in.

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

@flixos90
7 years ago

#21 @flixos90
7 years ago

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.

#22 @davidakennedy
7 years ago

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

@bradyvercher
7 years ago

#23 follow-up: @bradyvercher
7 years ago

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.

#24 in reply to: ↑ 23 ; follow-up: @flixos90
7 years ago

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.

#25 in reply to: ↑ 24 @bradyvercher
7 years ago

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

#26 @peterwilsoncc
7 years ago

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
7 years ago

#27 @bradyvercher
7 years ago

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

#28 @joemcgill
7 years ago

  • Keywords 2nd-opinion removed

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.

#29 @peterwilsoncc
7 years ago

  • Keywords needs-refresh added

@joemcgill thanks for looking over.

Requires refresh following [39237].

@joemcgill
7 years ago

#30 @joemcgill
7 years ago

  • Keywords needs-refresh removed

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.

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


7 years ago

#32 @peterwilsoncc
7 years ago

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

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.

#33 @westonruter
7 years ago

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.

#34 @swissspidy
7 years ago

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.

Note: See TracTickets for help on using tickets.