public function getValue(array $args = array()) {
  // @todo Make this actually generic.
  $param = $args[0];

This feels like writing perl :/

Is there any way this could be improved?

Comments

Crell’s picture

Crell commented

Where is that code? I think that's in dummy code that is going to get replaced anyway, based on the todo... :-)

joachim’s picture

joachim commented

It's in the handlers that are in butler itself -- context.inc -- and that hence I'm using as a model to follow.

Crell’s picture

Crell commented

If it's in the Http handler, ignore that. It's just a hacky placeholder.

Think of $args like menu arguments. It contains any parts of the context key that were not used for mapping. In this case, the key is http:get:foo, and there's a handler registered at http:get, so it gets array('foo') as its $args.

I'm open to suggestions on a cleaner way of passing that information through, but it seemed like the most logical approach especially since Drupal folks are used to that sort of logic from the menu.

joachim’s picture

joachim commented

Would be nicer if you could write

public function getValue() {
// has no args
}

public function getValue($arg1, $arg2) {

}

which is more like menu callbacks. But I appreciate that that would involve call_user_func_array or whichever one it is that's slow :/

Crell’s picture

Crell commented

Ah, yeah, we'd have to use cufa() there, and cufa() is like 4 times as slow as a function/method call. :-(

pounard’s picture

pounard
Primary language French
commented

Yes, I agree with Crell, magic is bad in most cases, especially in this one.
But I also agree with joachim don't like the actual $args array, I'd personally go for an intermediate solution, pass the trailing context key as a string such as:

1. I ask for "http:get:foo" to the context
2. I have find handler for "http"
3. I pass "get:foo" to the handler, and there the handler can do whatever he want to with.

Most of handlers won't "go:through:an:extensive:hierarchy" so I assume (but I might be wrong) that most calls will end up with a single word and no ":" char in it, I don't like passing array as parameters unless the data is really an array (but info array of path array is something I don't like).

Crell’s picture

Crell commented

The problem there is we have to reserialize the string, and then force each handler to deserialze the string. If someone asks for http:query:foo, then yes the handler will likely get a single argument "foo". However, there's nothing stopping someone from asking for http:query:foo:bar, just to be silly, in which case the handler will break if it assumes that the string is just a single key rather than a multi-part key.

I'd rather pass in something more self-documenting as well, but not at the cost of still-more cufa() calls or forcing more implode()/explode() cycles. Passing a serialized array is no nicer than passing an actual array, and in this case would be slower.

pounard’s picture

pounard
Primary language French
commented

If pass the string (as you call serialized, but not really in fact, we can see it as an indentifier, with or without ":" does not make any difference to me), you may force some handlers to explode() again the string, BUT: I guess that in most cases, handlers won't use the separator at all, so the problem is opposite: you force them to implode() once again.

So what's the balance in this?

Crell’s picture

Crell commented

Huh? They don't need to implode. Just $var = $args[0]. And if a stray $args[1] ended up in there they can safely ignore it.

pounard’s picture

pounard
Primary language French
commented

Maybe "some:arg" as a meaning as a string too. Once again we cannot predict all the use case. But don't worry I'm just messing up a bit, since we actually build the args array inside the handler lookup anyway I guess using it in function signature is the most efficient. I just don't like using array for everything.

Crell’s picture

Crell commented
Status: Active » Closed (works as designed)

Yeah, I'd prefer something more self-documenting but it doesn't seem feasible. So, marking this closed.