Content of css interfering with functions

[matveb]

This issue was driving me mad to debug.

Steps to reproduce:

1. Create a theme with just one style.css file with the following content:

   /*
   Theme Name: Test
   Description: Test
   Author: Test
   Template: twentyten
   */
   
   #status:hover p {
     color: #666; }
   

2. Install one of the plugins to switch themes (Theme Switcher, Theme Switch or Theme Test Drive).

---

The theme does not appear as an available theme on those plugins even though it appears fine on the admin theme page.

3. Change the content of the css from "status:hover" to "whatever:hover" and it appears.

[nacin]

Huh. Well, confirmed. Until now I didn't know that themes had a 'Status' header. I wonder if it is just there for the purpose of these theme switchers. The CSS is setting the Status header to hover, instead of publish (as would be the default).

This is an extremely rare edge case that is very tempting to wontfix, particularly due to this. If in get_file_data() we checked for word boundaries of headers, we'd still catch .status and #status. Additionally, # is valid as a PHP comment, so we couldn't exclude it, as someone could be doing this:

#Theme Name: test
#Description:
#Author:

#status:hover { ...

Even if we just accounted for [^\.\#] before the Status header, this could still affect other headers. If you have #template then WP might think it is a child theme, for example.

[azaozz]

Agree with @nacin that this is a "wontfix". IMHO this case only needs to be added to the codex with the explanation that the first CSS declaration after the header shouldn't match any of the possible header values.

[matveb]

@azaozz No problem in adding this to the codex and set as "wontifx" but wanted to state that, on the original file, the CSS declaration was on line 163; hence why it took me some time to find the culprit.

[nkuttler]

I'd like to see this bug fixed. I'm not sure how often get_file_data() is run, if only in the admin or on the frontend as well. My patch could probably be made a little faster by using a strpos() instead of the preg_match().

[nkuttler]

Patch using Tokenizer

[nkuttler]

Oh right, get_file_data() is used for readme.txt as well, so my patch probably breaks the parsing of them because the meta data is not in CSS/PHP-style comments. I'll look into that tomorrow.

[nkuttler]

The patch works fine for me. The get_file_data() code is normally only run in the admin and on theme CSS and plugin PHP files. So a performance hit doesn't seem relevant. The tokenizer seems to work fine and has been included in PHP since 4.3 (​http://www.php.net/manual/en/tokenizer.installation.php) which is the minimum requirement for WordPress.

If there is concern that people depart from the documented way of writing plugin meta info (​http://codex.wordpress.org/Writing_a_Plugin#File_Headers) a flag could be added to the get_file_data() call to differentiate between the parsing of PHP and CSS files. This way meta data in

# foo and

bar

type comments could be taken care of as well.

[matveb]

Some new findings:

It seems that all default header tags can be populated through CSS selectors, given that they were not defined earlier on the theme header.

/*
Theme Name: Test
Template: twentyten
*/

#status:hover p {
  color: #666; }

#author:hover p {
  color: #666; }

#themeuri:hover p {
  color: #666; }

#description:hover p {
  color: #666; }

#authoruri:hover p {
  color: #666; }

#version:hover p {
  color: #666; }

#tags:hover p {
  color: #666; }

On the WordPress admin theme tab you should see some "hover p {" around.

This does not seem to be too much of a concern when tags are already defined; but eventually you could set a template for themes which don't have one, effectively displaying a The parent theme is missing. Please install the "hover p {" parent theme.) on the admin.

Just add #template:hover p { color: #666; } to Twenty Ten's style.css not too far from the top and you have a broken Twenty Ten.

[westi]

Not too keen on using the tokeniser for this.

I think we can come up with a simpler improvement on the current scheme to cope with this.

For example we should only be picking out things where the "name" is anchored to the start of the line by at most white space.

[lancewillett]

Could the regex also be updated to only look for first letter capital for the default headers? Or do we allow the metadata name to be all lowercase?

Best practice for CSS selectors is to use lowercase, so if this were triggered after such a first-letter case check it would be the responsibility of the CSS author to fix it.

[mdawaffe]

Replying to nacin:

Additionally, # is valid as a PHP comment, so we couldn't exclude it, as someone could be doing this:

#Theme Name: test
#Description:
#Author:
 
#status:hover { ...

# is not a valid CSS comment.

​http://www.w3.org/TR/CSS/syndata.html#comments
​http://www.w3.org/TR/css3-syntax/#comments

I think we should force authors to use the /* */ style comment as that's the only valid CSS comment syntax.

I have no idea how fast/slow the tokenizer is, but these aren't PHP files, so it's not really the "right" tool.

Suggested regex:

/^(\s|\*)*{field name}:(.*)$/mi

That will pick up

/*
Theme Name: Test
Template: twentyten
*/

and

 /*
  * Theme Name: Test
  * Template: twentyten
  */

[nacin]

Indeed, I realized I was confusing things and made note of it in IRC at the time.

[nkuttler]

I'm beginning to think that patching up this function is the wrong way. Putting configuration information into txt or css was a bad idea to begin with.

PHP has parse_ini_file() built in. get_file_data() should really use that or something similar when some theme.cfg or plugin.cfg exists. This shouldn't be hard to implement while staying backward compatible.

This would obviously mean that code not in the distribution would need to be modified, like in the theme and plugin repositories.

Does this sound reasonable?

[westi]

Replying to nkuttler:

I'm beginning to think that patching up this function is the wrong way. Putting configuration information into txt or css was a bad idea to begin with.

PHP has parse_ini_file() built in. get_file_data() should really use that or something similar when some theme.cfg or plugin.cfg exists. This shouldn't be hard to implement while staying backward compatible.

This would obviously mean that code not in the distribution would need to be modified, like in the theme and plugin repositories.

Does this sound reasonable?

Not really.

That would mean we would have to change every plugin/theme to support the new format.

What we have works in 95% of cases and we should be able to easily fix the other 5%. We just haven't done it yet.

[scribu]

Going to close as dup of #15193 (newer, but has patch).

[nkuttler]

This wasn't fixed by 15193.

[nkuttler]

Setting to wontfix, as that seems to be the dev consensus.

[nkuttler]

Does this sound reasonable?

Not really.

That would mean we would have to change every plugin/theme to support the new format.

And I just wanted to add that no, this isn't true. It would be possible to deprecate the current behavior but keep it indefinitely.