WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#29803 closed defect (bug) (wontfix)

Silent failure when wp_embed_register_handler() has a non-public callback

Reported by: paulschreiber Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Embeds Keywords: has-patch
Focuses: Cc:

Description

WordPress fails silently when wp_embed_register_handler() has a non-public callback

If you have a class like this:

class Stuff {
   public static function hooks() {
     wp_embed_register_handler( 'foobar', $regex, array( __CLASS__, 'embed_handler' ) );
   }

  private static function embed_handler( $matches, $attr, $url, $rawattr ) {
    //.... do stuff
  }
}

And you run

Stuff::hooks();

Your handler is never registered and no error is shown or logged.

By contrast, add_action() checks for this condition and logs an error:

PHP Warning:  call_user_func_array() expects parameter 1 to be a valid callback, cannot access private method Stuff::enqueue_scripts() in /srv/www/wp/wp-includes/plugin.php on line 505

Attachments (1)

29803.diff (2.5 KB) - added by leewillis77 5 years ago.

Download all attachments as: .zip

Change History (5)

#1 @leewillis77
5 years ago

  • Keywords dev-feedback needs-unit-tests added

Actually, add_action() doesn't check for the callability of the function - hence PHP throwing the warning.

The embed code specifically checks that the callback is callable, and doesn't try to call it if not, so no error is thrown. I'm pretty sure we wouldn't want to remove that check, and have the embed code start throwing warnings where it didn't before.

It's possible we could add a check when the handler is registered - however, just because the function isn't callable at that point doesn't mean it won't be at the time it's needed. Perhaps making wp_embed_register_handler() return false if the callback isn't callable but registering it anyway, and relying on the caller to notice the return value that would be acceptable (Patch attached), not 100% sure that "smells" right though?

If someone's happy that's an acceptable approach I'll expand the patch to include tests.
We could also consider the fact that do_action() throws a warning to be the bug and fix that up

@leewillis77
5 years ago

#2 @DrewAPicture
5 years ago

  • Component changed from General to Embeds

#3 @wonderboymusic
5 years ago

  • Keywords has-patch added; dev-feedback needs-unit-tests removed

Lemme think about this. The embed handler not working should be enough of a sign that that your method has problems. I would argue against making classes where every method is static and a few are marked private for fun. Action handlers have to be public, always.

#4 @helen
5 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

I agree with the above observations.

Note: See TracTickets for help on using tickets.