WordPress.org

Make WordPress Core

Opened 5 weeks ago

Last modified 4 weeks ago

#44383 assigned defect (bug)

Deprecate the media_buttons_context filter with apply_filters_deprecated()

Reported by: birgire Owned by: zsiderov
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: needs-patch good-first-bug
Focuses: Cc:

Description

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

Attachments (3)

44383.diff (661 bytes) - added by zsiderov 5 weeks ago.
44383.diff
44383.1.diff (7.3 KB) - added by zsiderov 5 weeks ago.
44383.2.diff (664 bytes) - added by zsiderov 5 weeks ago.

Download all attachments as: .zip

Change History (15)

@zsiderov
5 weeks ago

44383.diff

#1 @zsiderov
5 weeks ago

Please find the attached changes above.

Let me know your thoughts.

Thanks, Zhivko

#2 @birgire
5 weeks ago

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.

#3 @zsiderov
5 weeks ago

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

#4 @audrasjb
5 weeks ago

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
5 weeks ago

#5 @zsiderov
5 weeks ago

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

#6 @zsiderov
5 weeks ago

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

Last edited 5 weeks ago by zsiderov (previous) (diff)

#7 @birgire
5 weeks ago

@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 :)

Last edited 5 weeks ago by birgire (previous) (diff)

#8 @birgire
5 weeks ago

  • Owner set to zsiderov
  • Status changed from new to assigned

@zsiderov
5 weeks ago

#9 @zsiderov
5 weeks ago

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.

#10 @birgire
4 weeks ago

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?

#11 @zsiderov
4 weeks ago

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

#12 @zsiderov
4 weeks ago

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?

Note: See TracTickets for help on using tickets.