Switch to anonymous functions for handler registration

In IRC chx noted that passing $context into the closure is not really necessary. We can't really use it, but we will be passing it to getValue() a few microseconds anyway. So let's leave that part off and just do:

use Name*Of*Handler as Handler;

$something = "Something very important";
$context->setHandler($context_key, function () use ($something) {
  return new Handler($something);
});

I have not updated the unit tests for this yet, so those won't work, but this is the engine-level change. It's actually quite small.

However, it occurs to me that catch's request earlier tonight in #1323210-16: Discoverability and documentation would suddenly become possible if, instead of creating the request object internal to the Http handler, we create it once and then pass it into the closure (the $something above), and pass it into multiple handlers that handle parts of the Http request. To wit, someething like:

$request = Request::createFromGlobals();  
$context->setHandler('http', function() use ($request) {
  // Woo, dependency injection! :-)
  return new \Drupal\Context\Handler\HandlerHttp($request);
});
$context->setHandler('http:query', function() use ($request) {
  // This is the same $request object as above.  The exact same.
  return new \Drupal\Context\Handler\HandlerHttp($request);
});
$context->setHandler('http:headers', function() use ($request) {
  // Yep, still the same $request object.
  return new \Drupal\Context\Handler\HandlerHttp($request);
});

(Mind you, I'm not sure I agree with that request but this would make it possible to do. :-) )

The more I think on this, the more I like it.

I meant to post this, really...

I wouldn't call that polymorphism. That's providing 3 separate API calls. (Doing that all in the same method would be bad for DX, as they all take different parameters and do different things with them.)

As to why not to do all of them, just because it's possible to offer 50 ways of doing something doesn't mean it's a good idea. Too many options is bad for DX, as it's harder to document, harder to read, and harder to debug. (Just look at Perl.)

The 3rd item in #5 is almost identical to the 2nd, and the 2nd can do everything the 3rd can do. So eliminate that code path. The first item in #5 is what we have now in the wscci branch, but as others, including pounard, have mentioned previously that results in a poor, difficult to document DX for the handler objects. We previously decided to stick with it because we couldn't come up with something better. Now we have something better. :-)

While there is an extra overhead here for the closure, I think in practice it's negligible. It's one, maybe 2 ZVals. So 40 bytes or so. Not something I'm worried about.

True, this is much closer to the DI systems used by some of the major frameworks. I consider that a good thing. :-) They don't support the stacking setup we do, though, so we'd still need to build that in anyway. While we could build something like this using Pimple (the micro-DI system that the slideshow above talks about), we'd have to extend it enough that we may as well rewrite it.

But it does mean if we adopt Symfony's DI or Pimple or something for service objects/plugins later (a central service locator is on the table to consider later), it's a consistent API model that people can learn and use in both places. So that's even more of an argument in favor of moving to this API approach. I like. :-)

Regarding dynamic/manually-instantiated closures, untrue:
http://us2.php.net/manual/en/language.oop5.magic.php#language.oop5.magic...

Do you have a use case where a closure would NOT be able to handle setting up a handler? I cannot think of one, but I can think of several where it is better than classname-as-string. The only reason to do classname-as-string is so that we can persist configuration to a file. If we're not doing that, then it becomes unnecessary.

We just had a discussion on this topic in the WSCCI office hours channel. Result:

There's three possible registration mechanisms, as described in #5 above.: Static (what we do now, with a class name string and a config array), Pre-instantiated (pass in an object that's already setup), and closure (what this issue is proposing).

For simple cases, where the class doesn't need any configuration, all three are close enough performance-wise and complexity-wise to be a wash. There's little if any practical difference.

The difference comes in the more complex use cases, where an object has lots of configuration and/or external dependencies. In that case, the closure and pre-instantiated approaches are equally expressive and equally complex, in terms of LOC. However, the closure approach will be better performing because the setup code can be delayed by the closure until the first time the handler is requested. Remember, many context handlers may never be used so any setup time spent on those is wasted.

The static approach is actually harder to read in that case, because rather than executable PHP code we have yet-another-big-array. The only advantage here is that, unlike a closure, we can serialize the registration information for the handler and store it in a configuration system. We hope to have one of those eventually (CMI), but right now we do not.

So... What I'm going to propose for the time being is that we switch to the closure approach as the primary mechanism, but keep the static registration around, documented as "you shouldn't use this; it's only here in case we have a use for it later, it may be removed if we cannot find a use for it." That way we can revisit this question later once core is more developed and determine if it's even an appropriate way to do configuration-driven handlers in the first place.

Cool?

yched: If we introduce a service locator for logic objects/plugins, definitely we should look at a more robust syntax. However, I'm unsure if we want/need something that complex right now for context. We couldn't just subclass off of Symfony's DiC because we need the stacking logic, which would require overriding enough of the base class that we probably wouldn't get any serious benefit out of it.

yched: Actually, looking at it again, #12 is just a more elaborate (and arguably more flexible) version of static registration.

Update from today's IRC meeting: We're going to drop the static approach too and go all-closure to keep the initial core patch as slim as possible. Once CMI is in core we can revisit. It's not that much code to add back later if appropriate. :-)

Marking critical as this is a blocker for submitting the next patch to core.

Current patch, also in the repo as a new branch. This converts all handlers to closure, fixes tests, and cleans up the Http handler to take an injected request object. Everything gets a lot cleaner along the way, especially testing of the Http handler.

I think we can now clean up the mock class some, too. I'll try to look into that tomorrow before merging. Meanwhile, review away!

chx, I'm unclear if you're talking about our current pile of inconsistent functions that is the ugly implementation, or the static-handler-registration system in wscci now.

I think the closure approach is, if anything, easier to read (less "utterly complex") than the static version... assuming you understand anonymous functions in PHP. But we learned how to deal with arrays, then learned how to deal with objects, we can learn how to deal with anonymous functions. They're a part of the language, like 'em or lump 'em. The overall impact is very much in mind, and my concerns with this approach are already listed above. We decided it was still a step forward, and of course we can adjust this approach as we convert more things to it and exercise it further.

If you mean Context API vs. what we have now, I'd argue that a single point of reference for request information is less complex, conceptually, than a half-dozen or more functions that few people even know exist. (I didn't even realize until last week that we weren't relying on $_GET['q'] anymore and were just faking it for BC reasons. That blew my mind! It's so obtuse it would never have occurred to me we would do that.) Context API itself is fully self-contained. Once we can convert stuff like the path lookup routines from global functions to something injectable, even those dependencies will be removed. More loosely coupled components reduces complexity, increases flexibility, and improves maintainability.

The logic here is, if anything, less complex than many systems in Drupal already.

catch: In hindsight, I realize that. But since I'd not seen the original issue it blew my mind that we'd make "write into $_GET['q']" something we do unconditionally. It's so weird I couldn't even get my brain to understand what you were saying. :-) That's my point about the Context API being less weird and complex than what is in core now.

I believe what chx is suggesting is to make the following legal as well:

$context->setHandler('path:raw', 'path_raw_factory');

function path_raw_factory() {
  return new Drupal*Context*Handler*HandlerPathRaw();
}

Which would be functionally (as in, have the same effect as) the same as the snippet in #25. The pro of that approach would be that debuggers and backtrace dumps have been able to handle that for years. It would also be very simple to implement in addition to closures.

The cons of that approach that I see would be:

1) It's a different syntax that people could run into and therefore have to grok. I want to try and minimize the number of those developers have to deal with (as above).

2) You'd have to keep in mind making sure that the function is loaded before it gets called. It doesn't have to be loaded when you register it, but it would have to be when you call it. That duality is one of the big problems with our bootstrap process right now, because you could call anything at any time, which means we need functions loaded at all times, which makes bootstrap expensive. (That's one of the reasons we've adopted PSR-0 classes and are emphasizing object-oriented approaches; autoloading.) To be fair, the closure approach needs the code loaded as well, but you can 100% guarantee that it is loaded because it's defined when you register it. So that's one less thing to have to think about.

3) With closures, the code is defined right there where you register it. You don't have to go hunting for it. With functions, they could be defined almost anywhere. It's quite common for functions to be defined "elsewhere in the same 2000 line file" right now, which makes it often hard to follow the logic without a really solid IDE with both forward and back operations.

4) It wouldn't actually improve debugging at all for handlers registered with a closure (obviously). So if you wanted the more traditional approach, you'd have to code it yourself. It doesn't give you any advantage in debugging someone else's code. See point #1.

So I wouldn't really be in favor of supporting named functions here. As both chx and neclimdul note, finding and introspecting a closure object is still possible.

If we remove the type hint, that would implicitly allow any type of callback, true. However, I do not at all agree with making bare functions the default approach, and certainly not with that being what core does. As noted in #29, bare functions have a number of problems, and are less capable than closures because they cannot inherit scoped variables. See #3 for an example of where that would be important.

Really, the only arguments against closures I've seen are:

1) We can't persist them, so they can't get saved to CMI or similar. This is certainly true, and we will be revisiting that question after we have a Config API to work with.

2) They have a less useful stack trace. Also true, but as chx noted in #26 that's a solvable problem (and, given that most will be fairly small, shouldn't really be a problem in the first place in practice).

3) This would be the first significant use of closures in core, so we're not used to them. I actually have a great deal of faith in Drupal developers' ability to pick up new language syntax, especially when it makes the code in question easier to read. :-)

So if "people will follow what core does" (which is generally true), what core does should be closures, not static function names that are harder to read, less flexible, and as far as am aware unique across the PHP world. (As in, harder for any new developers to wrap their heads around.)

I've pushed new code in this branch that removes the Closure type hint, and eliminates most of our one-off dummy test classes. By converting ContextMockHandler directly, with closures, I was able to simplify the unit tests quite a bit. We can probably eliminate the rest of the test classes too shortly, but I wanted to get these knocked out and pushed first.

I will probably merge this branch tomorrow. Right now I have to go shopping. :-)

Branch has been merged. We're almost ready for a core patch!