Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#22559 closed defect (bug) (fixed)

Solve for incorrect uses of media_buttons_context

Reported by: nacin's profile nacin Owned by: nacin's profile nacin
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: has-patch commit
Focuses: Cc:

Description

media_buttons_context is a filter designed to change the label 'Upload/Insert', which is now gone. Unfortunately a lot of code used this as a cheap way to insert media buttons, rather than using the media_buttons filter.

We need to remain compatible here, though it won't be easy. (Such is life.) We need some research into the common ways of modifying this string to insert more things.

If you are a developer reading this, please switch to media_buttons. It'll still work in both 3.4 and 3.5.

This came out of #22186.

Attachments (2)

22559.diff (1.1 KB) - added by nacin 11 years ago.
22559.2.diff (1.2 KB) - added by nacin 11 years ago.
Refreshed to fix/improve docs.

Download all attachments as: .zip

Change History (9)

#1 @batmoo
11 years ago

  • Cc batmoo@… added

#2 @nacin
11 years ago

As said by batmoo in #22186:

... being used incorrectly as you note (specially, using it to add media button instead of the media_buttons action). I don't think we need to support compatibility for the incorrect usages unless there's a compelling reason that people are using the context filter over the action (which I don't have).


Since 3.3, we've been wrapping the text from media_buttons_context with a link, rather including it inside %s. Anyone using this filter since then would needed to have closed an <a> tag then open a new one, all to avoid using the right hook. You'd get:

  • <a href="...">Upload/Insert <a href="">Upload Other Media</a></a>

Here's some research:

Seems like we should entirely drop the filter. That said, we can actually make it work again (as it did pre-3.3) with something like this: echo apply_filters( 'media_buttons_context', '' ); The only issue would be if a plugin was actually trying to close then open an <a> tag inside the filter, to avoid the crazy nesting. But, man, that's even worse.

@nacin
11 years ago

#3 @nacin
11 years ago

  • Keywords has-patch added

Attached patch tries to handle both of these situations:

// Results in nesting
add_filter( 'media_buttons_context', function( $str ) {
    return $str . ' <a href="#">Insert Other Media</a>';
} );
// Avoids nesting
add_filter( 'media_buttons_context', function( $str ) {
    return $str . ' </a> <a href="#">Insert Other Media';
} );

#4 @nacin
11 years ago

  • Severity changed from major to normal

That patch wil leave a stray </a> up front (which wasn't my intention), but it'll work, as it'll at least close the </a> at the end.

And if a plugin was doing this:

add_filter( 'media_buttons_context', function( $str ) {
    return $str . ' </a> <a href="#">Insert Other Media</a>';
} );

They'd have an extra </a>. But they also would have had an extra </a> to begin with.

@nacin
11 years ago

Refreshed to fix/improve docs.

#5 @nacin
11 years ago

  • Keywords commit added

22559.2.diff seems ready to go.

#6 @ryan
11 years ago

Looks good, but I tested only with the snippets here and not with any plugins.

#7 @nacin
11 years ago

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

In 22848:

Back compat for media_buttons_context, which is not the correct filter for adding new media buttons. If you want to add additional media buttons, use the media_buttons action instead. fixes #22559.

Note: See TracTickets for help on using tickets.