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?)

Comments

joachim’s picture

Status: Active » Needs review
StatusFileSize
new7.13 KB

Patch.

A brief eyeball test for sanity would be nice before I commit this ;)

joachim’s picture

Urgh whitespace -- some tabs crept in there somehow.

tayzlor’s picture

had 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); ?

joachim’s picture

Status: Needs review » Needs work

We 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!

joachim’s picture

Status: Needs work » Patch (to be ported)
StatusFileSize
new7.67 KB

Committed this patch.

Marking as to be ported, as this could be applied to the 6--1 branch.

joachim’s picture

Status: Patch (to be ported) » Fixed

Committed to 6--1.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.