get_file_data() regex issue

[scribu]

I always wondered how the plugin/theme header syntax could be so permissive.

It turns out the regex is too permissive.

If the description field is missing, for example, it will match any line at the beginning of a file that contains 'description:' in it, such as:

Some_Class_Description::init();

_e('Edit description:');

[scribu]

example plugin, demonstrating bug

[Denis-de-Bernardy]

Couldn't that prevent the following two from working?

# Description: foo

/**
 * Description: foo
 */

[Denis-de-Bernardy]

Also:

// Description: foo

[filosofo]

I've seen a number of plugins with whitespace before each header, as Denis mentions.

If you want to avoid the double colon of a static method, why not use a negative lookahead instead?

:(?!:)(.*)

[scribu]

It's not just static method calls I want to avoid.

Another example of the problem:

_e('Edit description:');

[scribu]

I always wondered how

[nacin]

Also:

#Description:
//Version:

etc.

We can probably get away with preventing alphanumeric characters, i.e. [^a-z0-9]*.

Related/duplicate: #14243

[scribu]

slightly more permissive

[scribu]

15193.2.diff allows any combination of whitespace, '/', '#' and '*' characters.

I think that's the sweetspot.

[scribu]

(In [16215]) Make get_file_data() regex more precise. Fixes #15193

[nacin]

There were no unit tests or thorough research through the plugin directory on this. This could very well break a number of plugins for not being inclusive enough.

This is a trivial bug of the lowest priority. No need to rush it. Suggest revert and punt.

[scribu]

If we can knowingly break many plugins with #14915, I think we can also afford to potentially break a few plugins with extremely weird header definitions.

[nacin]

Replying to scribu:

If we can knowingly break many plugins with #14915, I think we can also afford to potentially break a few plugins with extremely weird header definitions.

We don't bargain like that. #14915 was a change deliberately made to fix an important inconsistency. Here, the fix for an extreme edge bug has the potential to introduce some much more noticeable bugs, like a plugin no longer being considered valid and thus deactivating itself.

I'm asking for research, and potentially unit tests. A grep of "Plugin Name:" through the entire repo should show every preceding character in use, and patterns could be drawn.

[scribu]

Fair enough.

[scribu]

So, based on a random sample of 2800+ plugins, here are all the combinations I found:

Plugin Name
Plugin Name
    *   Plugin name
/*Plugin name
Plugin name
      Plugin Name
     *   Plugin Name
     * Plugin Name
    * Plugin Name
    Plugin Name
   * Plugin Name
   Plugin Name
  Plugin Name
 *   Plugin Name
 *  Plugin Name
 * Plugin Name
 *	Plugin Name
 *Plugin Name
 Plugin Name
//Plugin Name
/* Plugin Name
/*Plugin Name
* Plugin Name
*	Plugin Name
	/*Plugin Name
	*Plugin Name
		Plugin Name
	Plugin Name
Plugin Name
PLUGIN NAME
  preg_match("|Plugin Name
			preg_match("|Plugin Name
                <td width="80px" valign="top">Plugin Name
				<td width="80px" valign="top">Plugin Name
			while ( ( null !== $item_key = array_rand($items) ) && false !== strpos( $items[$item_key]->get_description(), 'Plugin Name

Notice how the last lines shouldn't be recognised as valid headers.

This is the command I used:

grep --exclude-dir=.svn --include=*.php -ri 'Plugin Name:' | cut -d ':' -f 2 | sort | uniq

[nacin]

The thoroughness is much appreciated :-)

Kind of concerned about themes, too, especially since CSS isn't required to parse. Also, this won't fix #14243, but I call that an acceptable edge case.

[scribu]

Sample: 1775 random themes

Result:

  Original Theme Name
	Original Theme Name
        Theme Name
      Theme Name
    Theme Name
  Theme Name
 *   Theme Name
 * Theme Name
 *Theme Name
 Theme Name
/* Theme Name
/*	Theme Name
/*Theme Name
* Theme Name
/*Theme Name
		Theme Name
	Theme Name
Theme Name

The 'Original Theme Name' is a header defined by a 1 or two themes, after 'Theme Name'

Command used:

ack -G 'style\.css' 'Theme Name:' | cut -d ':' -f 3 | sort | uniq

[nacin]

Thanks for that! Let's call this one fixed based on the results of the research.

[scribu]

(In [16552]) Make get_file_data() regex even more precise. Props hakre. See #15193

[scribu]

hakre pointed out that '\s' in regexes causes issues on Windows systems and we're not interested in matching '\n' or '\r' anyway, so I replaced it with ' \t'.

[miqrogroove]

/facepalm

#16751