Make WordPress Core

#22559 closed defect (bug) (fixed)

Solve for incorrect uses of media_buttons_context

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


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 17 months ago.
22559.2.diff (1.2 KB) - added by nacin 17 months ago.
Refreshed to fix/improve docs.

Download all attachments as: .zip

Change History (9)

comment:1 batmoo17 months ago

  • Cc batmoo@… added

comment:2 nacin17 months 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.

nacin17 months ago

comment:3 nacin17 months 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';
} );

comment:4 nacin17 months 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.

nacin17 months ago

Refreshed to fix/improve docs.

comment:5 nacin17 months ago

  • Keywords commit added

22559.2.diff seems ready to go.

comment:6 ryan17 months ago

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

comment:7 nacin17 months 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.