WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#8964 closed enhancement (fixed)

Allow adding headers to get_plugin_data

Reported by: strider72 Owned by: westi
Milestone: 2.9 Priority: normal
Severity: normal Version:
Component: Plugins Keywords: has-patch tested commit early
Focuses: Cc:

Description

It would be nice for plugin authors if we could add headers to what is searched for in get_plugin_data. The attached patch integrates this as a straightforward filter.

For example, an author might add a header that states the minimum WP version, or perhaps a URL for version checking. With this filter he could just drop that info into an easily accessed plugin header instead of hard-coding it somewhere further down. (Beyond simple location, this much simplifies certain processes, such as a plugin with which other plugins can make themselves compatible.)

The attached patch modifies get_plugin_data(). Default output is identical to existing, but you can now do the following filter:

function filter_plugin_data_headers( $extra_params, $plugin_file ) {
	if ( __FILE__ == $plugin_file ) {
		$extra_params['NewHeader'] = 'New Plugin Header';
	}
	return $extra_params;
}
add_filter( 'plugin_data_headers', 'filter_plugin_data_headers', 10, 2 );

In the above, the plugin could have:

New Plugin Header: just testing

and get_plugin_data() would return "just testing" as $plugin_data['NewHeader']

NOTE: This is explicitly written so that a plugin can *not* overwrite existing standard headers. That is, it is not possible to overwrite the Name, Description, etc. of another plugin, as that might pose the risk for mischief. You can only use this to add a new header, which will be returned as part of the get_plugin_data array.

Attachments (8)

get_plugin_data_filter.diff (2.2 KB) - added by strider72 5 years ago.
adds filter to get_plugin_data() so you can add new headers to search for
get_plugin_data_filter_2.diff (2.5 KB) - added by strider72 5 years ago.
changes the way array keys works as described in above comment
8964.diff (2.0 KB) - added by Denis-de-Bernardy 5 years ago.
improved patch, updated against 11255
trac-8964-demo.php (1.6 KB) - added by strider72 5 years ago.
"Plugin Header Test" plugin that demonstrates usage and tests against potential abuse. See source for notes.
8964-3.diff (2.7 KB) - added by strider72 5 years ago.
Better security than 8964.diff. Avoids plugins overwriting each other's headers by forcing key and value to be the same. Changes "plugin_headers" hook to "extra_plugin_headers"
wptrac_8964_patch4.diff (8.8 KB) - added by strider72 5 years ago.
Adds hooks to both plugins and themes, new function get_file_data()
wptrac_8964_patch5.diff (8.9 KB) - added by strider72 5 years ago.
bugfix -- patch 4 didn't properly handle Author header in all cases
wptrac_8964_patch6.diff (8.8 KB) - added by strider72 5 years ago.
Same as patch 5 with bug fixes as discussed in above comments

Download all attachments as: .zip

Change History (58)

strider725 years ago

adds filter to get_plugin_data() so you can add new headers to search for

comment:1 strider725 years ago

Just a note: I am aware of the similar patch at http://trac.wordpress.org/ticket/4048 , but the protests against that one seem to mainly have been the potential to "hack" other plugin by changing their headers. So again, this patch is explicitly designed to only allow you to add a new header to be returned, not to alter existing ones in any way.

comment:2 strider725 years ago

  • Cc strider72 added

Another idea is that perhaps it would be best to only allow the plugin to specify the header, e.g. "New Plugin Header", and the array key for get_plugin_data would be auto-generated from that, e.g. $plugin_data['NewPluginHeader]. Might make it more difficult for two plugins using this from interfering with each other. I'm not sure how this would best work though. Perhaps the key should just be the unaltered Plugin header, e.g. $plugin_data['New Plugin Header'] ?

comment:3 Denis-de-Bernardy5 years ago

yes, yes, yes! this would be sooooo much better than needing to re-open each plugin file each time an extra field gets added.

comment:4 strider725 years ago

Updated the patch so that the key in the resulting $plugin_data array equals the actual name of the Plugin Header.

This change prevents potential conflicts wherein two plugins might define a different but similar header, but then use the exact same key in the resulting array.

strider725 years ago

changes the way array keys works as described in above comment

comment:5 strider725 years ago

If we do this, it should apply to Themes as well.

comment:6 westi5 years ago

  • Owner set to westi
  • Status changed from new to assigned

This looks like a good idea.

We definitely need to ensure the changes are additive.

We should also support the same for themes so could you do a patch for that too?

I think the plugin_data_headers filter should be plugin specific to encourage correct programming with a helper function for registering for it like the activate/deactivate ones.

That should ensure that plugins are written to use it correctly and saves the code you have to check in the action for it being for you.

comment:7 strider725 years ago

1) The changes are additive -- I do a merge_array giving existing headers priority.

2) Let's get Plugins solid and then I'll go in and do the same for Themes.

3) By "plugin specific" you mean it should only work if it specifies the plugin to check? I say resoundingly, NO. That would defeat the ability to make my plugin work with "whatever other plugins are designed to be compatible". i want to be able to make code that says "For each plugin that has Header X, do this...."

Not sure how "plugin specific" ensures "correct programming"

I don't understand what you mean about a helper function. Why do this? Isn't the filter much simpler? K.I.S.S.

"... and saves the code you have to check in the action for it being for you."

I don't understand that statement at all.

A good example of this plugin is a 3rd party version checker. We can add a header "Version Check URL:" The plugin doing the check can readily find *all* plugins that have this header and check those URLs for update info. I'm not sure how extensive plugin registration helps this process.

For a specific use case, look at this: http://code.google.com/p/strider-core/
This is a plugin framework I'm working on, that includes the aforementioned version checking. There is no download under the "main" code section, but look in Downloads for a sample plugin that uses the framework. (It makes more sense in context anyway....)

comment:8 strider725 years ago

One more comment about making plugins "register" their own custom headers:

There are cases where you want the header to be returned even for plugins that are not activated. Again, the version check plugin is a good example. If a plugin has to register itself, this can't be done unless the plugin is actively running. What I'm trying to do is simply make it so any plugin can check plugins (itself or others) for additional specific headers.

Of course, if you want to check a header and only have a function do something to Plugin X, your function can be coded to only act if it's dealing with Plugin X... but that's up to the programmer.

comment:9 Denis-de-Bernardy5 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 2.8 to Future Release

broken patch

Denis-de-Bernardy5 years ago

improved patch, updated against 11255

comment:10 Denis-de-Bernardy5 years ago

  • Keywords has-patch tested added; needs-patch removed

comment:11 Denis-de-Bernardy5 years ago

  • Keywords needs-patch added; has-patch tested removed

problem on keys

comment:12 Denis-de-Bernardy5 years ago

  • Keywords has-patch tested commit added; needs-patch removed
  • Milestone changed from Future Release to 2.8

nm, the keys are overridden as needed by array_merge. with the patch I uploaded, you cannot override one of the default keys using a plugin -- i.e. the expected and needed behavior.

comment:13 westi5 years ago

  • Keywords tested commit removed
  • Milestone changed from 2.8 to 2.9

Lets leave this to 2.9 for now as we are feature frozen and I want to make sure we get this feature right.

comment:14 Denis-de-Bernardy5 years ago

  • Keywords tested commit early added; plugins filters removed

well, you might want this in 2.9 rather than 2.8, which I can understand if we're in feature freeze. but it's still tested and commit-worthy... dunno why you removed it from the commit candidate list.

comment:15 Denis-de-Bernardy5 years ago

it could easily make it in 2.8, too. I sort of need this for one of my own plugins, and I'm apparently not alone. currently, I end up re-opening every plugin file a second time to extract a single field. it has worked well for 3 WP versions, at it'll continue to work for another one, but that hardly is the most efficient way to do things.

imo, this should get into 2.8, since it works and you've at least two people here who are ready to nail down potential issues on the spot. and we'd enhance it afterwards (though I can't think of anything *else* we'd want in addition).

else, the ticket may end up just sitting there for years, as is so typical in trac.

comment:17 markjaquith5 years ago

+1 for early 2.9

comment:18 Denis-de-Bernardy5 years ago

amen to that, then.

comment:19 strider725 years ago

When this is committed I'll do an equivalent patch for themes (using same code, basically). Both could then make it into 2.9.

comment:20 strider725 years ago

Okay -- my original patch passed $plugin_file to the filter function, and the new patch doesn't.

Not sure if this is important or not, but I thought the distinction worth noting, as it was a sort of silent change between versions....

comment:21 follow-up: Denis-de-Bernardy5 years ago

Picture a third party looking into adding headers that your plugin adds, without you knowing it.

comment:22 Denis-de-Bernardy5 years ago

still applies clean

comment:23 in reply to: ↑ 21 strider725 years ago

Replying to Denis-de-Bernardy:

Picture a third party looking into adding headers that your plugin adds, without you knowing it.

This doesn't make sense (at least it's not a problem). So what if two plugins add the same header? Because the header is the array key, it's only added once, and because the returned parameter *is* the Header Name, they both add the same header that will return the same parameter within the plugin_data array. So it's no problem.

...unless your patch does something different than mine did. If you add "New Header", then

New Header: foo

gets us plugin_dataNew Header? = 'foo'

It doesn't matter which one, or how many, different plugins add "New Header". If it's added it's added for *all* plugins. (And as most plugins won't have that custom header, they'll just return "".)

comment:24 strider725 years ago

Stoopid wiki formatting. Should be:

gets us plugin_data['New Header'] = 'foo'

comment:25 strider725 years ago

Aha! Denis, your updated patch leaves out a *very* important line:

// Use keys to reduce conflicts and make conflicts easier to find.
foreach ( $extra_headers as $key=>$value ) { $extra_headers[$key] = $key; }

This goes just *before* the array_merge. What this does is (by design) completely ignores the value, and uses only the array keys. Thus you can't have the conflicts that you are concerned about. The key of the new parameter in plugin_data is *always* the exact same as the name of the header.

As I looks at that code, however, I see a way to make it better. Try this instead -- again just before the array_merge:

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

This is an improvement from the plugin author's standpoint. Rather than adding a key, e.g. $extra_headers['New Header'] = true; they just have to add a value: $extra_headers[] = 'New Header';

Array_unique removes duplicate values, and then the array_combine takes all the values and sets those as both Key and Value for each element. Perfect! :)

comment:26 strider725 years ago

Correction: We don't need array_unique, because array_combine already effectively does that. (Not normally, but because we're in the unusual position where keys and values are the same, it works.)

So, just add this right before the array_merge line in Denis's patch:

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

comment:27 strider725 years ago

ARGH! Array_combine is PHP 5! Sorry, sorry, sorry. (And sorry for spamming the inboxes of whomever get updates on this patch.) We need this to work with PHP 4. This should do it:

$extra_headers = array_flip( $extra_headers );
foreach( $extra_headers as $key=>$value ) {
   $extra_headers[$key] = $key;
}

Again, sorry for so many posts. This should work out nicely. Add it to Denis's patch, just above the array_merge line.

comment:28 strider725 years ago

With the new patch 8964, plugins can add new plugin headers by doing something like this in a hooked function:

$extra_headers[] = 'My New Header';
return $extra_headers;

strider725 years ago

"Plugin Header Test" plugin that demonstrates usage and tests against potential abuse. See source for notes.

comment:29 strider725 years ago

Example of usage in a plugin:

function header_demo( $extra_headers ) {
	$extra_headers[] = 'Demo Header';
	return $extra_headers;
}
add_filter( 'plugin_headers', 'header_demo' );

comment:30 strider725 years ago

Two files added:
1) New patch that updates for current code and solidifies security
2) Demo plugin

Demo plugin both adds headers and has notes in source showing what's happening.

Demo plugin also has faux "malicious" hook which demonstrates that attempts to alter default headers should fail; this patch *only* allows the addition of new headers.

comment:31 westi5 years ago

Ok had a look over this.

I'm wondering if the following changes might improve things:

  • Rename the plugin_headers filter to extra_plugin_headers
  • Expect the array back from extra_plugin_headers filter which contains Name => Regex match like the one specified for the normal headers.
  • Use array_merge($extras,$defaults) so that you can't overwrite the defaults.

Otherwise all looks good :-)

comment:32 follow-up: strider725 years ago

1) Good idea, since it doesn't allow you to change the existing headers

2) Could you explain what you mean? The only thing that matters in the returned array is the values. Keys are irrelevant. In the final it uses $value => $value for the new header

3) It already does this:
$all_headers = array_merge($extra_headers, $default_headers);

comment:33 in reply to: ↑ 32 westi5 years ago

Replying to strider72:

1) Good idea, since it doesn't allow you to change the existing headers

2) Could you explain what you mean? The only thing that matters in the returned array is the values. Keys are irrelevant. In the final it uses $value => $value for the new header

Give extra headers the same ability as the built in ones - i.e. the key name that ends up in the array can be different from the string matched in the file.

3) It already does this:
$all_headers = array_merge($extra_headers, $default_headers);

True - I was refering to removing all the array_flippery which seemed like a bit over the top - just make the requirements on the plugin behaviour like I suggest about and then that can all be stripped out.

comment:34 strider725 years ago

Give extra headers the same ability as the built in ones - i.e. the key name that ends up in the array can be different from the string matched in the file.

I explicitly avoided this so that different plugins can't accidentally overwrite each other.

What happens when one plugin adds a header of "Foo" with a key of "baz" and another adds a "Bar" header also with a key of "baz"? Can't have both. If the header and key (by definition) match, it simplifies things greatly. "Foo" is "Foo" and "Bar" is "Bar". Period.

comment:35 strider725 years ago

Note: somewhere down the road when WordPress moves to PHP 5+, all the "array_flippery" can be reduced to one line:

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

As I see it, the only change now is to change the "plugin_headers" hook to "extra_plugin_headers".

comment:36 strider725 years ago

New patch changes "plugin_headers" hook to "extra_plugin_headers". Also some (very) minor code cleanup.

strider725 years ago

Better security than 8964.diff. Avoids plugins overwriting each other's headers by forcing key and value to be the same. Changes "plugin_headers" hook to "extra_plugin_headers"

strider725 years ago

Adds hooks to both plugins and themes, new function get_file_data()

comment:37 strider725 years ago

New patch. I've abstracted the header-fetching to a separate function which is used by both plugins and themes. So, yes, this patch adds both an extra_plugin_headers hook and an extra_theme_headers hook.

Basically, you can make a wrapper function that:

1) defines an array of the headers to be searched for
2) passes it to get_file_data() along with the file
3) optionally, passes a $context string that automatically instantiates an extra_$context_headers hook.

This also allows plugins to use this function to search for similarly styled headers in different types of files. Makes it easy for a plugin to fetch data relating to custom config files or such that apply to itself. :)

strider725 years ago

bugfix -- patch 4 didn't properly handle Author header in all cases

comment:38 strider725 years ago

Just added a patch that fixes a bug in the previous patch. No other changes, though it's nice to see it still applies clean. :)

Westi, could you please take a look at this again? I would really like this committed for 2.9. Not sure if something particular is holding it up.

This patch:

1) adds extra_plugin_headers hook so plugins can add to the list of headers that are checked for in get_plugin_data().

2) adds extra_theme_headers hook. Same as above, but for themes.

3) adds new function get_file_data() so that plugins can use a standard method to examine a particular file for plugin-like headers. Function can even be used to add an arbitrary extra_XXX_headers hook.

4) Both get_plugin_data() and get_theme_data() now use get_file_data() for their core functionality, removing some code repetition from core.

comment:39 strider725 years ago

  • Cc strider72 removed

comment:40 strider725 years ago

In patch 5, there's a minor error in the documentation/header for the get_file_data() function. We need to remove the line:

* @param bool $translate If the returned data should be translated

and insert the line:

* @param string $context If included, a filter hook "extra_<$context>_headers" will allow other code to alter the headers array

comment:41 strider725 years ago

Finding bugs, gosh stinking darn it! (Pardon my language)

In get_theme_data() from patch 5,

$theme_data['Name'] = $title = wp_kses( $theme_data['Name'], $themes_allowed_tags );

should be

$theme_data['Name'] = $theme_data['Title'] = wp_kses( $theme_data['Name'], $themes_allowed_tags );

Changing the subject slightly:

Also in get_theme_data() from patch 5, at the end just before output I have the line:

unset( $theme_data['AuthorURI'] );

That's there because I was rather slavishly ensuring that the output of the function was unchanged. Considering that get_plugin_data() now returns AuthorURI, get_theme_data should do the same. Thus, that unset() should be removed.

I'll get a new patch up that addresses all of the above (plus my earlier comment).

NOTE: if get_theme_data returns AuthorURI, it follows that it should also return Author without turning it into a link (again, making it the same as get_plugin_data). That would require changes to the Themes admin page though, so I'll leave that for a separate ticket.

strider725 years ago

Same as patch 5 with bug fixes as discussed in above comments

comment:42 strider725 years ago

New patch 6 includes everything from patch 5 except:

  1. Fixed bug that missed outputting "Theme" in get_theme_data. Identical to "Name", but presumably for backward compatibility? I'm just matching the output of get_theme_data as it already exists.
  1. get_theme_data now also outputs "AuthorURI". Just a side-effect of how the new function works -- previous patch explicitly removed it, but why bother? Also brings it closer to parity with get_plugin data. Happy side effect, I say. :)

comment:43 strider725 years ago

Patch 6 also includes a fix to the doc header of the get_file_data function. The parameters are described correctly now.

comment:44 westi5 years ago

I've kicked the tires on the latest patch and all looks good.

Found one typo but nothing else.

We already had some theme metadata tests in the wordpress-tests and I have added some for plugin metadata too.

Going to leave this open for a little while in case any bugs crop up in further tests.

comment:45 westi5 years ago

(In [12044]) Allow for plugins to enhance the number of metadata fields captured from plugin and theme headers. See #8964 props strider72.

comment:46 westi5 years ago

(In [12048]) Ensure that Title is always set for plugin data as it was before. see #8964.

comment:47 strider725 years ago

Westi -- I don't recall the specifics, but I'm pretty sure your last-minute "Title" addition is redundant.

A change in core (a while back) took away the Title attribute from that function, and I submitted a patch to re-add it. Somebody else implemented a different solution involving a wrapper function of some sort. I'll see if I can find the old patch & what happened with it....

comment:48 strider725 years ago

Westi: My error -- you are correct.

I was thinking of this: http://core.trac.wordpress.org/ticket/7479

...but the Title does still need to be set, as you have done. No changes needed.

comment:49 strider724 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

This was committed a month ago. Should have been closed as "fixed" I assume.

comment:50 hakre4 years ago

well it depends on the point of view. For a plugin to add headers it must already be active with this implementation.

For a plugin that can add headers by just writing them in their file, the implementation must be different. I opened a new ticket regarding an improvement on the overall domain: #13699

Note: See TracTickets for help on using tickets.