Deprecate the media_buttons_context filter with apply_filters_deprecated()

[birgire]

Within the media_buttons() function, in the wp-admin/includes/media.php file, we have the deprecated filter:

/**
 * Filters the legacy (pre-3.5.0) media buttons.
 *
 * Use {@see 'media_buttons'} action instead.
 *
 * @since 2.5.0
 * @deprecated 3.5.0 Use {@see 'media_buttons'} action instead.
 *
 * @param string $string Media buttons context. Default empty.
 */
 $legacy_filter = apply_filters( 'media_buttons_context', '' );

I think we should consider using the ​apply_filters_deprecated() instead, with the $messages input, explaining it further that an action should be used instead.

Similar deprecation ticket: #44341

[zsiderov]

44383.diff

[zsiderov]

Please find the attached changes above.

Let me know your thoughts.

Thanks,
Zhivko

[birgire]

Thank you for the patch @zsiderov

Notes on 44383.diff​ :

• The $array( '' ) should be an array.
• Should 'Please use "media_buttons" action instead.' be translatable?
• Did you test it? If so how did the full notice look like? It would be nice to see that.

[zsiderov]

Thanks for the feedback @birgire.

• I will change "$array( ' ' )" to "array( ' ' )".
• I am not sure how to make 'Please use "media_buttons" action instead.' to be translatable? What is the best way to do so?
• I will google it to see how to trigger the media_buttons filter and attach a screenshot of the notice. If you know on top of your head a quick way to test it, let me know.

Thanks,
Zhivko

[audrasjb]

Hi @zsiderov , welcome to WordPress Trac and thanks for your patch :)

I guess you should use __() function to localize this string.
Also, array() should do the job.

$legacy_filter = apply_filters_deprecated( 'media_buttons_context', array(), '3.5.0', false, __( 'Please use "media_buttons" action instead.' ) );

Cheers,
Jb

[zsiderov]

You are ⭐️ Thanks for the help @audrasjb 😊 I have attached the new diff.

I couldn't trigger the message notification for a screenshot 😞 The things I have tried are below.

add_action('media_buttons_context','jcorgbuttons');

function jcorgbuttons($context) {
	return $context.="<a href='javascript:void(0)'>Hello</a>";
}

and :

function add_my_media_button() {
  echo '<a href="#" id="insert-my-media" class="button">Add my media</a>';
}

add_action('media_buttons', 'add_my_media_button', 15);

I am open to suggestions.

Thanks,
Zhivko

[zsiderov]

Hmm, looks like my rebase has triggered other changes. Was I not supposed to rebase from master?

[birgire]

@zsiderov thanks for the updated patch. Some more notes:

By translatable I had what @audrasjb wrote, in mind.

The context is filter, not action, so we could test it with:

add_filter( 'media_buttons_context', 'jcorgbuttons' );

function jcorgbuttons( $context ) {
    return $context .= "<a href='javascript:void(0)'>Hello</a>";
}

In /wp-includes/default-filters.php we have the media_buttons() function hooked into media_buttons actions:

add_action( 'media_buttons', 'media_buttons' );

We can visit an Edit Post page in the backend, to trigger the deprecate error.

The input argument array() will need to be array( '' ), as the previous filter input is '', not null. Also it's necessary, to avoid a PHP notice.

Then we can consider what would be a suitable $replacement argument.

PS:

I noticed a bug in core.

The default $replacement input value for apply_filters_deprecated is false, but the default $replacement input value for
_deprecated_hook is null and has only as ! is_null( $replacement ) check.

It would make sense to let those two functions check the same value (either both use false or both null).

See those two function in core here:

https://core.trac.wordpress.org/browser/tags/4.9.6/src/wp-includes/functions.php#L4063

https://core.trac.wordpress.org/browser/tags/4.9.6/src/wp-includes/plugin.php#L597

PPS:

ups my comment got posted before I had finished it, so I had to edit :)

[zsiderov]

Thanks @birgire and @audrasjb for the help! I have attached the new file.

@birgire I wasn't sure what to do about the bug you mentioned about $replacement being used in apply_filters_deprecated and _deprecated_hook differently. I left it as false as this is what the is also mentioned in ​https://developer.wordpress.org/reference/functions/apply_filters_deprecated/

Should this go as a separate PR if it needs to be changed? I can see the docs talking about it as being false as default argument. So if a change is going to be made - will the change need to be reflected in the docs as well? Alternatively both functions can check for null and false which will help with it since we only want to hit the if statement when this is only true - so this brings the question should we not be checking for null and false in the first place?

I failed miserably again with viewing the error and creating a screenshot. My assumption was that when I open the dashboard in the browser and navigate to Edit Post I will see it as a message there 😎 My lack of knowledge of WordPress mechanics is pretty obvious there - not sure how to trigger it i.e. visit the Edit Post page in the back-end  😞

What will you guys recommend to do first before continue with any other tickets? Create a theme and plugin? Something else? I really need to get familiar with WordPress mechanics and common behaviour first as this bug is just one line change and I have troubles making such a simple change let alone something more complex.

[birgire]

Awesome to hear you've decided to dive into WordPress development.

There are also many ways to help with tickets; like patching, testing, refreshing, documentation, designing, etc.

I'm sure creating themes and plugins is a good way to better understand WordPress mechanics.

Also helping out on support forums, can enhance debugging skills.

The ​Core Contributor Handbook is also very helpful, regarding core contributions.

---

Maybe this can help how to approach the PHP errors/notices:

​https://codex.wordpress.org/Debugging_in_WordPress

There are also plugins, like Query Monitor, that display it in an easy to read manner.

---

I have already created a another ticket #44406 regarding the input mismatch.

---

It seems that using 'media_buttons' for the replacement input, is currently more suitable than using false or null.

@zsiderov maybe you could checkout those outputs, so we have them documented here?

[zsiderov]

Sweet, thx for the tips @birgire. I will have a look at the outputs and share them back 😊

[zsiderov]

Does any of you know if I want to contribute to JavaScript/React where I can find more information about it? Will I filter the bug tracker or?

[trishkedi]

@birgire I have changed replacement output from 'false' to 'media_buttons'

[hellofromTonya]

Hello all,
This filter was deprecated in [46684] #48255. Marking this ticket as a duplicate of that ticket. Thank you everyone for your contribution!