Make WordPress Core

Opened 13 years ago

Closed 13 years ago

#19510 closed defect (bug) (fixed)

wp_print_styles() causing mid-page scripts/styles to bleed into admin

Reported by: adambackstrom's profile AdamBackstrom Owned by:
Milestone: 3.3.1 Priority: normal
Severity: major Version: 3.3
Component: Editor Keywords: has-patch commit
Focuses: Cc:

Description

I've been calling wp_enqueue_style() from the wp_print_styles action, as the wp_enqueue_style Codex page seemed to recommend. With the new wp_editor(), this causes styles intended for the template to bleed over into the admin. Example:

function trunktest_styles() {
	wp_enqueue_style( 'trunktest', site_url('?trunktest=1') );
}
add_action( 'wp_print_styles', 'trunktest_styles' );

function trunktest_init() {
	if( isset( $_GET['trunktest'] ) ) {
		header( 'Content-type: text/css' );
		echo 'body { background-color: red !important; }';
		die();
	}
}
add_action( 'init', 'trunktest_init' );

In WordPress 3.3 RC3, wp_editor() causes this style tag to be printed mid-way down the Dashboard page and various Comments pages. This behavior does not exist in WordPress 3.2.1, or on the majority of 3.3 RC3 pages.

Apologies if this is just an error in my usage of wp_enqueue_style().

Attachments (1)

19510.diff (601 bytes) - added by nacin 13 years ago.

Download all attachments as: .zip

Change History (22)

#1 @dd32
13 years ago

You're almost correct, in that wp_print_styles was usually only called on the front end in 3.2, it could be called from within the admin, Since WordPress 2.6 the function has been called in the admin if a CSS stylesheet is enqueued after the other stylesheets have already been printed, in 3.3 though, this happens when WP_Editor() is used anywhere too.

'wp_enqueue_scripts' is a better hook for adding/enqueueing Styles/Javascript on the front end, whilst 'admin_enqueue_scripts' is available on the back-end.

As you've noted though, It's quite common to suggest using 'wp_print_styles' to enqueue CSS, this might be due to there being no 'wp_enqueue_style' action.. The codex examples all use this too.

Last edited 13 years ago by dd32 (previous) (diff)

#2 @nacin
13 years ago

  • Keywords dev-feedback added
  • Milestone changed from Awaiting Review to 3.3
  • Severity changed from normal to blocker
  • Summary changed from wp_editor() causing wp_print_styles to bleed into admin to wp_print_styles() causing mid-page scripts/styles to bleed into admin

The issue here is that we use wp_print_styles( $handle ) in wp_editor() and elsewhere to print scripts/styles directly.

This fires the wp_print_styles hook, which is incorrectly listed on the Codex as the best hook (that'd be wp_enqueue_scripts) and is referenced 965 times in the plugins directory.

Combine this with mid-page styles/scripts, and you have frontend styles leaking.

Possible solution: Re-tool print_admin_styles() to accept (and print) a handle, and change our wp_print_styles( $handle ) calls to print_admin_styles( $handle ).

#3 @nacin
13 years ago

Or:

Only fire the wp_print_styles hook (which is in wp_print_styles()) for ! is_admin(). Unsure what that will break, though since we don't use wp_print_styles() (without a handle) in the admin, it should be pretty safe. Going forward, though, we need an admin version of this function.

#4 @dd32
13 years ago

See #9346 for mid-page script/style enqueue's

#5 follow-ups: @scribu
13 years ago

Or we could make 'wp_print_styles' pass the handle as a parameter. Same for 'wp_print_scripts'.

#6 in reply to: ↑ 5 @nacin
13 years ago

Replying to scribu:

Or we could make 'wp_print_styles' pass the handle as a parameter. Same for 'wp_print_scripts'.

That doesn't help the current regression, no?

#7 @nacin
13 years ago

Or:

Avoid print_late_styles() for is_admin(). May want to consider the same for scripts.

#8 in reply to: ↑ 5 @azaozz
13 years ago

Replying to scribu:

Or we could make 'wp_print_styles' pass the handle as a parameter.

You mean if there's a style handle, we don't run the action? That may work:

function wp_print_styles( $handles = false ) {
  if ( empty($handles) )
    do_action( 'wp_print_styles' );
...

Thinking that even if we have a separate function for the admin, a plugin may still use wp_print_styles() in the admin. Seems choices are either to disallow enqueueing mid-page for styles or not run the hook in the admin.

Last edited 13 years ago by azaozz (previous) (diff)

#9 @dd32
13 years ago

Avoid print_late_styles() for is_admin(). May want to consider the same for scripts.

Mid-page enqueues are new in 3.3 - if we can safely neuter it for 3.3 admin without affecting any core functionality, then this gets my vote.

We can revisit it in 3.4 if need be.

#10 follow-up: @scribu
13 years ago

Replying to azaozz:

function wp_print_styles( $handles = false ) {
  if ( empty($handles) )
    do_action( 'wp_print_styles' );
...

Yes, that would be a better fix for the problem I was thinking about:

We really use wp_print_scripts() in two ways:

a) to output all the styles enqueued so far, in which case the 'wp_print_styles' hook makes sense without a parameter.

b) as a wrapper for $wp_styles->do_items( $handle ), in which it doesn't make sense to fire the hook, even with a parameter, due to the way it's already used in the wild.

+1.

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

#11 @nacin
13 years ago

  • Keywords regression added
  • Severity changed from blocker to critical

#12 in reply to: ↑ 10 @azaozz
13 years ago

Replying to scribu:

We really use wp_print_scripts() in two ways:

a) to output all the styles enqueued so far, in which case the 'wp_print_styles' hook makes sense without a parameter.

b) as a wrapper for $wp_styles->do_items( $handle ), in which it doesn't make sense to fire the hook, even with a parameter, due to the way it's already used in the wild.

Right. The same is true for both wp_print_scripts() and wp_print_styles(). It doesn't make sense to run the hooks when used as shortcuts to $wp_styles->do_items('handle') and $wp_scripts->do_items('handle'). Until 3.3 this wasn't causing problems as these hooks didn't matter when run mid-page.

Last edited 13 years ago by azaozz (previous) (diff)

#13 @dd32
13 years ago

Taking a step back here, This really only affects plugins/themes that don't use specific selectors.

I'd be willing to bet that most of the plugins using this hook, will be enqueuing CSS for specific classes - otherwise they'll be breaking more than just a few things on the front end.

A quick hotfix which should solve any problems, whilst hopefully not causing any others, would be something like this:

add_action( 'admin_init', function() { remove_all_actions('wp_print_styles'); }, 1);

that'll catch and remove any hooks that were probably meant for the front end.

#14 @westi
13 years ago

  • Keywords regression removed

I don't think this is a critical / blocker issue for 3.3

We will (however much we hope we won't) find one or more similar issues once we release due to the extreme extensibility of WordPress and the 10000's of plugins that exist.

We should look to address this issue along with any others that come up in 3.3.1 and communicate the incorrect hook usage through codex updates and a wpdevel post.

We could also email plugin authors we know are making this mistake a heads up as the fix for them is simple.

Removing regression because as dd32 points out the bug was possible to trigger in previous releases we have just made it much easier to trigger.

#15 follow-up: @nacin
13 years ago

While the Hotfix plugin could solve things, this problem is very difficult to identify without a bit of skill. A user will need to inspect styles to figure out that they are being printed by an errant stylesheet. The blind "Re-activate plugins one by one" will often be needed to find the culprit.

I've seen a lot of situations where "Just install Hotfix" has been recommended even though the plugin will not address the problem. It muddies the plugin's usefulness -- it's best for when there is a targeted, specific error message that clearly shows that Hotfix needs to be used. And I'm not sure I like us relying on it to hit a deadline.

I think the best and most targeted fix here is to simply not fire the wp_print_styles hook in the admin. Then come 3.4, we stop using wp_print_styles() in the admin all together, and if it is called in the admin, we return print_admin_styles( $handle );.

I've posted to wpdevel: http://wpdevel.wordpress.com/2011/12/12/use-wp_enqueue_scripts-not-wp_print_styles-to-enqueue-scripts-and-styles-for-the-frontend/. *We* know that *_enqueue_scripts is proper, but most good developers — and anyone following the Codex — will be finding that they've been doing it wrong. If we punt this to 3.3.1, I won't strongly object, but I think we will be making a mistake.

#16 @nacin
13 years ago

  • Milestone changed from 3.3 to 3.3.1
  • Severity changed from critical to major

@nacin
13 years ago

#17 in reply to: ↑ 15 @azaozz
13 years ago

19510.diff looks good for 3.3.1.

Replying to nacin:

...Then come 3.4, we stop using wp_print_styles() in the admin all together, and if it is called in the admin, we return print_admin_styles( $handle );.

Agreed, that seems the best solution for 3.4 (for both styles and scripts).

#18 @nacin
13 years ago

  • Keywords has-patch commit added; dev-feedback removed

#19 @ryan
13 years ago

In [19649]:

Trigger the wp_print_syles action from wp_print_styles() only when handles are not passed. Prevents front-end styles from leaking into the admin. Props nacin. see #19510 for trunk

#20 @ryan
13 years ago

In [19650]:

Trigger the wp_print_syles action from wp_print_styles() only when handles are not passed. Prevents front-end styles from leaking into the admin. Props nacin. see #19510 for 3.3

#21 @ryan
13 years ago

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

We'll create a new ticket for cleaning up wp_print_styles calls in 3.4.

Note: See TracTickets for help on using tickets.