WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#14243 closed defect (bug) (wontfix)

Content of css interfering with functions

Reported by: matveb Owned by: westi
Milestone: Priority: lowest
Severity: minor Version:
Component: Themes Keywords: needs-patch
Focuses: Cc:

Description (last modified by nacin)

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.

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

Attachments (1)

functions.php.diff (640 bytes) - added by nkuttler 8 years ago.
Patch using Tokenizer

Download all attachments as: .zip

Change History (21)

#1 @nacin
8 years ago

  • Description modified (diff)

#2 follow-up: @nacin
8 years ago

  • Component changed from General to Themes
  • Priority changed from normal to lowest
  • Severity changed from normal to minor

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.

#3 @nkuttler
8 years ago

  • Cc office@… added

#4 @azaozz
8 years ago

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.

#5 @matveb
8 years ago

@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.

#6 @nkuttler
8 years ago

  • Keywords has-patch added

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
8 years ago

Patch using Tokenizer

#7 @nkuttler
8 years ago

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.

#8 @nkuttler
8 years ago

  • Keywords dev-feedback added

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.

#9 @matveb
8 years ago

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.

#10 @westi
8 years ago

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.

#11 @lancewillett
8 years ago

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.

#12 @lancewillett
8 years ago

  • Cc lance@… added

#13 in reply to: ↑ 2 @mdawaffe
8 years ago

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
  */

#14 @nacin
8 years ago

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

#15 follow-up: @nkuttler
8 years ago

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?

#16 in reply to: ↑ 15 ; follow-up: @westi
8 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to westi
  • Status changed from new to accepted

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.

#17 @scribu
8 years ago

  • Milestone Future Release deleted
  • Resolution set to duplicate
  • Status changed from accepted to closed

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

#18 @nkuttler
8 years ago

  • Resolution duplicate deleted
  • Status changed from closed to reopened

This wasn't fixed by 15193.

#19 @nkuttler
8 years ago

  • Resolution set to wontfix
  • Status changed from reopened to closed

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

#20 in reply to: ↑ 16 @nkuttler
8 years ago

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.

Note: See TracTickets for help on using tickets.