Make WordPress Core

Opened 13 years ago

Closed 13 years ago

#20897 closed defect (bug) (fixed)

extra_theme_headers hook no longer available

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

Description

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 13 years ago.
20897.diff (464 bytes) - added by nacin 13 years ago.
20897.2.diff (713 bytes) - added by georgestephanis 13 years ago.
20897.3.diff (671 bytes) - added by georgestephanis 13 years ago.
20897.4.diff (1.2 KB) - added by nacin 13 years ago.

Download all attachments as: .zip

Change History (18)

#1 @greenshady
13 years ago

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

#2 @SergeyBiryukov
13 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
13 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
13 years ago

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

#5 @georgestephanis
13 years ago

Semi-related ... in theme-check, that's not 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

Last edited 13 years ago by georgestephanis (previous) (diff)

#6 @greenshady
13 years ago

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

#7 @georgestephanis
13 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 13 years ago by SergeyBiryukov (previous) (diff)

@nacin
13 years ago

#8 @nacin
13 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
13 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
13 years ago

Or perhaps more readable this way.

#11 @nacin
13 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().

@nacin
13 years ago

#12 @westi
13 years ago

  • Keywords dev-reviewed added

20897.4.diff looks good to me too.

#13 @westi
13 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.