content_retriever_xmlrpc() is basically a long switch{} statement, where we laboriously check to see if we're using a key, and then pass the parameters we got.
This is itching for refactoring, as currently it's hard to maintain and hard to extend.
I suggest we use func_get_args() and just pass on whatever we received -- the only reason we switch for each method call is to pass the variable number of arguments to xmlrpc().
The alternative would be to move the building of the connection arguments further up, and always call like this:
$args = content_retriever_build_xmlrpc_args()
content_retriever_xmlrpc($args, *what we'd pass here anway*)
but we're still looking at a call_user_func_array() because we don't know how many arguments we might be dealing with. (*sigh* why isn't PHP more like perl?)
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 708472-2.content_distribution.content_retriever_xmlrpc-refactor.patch | 7.67 KB | joachim |
| #1 | 708472.content_distribution.content_retriever_xmlrpc-refactor.patch | 7.13 KB | joachim |
Comments
Comment #1
joachim commentedPatch.
A brief eyeball test for sanity would be nice before I commit this ;)
Comment #2
joachim commentedUrgh whitespace -- some tabs crept in there somehow.
Comment #3
tayzlor commentedhad problems applying the patch to latest 2.x branch but had a quick scan through it.
looks fine to me. only thing i wondered about was the use of -
array_splice($xmlrpc_args, 0, 4, $connection_args);
instead of $args = array_merge($connection_args, $xmlrpc_args); ?
Comment #4
joachim commentedWe need either slice or splice, as the call to func_get_args() gets us everything and we want to discard the first four.
So it's either:
1. func_get_args()
2. take a slice from 4 onwards
3. array_merge $connection_args, slice
or the current approach, which is to splice in the connection args. I guess the current method is a bit more flashy ;) but I can see it's potentially harder to follow. I'll change to explicitly slice.
Thanks for the review!
Comment #5
joachim commentedCommitted this patch.
Marking as to be ported, as this could be applied to the 6--1 branch.
Comment #6
joachim commentedCommitted to 6--1.