Opened 13 years ago
Closed 13 years ago
#14814 closed defect (bug) (fixed)
Use Dependency Injection or the like to instantiate wp_xmlrpc_server
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | 3.1 | Priority: | normal |
Severity: | normal | Version: | 3.1 |
Component: | XML-RPC | Keywords: | has-patch |
Focuses: | Cc: |
Description (last modified by )
Filter the wp_xmlrpc_server
class or use some other means so that plugin authors can extend the XML-RPC server functionality.
Attachments (2)
Change History (25)
#3
follow-up:
↓ 4
@
13 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
@
13 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.
#6
in reply to:
↑ 5
@
13 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:
↓ 8
@
13 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
@
13 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:
↓ 12
@
13 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
@
13 years ago
That last line should even be:
return is_object($class) ? $class : $class = new $class;
#11
@
13 years ago
lol, getting tired tonight. even:
return is_object($class) ? $class : self::$adapters[$adapter] = new $class;
#12
in reply to:
↑ 9
;
follow-up:
↓ 13
@
13 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
@
13 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:
↓ 15
@
13 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
@
13 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:
↓ 17
↓ 18
@
13 years ago
I'm assuming empty is much faster than class_exists. Do you think that's unreasonable?
#17
in reply to:
↑ 16
@
13 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
@
13 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
@
13 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
#21
in reply to:
↑ 20
@
13 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.
Related: #11559