Make WordPress Core

Opened 12 years ago

Closed 8 years ago

#18974 closed defect (bug) (fixed)

Plugin table IDs can collide with core IDs

Reported by: viper007bond's profile Viper007Bond Owned by: obenland's profile obenland
Milestone: 4.5 Priority: normal
Severity: normal Version: 3.3
Component: Plugins Keywords: has-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 (2)

18974.diff (566 bytes) - added by adambackstrom 12 years ago.
35083-18974.patch (834 bytes) - added by khag7 8 years ago.
ditch the id attribute in favor of the data-plugin attribute; also use esc_attr

Download all attachments as: .zip

Change History (27)

#1 @Viper007Bond
12 years ago

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

#2 follow-up: @scribu
12 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 12 years ago by scribu (previous) (diff)

#3 in reply to: ↑ 2 ; follow-ups: @Viper007Bond
12 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.

#4 in reply to: ↑ 3 ; follow-up: @mikeschinkel
12 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.

#5 in reply to: ↑ 4 ; follow-ups: @azaozz
12 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.

#6 @GaryJ
12 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.

#7 in reply to: ↑ 3 @scribu
12 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, anywhere, then?

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

#8 in reply to: ↑ 5 @Viper007Bond
12 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. :)

#9 in reply to: ↑ 5 ; follow-up: @mikeschinkel
12 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.

#10 in reply to: ↑ 9 @azaozz
12 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.

#11 follow-up: @nacin
12 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.

#12 in reply to: ↑ 11 @azaozz
12 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.

#13 follow-up: @scribu
12 years ago

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

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

#14 in reply to: ↑ 13 @azaozz
12 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.

#15 @scribu
12 years ago

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

@adambackstrom
12 years ago

#16 @adambackstrom
12 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 12 years ago by adambackstrom (previous) (diff)

#17 @jdgrimes
11 years 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.

#18 @nacin
10 years ago

  • Component changed from Administration to Plugins
  • Focuses administration added

#19 @chriscct7
8 years ago

  • Priority changed from low to normal
  • Severity changed from minor to normal

#20 @SergeyBiryukov
8 years ago

#35083 was marked as a duplicate.

@khag7
8 years ago

ditch the id attribute in favor of the data-plugin attribute; also use esc_attr

#21 @khag7
8 years ago

  • Keywords has-patch added; needs-patch removed

#22 @SergeyBiryukov
8 years ago

  • Milestone changed from Future Release to 4.5

#24 @obenland
8 years ago

  • Owner set to obenland
  • Status changed from new to accepted

#25 @obenland
8 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 36205:

Plugins: Avoid ID attribute collisions in plugins list table.

Removes id attributes with non-unique plugin slug and adds a data attribute
with the unique plugin file.

Props khag7.
Fixes #18974.

Note: See TracTickets for help on using tickets.