Use Dependency Injection or the like to instantiate wp_xmlrpc_server

[filosofo]

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

[hakre]

Related: #11559

[hakre]

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.

[filosofo]

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.

[Denis-de-Bernardy]

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

[filosofo]

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.

[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.

[filosofo]

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.

[Denis-de-Bernardy]

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.

[Denis-de-Bernardy]

That last line should even be:

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

[Denis-de-Bernardy]

lol, getting tired tonight. even:

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

[filosofo]

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.

[Denis-de-Bernardy]

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.

[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:

$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.

[hakre]

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.

[filosofo]

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

[Denis-de-Bernardy]

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();

[hakre]

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?

[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.

Related: #14820

[filosofo]

How about filter_xmlrpc_server_class.14814.diff ?

[hakre]

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.

[westi]

(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.