Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#20897 closed defect (bug) (fixed)

extra_theme_headers hook no longer available

Reported by: greenshady Owned by: westi
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.4
Component: Themes Keywords: has-patch commit dev-reviewed
Focuses: Cc:


In the past, to get theme data, one would call get_theme_data(), which would then call get_file_data(), to get file header data about a theme. This function would allow you to filter the file headers by context (e.g., plugin, theme) to add in custom file headers. Now, the extra_file_headers hook is no longer available with the switch to WP_Theme.

This hook needs to be available for backwards compatibility with themes and plugins that use it.

Here's some example code from the Theme Check plugin used by the theme review team for testing:

add_filter( 'extra_theme_headers', 'tc_add_headers' );

function tc_add_headers( $extra_headers ) {
	$extra_headers = array( 'License', 'License URI', 'Template Version' );
	return $extra_headers;

Attachments (5)

20897.patch (639 bytes) - added by SergeyBiryukov 3 years ago.
20897.diff (464 bytes) - added by nacin 3 years ago.
20897.2.diff (713 bytes) - added by georgestephanis 3 years ago.
20897.3.diff (671 bytes) - added by georgestephanis 3 years ago.
20897.4.diff (1.2 KB) - added by nacin 3 years ago.

Download all attachments as: .zip

Change History (18)

#1 @greenshady
3 years ago

  • Summary changed from extra_theme_file headers no longer available to extra_theme_headers hook no longer available

#2 @SergeyBiryukov
3 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.4

extra_theme_headers is still available, in the sense that custom headers data can be retrieved with WP_Theme->get() or WP_Theme->display().

get_theme_data() is deprecated, but we should probably keep backwards compatibility there too.

#3 @nacin
3 years ago

  • Keywords commit needs-unit-tests added

I think 20897.patch would work, yep. We could probably use some unit tests for this.

WP_Theme avoids get_theme_data() but it does pass a context of 'theme' to get_file_data(), so the filter still works. As SergeyBiryukov says, get() and display() do work. Hidden feature. :-)

#4 @greenshady
3 years ago

Correct. WP_Theme->get() and WP_Theme->display() work fine. get_theme_data() just needs that hook for backwards compatibility.

#5 @georgestephanis
3 years ago

Semi-related ... in theme-check, that's a good way to operate on a filter ... just sent a forum post your way @ http://wordpress.org/support/topic/plugin-theme-check-function-tc_add_headers-needs-to-filter-not-replace

Version 0, edited 3 years ago by georgestephanis (next)

#6 @greenshady
3 years ago

That's not my plugin. I was just using it as an example, but I do see the problem.

#7 @georgestephanis
3 years ago

Also, instead of

$extra_headers = array_combine( $extra_headers, $extra_headers );

foreach ( $extra_headers as $key ) 
    $extra_headers[ $key ] = $theme->get( $key );

$theme_data = array_merge( $extra_headers, $theme_data );

Would it be more efficient to use array_count_values over array_combine (I've never seen a performance comparison) like ...

$extra_headers = array_count_values( $extra_headers );

foreach ( $extra_headers as $key => $value ) 
    $extra_headers[ $key ] = $theme->get( $key );

$theme_data = array_merge( $extra_headers, $theme_data );

Or should we avoid having unset ones merged in to the $theme_data by ...

$fetched_extra_headers = array();

foreach ( $extra_headers as $header_name ) 
    if( $header_value = $theme->get( $header_name ) )
        $fetched_extra_headers[ $header_name ] = $header_value;

$theme_data = array_merge( $fetched_extra_headers, $theme_data ); 
Last edited 3 years ago by SergeyBiryukov (previous) (diff)

3 years ago

#8 @nacin
3 years ago

  • Keywords needs-unit-tests removed

I was able to simplify this patch: 20897.diff.

Here are some unit tests to back it up: http://unit-tests.trac.wordpress.org/changeset/736.

#9 @georgestephanis
3 years ago

Minor issue with 20897.diff -- doubtful that it will ever be relevant, but if someone filters in URI, Description, Author, or AuthorURI to extra_theme_headers then we'd override the values previously set.

array_merge() as used in the initial patch avoids that by having the previously defined $theme->display() versions trump the $theme->get() versions.

Also, the current version of the function has 'AuthorName' => $theme->display('Author', false, false), on line 3114 -- ->display() with two false params is functionally the same as ->get() -- may be simpler to use that instead?

20897.2.diff attached with these ideas worked out.

#10 @georgestephanis
3 years ago

Or perhaps more readable this way.

#11 @nacin
3 years ago

Sure, I had considered the overlap, and while it's not going to happen in practice, we can add a little sanity check there.

20897.4.diff also prevents load_textdomain() from being called when we are not trying to translate something via display().

3 years ago

#12 @westi
3 years ago

  • Keywords dev-reviewed added

20897.4.diff looks good to me too.

#13 @westi
3 years ago

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

In [21050]:

Restore the 'extra_theme_headers' filter in the deprecated get_theme_data function so that plugins/themes using this function can still access their extra headers.

Fixes #20897 props nacin, georgestephanis, SergeyBiryukov.

Note: See TracTickets for help on using tickets.