Make WordPress Core

Opened 14 years ago

Closed 14 years ago

#13699 closed feature request (maybelater)

File Header (Plugin Header, Theme Header) Improvement

Reported by: hakre's profile hakre Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.0
Component: General Keywords: has-patch dev-feedback
Focuses: Cc:

Description

Summary:

There is a design issue with the current implemenation of how headers are parsed from theme and plugin files.

The main flaw is, that the current code hardencodes concrete header names. There has already been an extension to it, so plugins can de-couple from concrete header names ([12044]) but in the end, the root cause has not been touched: Make the implementation work with (almost) any header name.

Motivation:
Goal is to reduce the dependency within the API from concrete header names to make it more flexible for the things to come.

Next to that, plugin and theme authors can benefit from a better implemenation to re-use existing API functions to parse files and to more easily add own header names.

Domain: File Headers are commonly used in the WordPress project to add meta-information to themes and plugins. They are file based and are sometimes optional and sometimes mandatory. Basically they are providing meta information for source-code files (CSS in Themes, PHP in Plugins) which is used in the backend later (The Appearance and Plugins Admin-Pages).

Implementation: I made a first step in a two step implementation as a proof of concept:

Step 1: Generic File Header Parsing function
Step 2: Better use of that function

For the first step, I've provided a patch. It contains a re-implementation of a file header parser that does this independent to header names. Next to that it contains various code improvements in the existing function.

The second step is not yet done and should be done after discussion about the generic parsing implementation. One reason is, that in the details there are additional flaws which could be fixed as well. For example, after the extension to headers in #8964 / [12044], the return array of get_file_data not only remains undocumented but keys are named inconsistent (plugins can not key them as the core keyes them).

Benefits: Overall this will help to deal with existing and new headers, because discussing and (re-)implementing them does not need to alter that many lines of code. This will make things more stable in the end.

Additionally on the social side, discussion can be better spereated between technical issues (that string can not be used as header) and strategic issues (we need a header named as ...). This will especially help developers who are creating patches etc. related to the topic.

Related: There are some tickets I'd like to list (ordered by number) because they are touching the topic:

  • #8964 - Allow adding headers to get_plugin_data
  • #10814 - Plugin GUIDs
  • #12130 - Add Donate URI plugin header
  • #12260 - Add PHP requirement to plugin info retrieved trough API call

Next to that I've created a documentation in codex that should be pretty complete in terms of file headers.

Attachments (2)

13699.step1.patch (3.6 KB) - added by hakre 14 years ago.
Step 1
13699.step1-r2.patch (3.4 KB) - added by hakre 14 years ago.

Download all attachments as: .zip

Change History (18)

@hakre
14 years ago

Step 1

#1 @GautamGupta
14 years ago

  • Cc gautam.2011@… added

#2 @chipbennett
14 years ago

  • Cc chip@… added

#3 @hakre
14 years ago

  • Keywords dev-feedback added

#4 follow-up: @strider72
14 years ago

I don't understand the problem with the current (pluggable) headers. If for some reason you want to implement "Header X" in plugins, that won't do anything unless there is code running that.... you know... *does something* with it.

Whatever code is doing something with Header X can add Header X to the headers that are returned by get_plugin_data().

Can you give a solid example of what you're talking about so I can understand you?

#5 in reply to: ↑ 4 @hakre
14 years ago

Replying to strider72:

I don't understand the problem with the current (pluggable) headers.

The problem is that you can only deal with headers if you a) know their names upfront and b) you were already in the position to execute code to add those known names.

Only if a) and b) is an option you can deal with additional header in wordpress. For example, this is not the case with files that do not get executed.

#6 @hakre
14 years ago

Related: #9532

#7 @hakre
14 years ago

Related: #3089

#8 @hakre
14 years ago

Interesting part is:

Replying to nbachiyski:

Oops, I forgot the formatting ;)
Here are the sampe headers again:

 /*
 Plugin Name: Bender, bender, where are you?
 Plugin Name[bg_BG]: Бендър, бендър, къде си?
 Plugin URI: http://bender.wow/
 Plugin URI[bg_BG]: http://bg.bender.wow/
 ... and so on
 */

#9 @hakre
14 years ago

Tag: WPFP

#10 follow-up: @scribu
14 years ago

To summarize, you want to get all the headers from a file, even if they weren't registered beforehand.

Considering how lax the current parsing is, I don't think this is possible, without breaking several themes & plugins.

See #15193

#11 in reply to: ↑ 10 @hakre
14 years ago

Replying to scribu:

To summarize, you want to get all the headers from a file, even if they weren't registered beforehand.

Exactly. The API function provides them then.

Considering how lax the current parsing is, I don't think this is possible, without breaking several themes & plugins.

See #15193

Well if it's possible for that regex introduced in #15193, why shouldn't it be possible here? So technically, this would break several themes & plugins as much as it would be breaking in [16215]. If not some of the cases have been already addressed in this code. But I'll update the patch, I didn't run that many tests as you did. Will take some time as it is stale.

#12 @hakre
14 years ago

Patch has been refreshed against trunk. As said, the way how headers are parsed are basically the same. I changed the regexes to reflect [16215]. In difference to the current parser, this one expects all headers to be lined-up in one block (one after the other, one per line) and it actually expects a header to have a value. In the current implementation this differs, it looks for headers everywhere in the first 8KB of the file (which turned out the error in #15193 most probably) and does not take care if a header actually contains a value or not.

I've tested the 13699.step1-r2.patch against a multitude of plugins with different formattings, and it looks good to me so far. But I have not run it against a list of all plugins from the repository for example.

#13 follow-up: @scribu
14 years ago

You still haven't provided an actual use case for this.

#14 in reply to: ↑ 13 @chipbennett
14 years ago

Replying to scribu:

You still haven't provided an actual use case for this.

Here's one example: "License" and "License URI" headers now being added to Themes.

#15 @scribu
14 years ago

Yes, but as somebody previously mentioned, you already have a filter for registering custom headers.

I meant an example where headers need to be collected without being registered first.

#16 @westi
14 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed

Before we change this code again we need to ensure we have good test coverage and a list of bugs that we are fixing.

For now the code works well enough.

Note: See TracTickets for help on using tickets.