WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 3 months ago

#18974 new defect (bug)

Plugin table IDs can collide with core IDs

Reported by: Viper007Bond Owned by:
Milestone: Future Release Priority: low
Severity: minor Version: 3.3
Component: Plugins Keywords: needs-patch
Focuses: administration Cc:

Description

Go to Plugins > Add New and install my "Local Time" plugin.

Go to Plugins > Installed Plugins. My plugin's details will show up in italics. This is because the table row is given the ID local-time (my plugin's slug) and core has this CSS:

#utc-time, #local-time {
	padding-left: 25px;
	font-style: italic;
	font-family: sans-serif;
}

https://core.trac.wordpress.org/browser/tags/3.2.1/wp-admin/css/wp-admin.dev.css#L4287

Targeting this in options-general.php:

<span id="local-time"><?php printf(__('Local time is <code>%1$s</code>'), date_i18n($timezone_format)); ?></span>

https://core.trac.wordpress.org/browser/tags/3.2.1/wp-admin/options-general.php#L167

I'm not quite sure why core is using an ID instead of a class. I'm also not quite sure why the plugins table isn't using a prefix of say plugin- since the slug can literally be pretty much anything.

Probably better to change the former than the latter for backwards compatibility reasons though.

Attachments (1)

18974.diff (566 bytes) - added by adambackstrom 2 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Viper007Bond3 years ago

  • Summary changed from Plugin IDs can collide with core IDs to Plugin table IDs can collide with core IDs

comment:2 follow-up: scribu3 years ago

There's a similar problem with the plugin install screen, only the plugin name is used as a class instead of as an ID.

Standardizing on 'id' and prefixing with 'plugin-' would be a more future-proof solution.

Last edited 3 years ago by scribu (previous) (diff)

comment:3 in reply to: ↑ 2 ; follow-ups: Viper007Bond3 years ago

Replying to scribu:

Standardizing on 'id' and prefixing with 'plugin-' would be a more future-proof solution.

I don't get the purpose of using IDs. I think classes are a much better option.

comment:4 in reply to: ↑ 3 ; follow-up: mikeschinkel3 years ago

  • Cc mikeschinkel@… added

Replying to Viper007Bond:

I don't get the purpose of using IDs. I think classes are a much better option.

Without IDs it can be much harder to target elements robustly using jQuery. FWIW.

comment:5 in reply to: ↑ 4 ; follow-ups: azaozz3 years ago

Replying to mikeschinkel:

Without IDs it can be much harder to target elements robustly using jQuery. FWIW.

That's not entirely true. In the past IDs were faster to target from JS, currently targeting classes is about as fast as targeting IDs (except in IE7).

The original problem:
"Go to Plugins > Installed Plugins. My plugin's details will show up in italics. This is because the table row is given the ID local-time (my plugin's slug)"

Seems the plugin needs a more "proprietary" slug, perhaps viper-local-time, etc.

comment:6 GaryJ3 years ago

I don't think plugin names should have to consider the possibility of clashing with generic (unprefixed) classes / id attributes.

Classes, not IDs, should be used for CSS. Always.

comment:7 in reply to: ↑ 3 scribu3 years ago

Replying to Viper007Bond:

Replying to scribu:

Standardizing on 'id' and prefixing with 'plugin-' would be a more future-proof solution.

I don't get the purpose of using IDs. I think classes are a much better option.

Ids make more sense because plugin slugs are unique.

Classes, not IDs, should be used for CSS. Always.

That strikes me as an overly broad generalisation. Why have ids at all, then?

Version 0, edited 3 years ago by scribu (next)

comment:8 in reply to: ↑ 5 Viper007Bond3 years ago

Replying to azaozz:

Seems the plugin needs a more "proprietary" slug, perhaps viper-local-time, etc.

WordPress is generating that slug, not my plugin:

$id = sanitize_title( $plugin_name );

I don't think I should have to change the displayed name/title of my plugin just to avoid clashing with a too generic CSS rule. :)

comment:9 in reply to: ↑ 5 ; follow-up: mikeschinkel2 years ago

Replying to azaozz:

Replying to mikeschinkel:

Without IDs it can be much harder to target elements robustly using jQuery. FWIW.

That's not entirely true. In the past IDs were faster to target from JS, currently targeting classes is about as fast as targeting IDs (except in IE7).

I wasn't referring to performance but instead to targeting an unique element without worrying that your selector can match others inadvertently. That does assume that all IDs on a page are unique, of course.

comment:10 in reply to: ↑ 9 azaozz2 years ago

Replying to mikeschinkel:

...That does assume that all IDs on a page are unique, of course.

Agreed, but that's the inherited problem with HTML IDs when dynamically generating it from many different PHP functions: in some cases IDs can be repeated which breaks the DOM.

IMHO best option is to use (plugin unique) class names and target them after looking at the DOM, i.e. limiting the DOM range.

comment:11 follow-up: nacin2 years ago

  • Keywords needs-patch added

Let's get a patch here. Let's add .plugin-$plugin (sanitize_title). Strongly tempted to kill the IDs as well. Not sure of too many trying to modify this page.

comment:12 in reply to: ↑ 11 azaozz2 years ago

Replying to nacin:

Strongly tempted to kill the IDs as well. Not sure of too many trying to modify this page.

I'm not sure why we have these IDs there in the first place. Seems the user cases are to facilitate either highlighting specific plugin (i.e. make one plugin look "special") or hiding the info about installed/active plugin(s).

Don't like the idea of the first one and thinking the second one may have security implications.

In any case, in order to fix the originally reported problem we will have to either change or remove the IDs. But if we are changing them (which would require plugins that use them to be updated) may as well remove them and add classes.

comment:13 follow-up: scribu2 years ago

I don't mind removing the ids, but could someone please explain why using classes instead would be better?

Last edited 2 years ago by scribu (previous) (diff)

comment:14 in reply to: ↑ 13 azaozz2 years ago

Replying to scribu:

With the current state of JS and CSS classes are more flexible and have no shortcomings compared to using IDs.

comment:15 scribu2 years ago

Could you be a bit more specific? bearing in mind that, in this case, both would be unique in the document.

adambackstrom2 years ago

comment:16 adambackstrom2 years ago

  • Cc adam@… added

There are a few edge cases that will still conflict with .plugin-$plugin: see existing styles for .plugin-title and .plugin-update. These appear harmless for the time being, but that may not always be the case.

Last edited 2 years ago by adambackstrom (previous) (diff)

comment:17 jdgrimes8 months ago

Maybe .plugin_$plugin should be used instead. I know that the underscore is against the CSS coding standards, but that will avoid problems with conflicts. (I don't think that there are any .plugin_* styles currently in core.

comment:18 nacin3 months ago

  • Component changed from Administration to Plugins
  • Focuses administration added
Note: See TracTickets for help on using tickets.