WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#14814 closed defect (bug) (fixed)

Use Dependency Injection or the like to instantiate wp_xmlrpc_server

Reported by: filosofo Owned by:
Milestone: 3.1 Priority: normal
Severity: normal Version: 3.1
Component: XML-RPC Keywords: has-patch
Focuses: Cc:

Description (last modified by filosofo)

Filter the wp_xmlrpc_server class or use some other means so that plugin authors can extend the XML-RPC server functionality.

Attachments (2)

refactor-xml-rpc-class.14814.diff (186.6 KB) - added by filosofo 9 years ago.
filter_xmlrpc_server_class.14814.diff (397 bytes) - added by filosofo 9 years ago.

Download all attachments as: .zip

Change History (25)

#1 @filosofo
9 years ago

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

#2 @hakre
9 years ago

Related: #11559

#3 follow-up: @hakre
9 years ago

I like the idea.

Maybe the server should have an init function so to load it's methods array that can be modified by an extending class w/o calling the hooks in the constructor. Or vice versa.

#4 in reply to: ↑ 3 @filosofo
9 years ago

Replying to hakre:

Maybe the server should have an init function so to load it's methods array that can be modified by an extending class w/o calling the hooks in the constructor. Or vice versa.

The parent IXR class has a setCallbacks method. I agree that something like that is a good idea.

#5 follow-up: @Denis-de-Bernardy
9 years ago

Maybe be we could add a lazy-loaded dependency injection...

#6 in reply to: ↑ 5 @filosofo
9 years ago

Replying to Denis-de-Bernardy:

Maybe be we could add a lazy-loaded dependency injection...

Do you mean for the callbacks associated with particular XML-RPC methods?

I'm not sure exactly what you have in mind, but it's probably worth having a separate ticket for it.

I'm trying to do something really simple with this ticket, basically just cut-n-paste except for the filter on the server class name.

#7 follow-up: @josephscott
9 years ago

I appreciate the idea, but I'm not sure this will actually solve your use case very well. Nearly all of the RPC functions will return an XML-RPC error if the login fails, which won't work for JSON RPC responses.

#8 in reply to: ↑ 7 @filosofo
9 years ago

Replying to josephscott:

I appreciate the idea, but I'm not sure this will actually solve your use case very well. Nearly all of the RPC functions will return an XML-RPC error if the login fails, which won't work for JSON RPC responses.

Actually, I have the plugin set up to handle IXR_Error error objects (and WP_Error objects) returned from the server methods. (Currently I have a WP_JSON_RPC_Server class that extends IXR_Server; it would be nice if I could extend wp_xmlrpc_server instead.)

I'm working on a sample implementation, and when I'm done I'll post a link to the plugin on the JSON RPC ticket so you can see it in action.

#9 follow-up: @Denis-de-Bernardy
9 years ago

Replying to filosofo:

Replying to Denis-de-Bernardy:

Maybe be we could add a lazy-loaded dependency injection...

I'm not sure exactly what you have in mind, but it's probably worth having a separate ticket for it.

Googling helps :-)

http://www.google.com/search?q=dependency+injection+php

I'm trying to do something really simple with this ticket, basically just cut-n-paste except for the filter on the server class name.

I had something similar to what's done in the patch, but more like:

# in plugin file:
function my_xmlrpc_server($class) {
    require_once 'my_xmlrpc_server.php';
    return $my_xmlrpc_server;
}
add_filter('xmlrpc_server_class', 'my_xmlrpc_server');

# at the end of the default xmlrpc server file:
$xmlrpc_server_class = apply_filters('xmlrpc_server_class', 'xmlrpc_server');
$xmlrpc_server = new $xmlrpc_server_class;

The file is lazy loaded already, so it's not much of a big deal.

In other apps, the lazy loaded part generally looks like this:

class Foo {
  static $adapters = array('storage' => 'default');

  ...

  function doStuff() {
    $storage = Storage::get(self::$adapters['storage]);
  }
}

Foo::config(array('storage' => 'memcache')); // override the default

class Storage { // typically inherits from a base class that does this
  static $adapters =array(
    'default' => 'MemoryStorage',
    'memcache' => 'MemcacheStorage'
    );
  static function get($adapter) {
    $class = self::$adapters[$adapter];
    // only autoload and instantiate the class if it gets used
    return is_object($class) ? $class : new $class;
  }
}

the same could be done for quite a few other classes. e.g. wpdb, locale-related stuff, etc.

#10 @Denis-de-Bernardy
9 years ago

That last line should even be:

return is_object($class) ? $class : $class = new $class;

#11 @Denis-de-Bernardy
9 years ago

lol, getting tired tonight. even:

return is_object($class) ? $class : self::$adapters[$adapter] = new $class;

#12 in reply to: ↑ 9 ; follow-up: @filosofo
9 years ago

Replying to Denis-de-Bernardy:

Googling helps :-)

It might, if I were wondering what dependency injection is.

Really, I'm not principally interested in replacing the wp_xmlrpc_server class, as I can already (and am forced to) do that; I want to extend it. That part of the patch was a offhand enhancement, which maybe I shouldn't have included for simplicity's sake.

You're talking about something that's a separate issue from the point of this ticket, something with more wide-ranging and fundamental implications for how WP handles dependencies.

#13 in reply to: ↑ 12 @Denis-de-Bernardy
9 years ago

Replying to filosofo:

Really, I'm not principally interested in replacing the wp_xmlrpc_server class, as I can already (and am forced to) do that; I want to extend it. That part of the patch was a offhand enhancement, which maybe I shouldn't have included for simplicity's sake.

Please bear with me. My suggestion is merely a bit more versatile than what the patch suggests. Consider:

if ( ! empty( $xmlrpc_server_class ) && class_exists( $xmlrpc_server_class ) ) {
    $wp_xmlrpc_server = new $xmlrpc_server_class;
    $wp_xmlrpc_server->serve_request();
}

vs. my suggested, less conflict prone and even more pluginable suggestion:

$xmlrpc_server_class = apply_filters('xmlrpc_server_class', 'xmlrpc_server');
$wp_xmlrpc_server = new $xmlrpc_server_class;
$wp_xmlrpc_server->serve_request();

in the suggested patch, anyone can override the global and whoever comes last in alphabetical plugin order trumps. in my own suggest, whoever goes last as far as priority is concerned does. extending or whatever has nothing to do with it. you can extend in either case as needed.

#14 follow-up: @filosofo
9 years ago

I think you're just thrown off by the way the diff is rendered. Apply my patch, and you'll see the following at the bottom of xmlrpc.php:

$xmlrpc_server_class = apply_filters('wp_xmlrpc_server_class', 'wp_xmlrpc_server');

if ( ! empty( $xmlrpc_server_class ) && class_exists( $xmlrpc_server_class ) ) {
        $wp_xmlrpc_server = new $xmlrpc_server_class;
        $wp_xmlrpc_server->serve_request();
}

The empty check is to allow someone to abort the server response with a callback that returns false.

#15 in reply to: ↑ 14 @hakre
9 years ago

Replying to filosofo:

...
The empty check is to allow someone to abort the server response with a callback that returns false.

Then if ( class_exists( $xmlrpc_server_class ) ) should be sufficient, because class_exists( (bool) FALSE ) will be false as well. No need to check for ! empty($xmlrpc_server_class) additionally.

#16 follow-ups: @filosofo
9 years ago

I'm assuming empty is much faster than class_exists. Do you think that's unreasonable?

#17 in reply to: ↑ 16 @Denis-de-Bernardy
9 years ago

Replying to filosofo:

I think you're just thrown off by the way the diff is rendered. Apply my patch, and you'll see the following at the bottom of xmlrpc.php:

I had missed that indeed. Nevermind. :-P

I'm assuming empty is much faster than class_exists. Do you think that's unreasonable?

As far as using empty() instead of checking the variable, I'd say yes.

// fast, but raises notices if $var is not declared:
$var && foo();
// fast, without raising notices:
isset($var) && $var && foo()
// slower, but less verbose:
!empty($var) && foo();

#18 in reply to: ↑ 16 @hakre
9 years ago

Replying to filosofo:

I'm assuming empty is much faster than class_exists. Do you think that's unreasonable?

I have not run any tests on that micro optimization. I suggested it mainly because it's not necessary. Allways measure btw. :) It can be faster, true. Is it expected to be non-existant normally? No! So It's an extra check for nothing - even when empty() is faster then class_exists(), it's allways slower because the standard WP installation expects a XMLRPC Server class. Doesn't it?

#19 @filosofo
9 years ago

  • Description modified (diff)
  • Milestone changed from 3.1 to Future Release
  • Owner filosofo deleted
  • Summary changed from Allow WP XML-RPC Library to be Extended to Use Dependency Injection or the like to instantiate wp_xmlrpc_server

All of this is valid discussion fodder, but not what I was really hoping to accomplish. I am changing the title of the ticket to reflect its conversational trajectory more accurately.

Related: #14820

#20 follow-up: @filosofo
9 years ago

How about filter_xmlrpc_server_class.14814.diff ?

#21 in reply to: ↑ 20 @hakre
9 years ago

Replying to filosofo:

All of this is valid discussion fodder, but not what I was really hoping to accomplish. I am changing the title of the ticket to reflect its conversational trajectory more accurately.

I have no problems with the general improvement and to move the file out. That's something that has been done in the past (see: #11559) so I think this can be done here as well. This improves extensibility.

+1 for split

Providing a filter to overload the class is something pretty useful as well.

+1 for adding a filter.

Anything on top of this can be discussed in another iteration from what I see. No need to slow this change down.

#22 @westi
9 years ago

  • Milestone changed from Future Release to 3.1

#23 @westi
9 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [15648]) Complete the move to a seperate class file for #14820 props filosofo.
Add support for replacing the class used. Fixes #14814 props filosofo.

Note: See TracTickets for help on using tickets.