[meta] Architecting a dynamic Context Pool

// Let's get ourselves a completely new dependency injection container to
// store stuff into.
$container = new Symfony\Component\DependencyInjection\ContainerBuilder();
...
$id = $context->id;
$container->register($id, $context->class)->...

This is important, because although drupal_container() currently returns a ContainerBuilder instance, that is only temporary, and once we figure out how to take advantage of Symfony compilation, it will return a Container instance only, so we won't be able to call register() on it to add dynamic object ids.

The idea of using a separate ContainerBuilder object at runtime (not at compile time) is intriguing. It certainly provides a rich API for adding definition objects, and handles lazy loading very nicely. OTOH, we potentially lose out on scope inheritance, since a layout within a layout would instantiate a new ContainerBuilder, so would require the overhead of reinitialization of contexts that could otherwise pass from parent layout to child layout unchanged. In IRC, EclipseGc mentioned that we might only use a DIC container for top-level layouts anyway, and handle child layouts in some other way, but if we're wanting a solution that could encompass contexts in general, like Rules, this might be worth more consideration.

I think a good next step would be to come up with some concrete use-cases to write proof of concept code for. Perhaps we can include a simple page layout, an entity layout (view mode) that would be a child of a page layout's block (e.g., a node teaser list), something from Context module, and something from Rules?

xpost

I don't see any problems with this.

The only part that still bugs me is the separate $request and $context/$container variable, which we'll have to pass forward to each and everywhere, deep down the rabbit hole. Like @EclipseGc, I still wonder whether that couldn't be tacked on $request->context, but doesn't really matter for this issue.

Likewise, the hook_response_build() most likely won't be a hook, but rather is what the new kernel will be. It kinda looks like how I'd imagine the new menu_executive_active_handler() in D8.

This proposed implementation pretty much looks like how I imagine that ctools/views/page's as well as rules' implementations look like (I'm guessing here and actually have no idea, but the prototyped logic makes sense to me).

Thus, RTBC, if you'd ask me. :)

+++ b/ContextPool.php
@@ -0,0 +1,109 @@
+        // Build a new array of executed contexts to inject to our condition
+        // plugin. This removes the need for a plugin's actual executing
+        // function to know where in the container a context resides, in favor
+        // of simply injecting the appropriate context instead.
+        $contexts = $container->get($context);
...
+        $test = $plugin->condition($contexts);

I chatted more about this with EclipseGc in IRC, and he pointed out to me that this line contains an important nugget. What this implies is this $container (as opposed to drupal_container()) is strictly an internal object. It does not get passed to plugins/blocks/whatever. Instead the actual fetched context object(s) do. So the recommendation here is to use ContainerBuilder as a private utility by the layout/context system, but ultimately, if at some point we decide it's not the best utility for the job, we can refactor it, without affecting all of Drupal code that uses $context.

So I think this introduces the idea that we have two different kinds of context objects: those that are "injected" into client code, as above, and those that are "located" by client code (e.g., drupal_container()->get('language.interface')). The latter requires static ids (e.g., 'language.interface'), even though the object returned by that id can be dynamic (each block can specify a different interface language). The former doesn't require static ids, since only the internal system uses the ids while client code gets the object itself.

The only part that still bugs me is the separate $request and $context/$container variable, which we'll have to pass forward to each and everywhere

I think some people have been discussing replacing the drupal_container() wrapper function with passing that $container object to all client code, in which case, we may be up to 3 variables: $request, $context, and $container that all get passed forward. I agree that that can become too verbose, but I don't think we need to cross that bridge yet, since we first need to see what the system looks like at all with all these objects and their interdependencies, then we can optimize for syntax convenience.

this introduces the idea that we have two different kinds of context objects: those that are "injected" into client code, as above, and those that are "located" by client code (e.g., drupal_container()->get('language.interface')). The latter requires static ids (e.g., 'language.interface'), even though the object returned by that id can be dynamic (each block can specify a different interface language). The former doesn't require static ids, since only the internal system uses the ids while client code gets the object itself.

Hold on. This makes not much sense to me. We have a DI container. Let's use it.

Both examples logically do the same, merely the time of injection during bootstrap differs.

My expectation is that $container === $container, the same that also contains $container->get('language.interface'). This container will only ever change when we begin a new sub-request. If we don't, it doesn't, because there's no reason for it to be different.

Building additional, separate DICs would make tests significantly harder to write. Let's always make sure to also think from that perspective.

This container will only ever change when we begin a new sub-request.

I don't think that drupal_container() will change when we begin a new subrequest. What's cool about Symfony's container is it has enterScope() and leaveScope() methods that allow the container to remain the same (and therefore, the ids and definitions of objects to remain the same), but for the result of a get() call to vary with "scope". As I understand it, this is actually a really important aspect of Symfony's architecture, and allows for really good performance/memory optimization.

Building additional, separate DICs would make tests significantly harder to write.

As long as the $container proposed in this issue is an internal object only, I don't think we should write tests to it. Instead, we'll need to write tests that $context objects get passed where they need to correctly, and that code that uses $context objects does so correctly. None of this has any knowledge that a $container object is part of the context private implementation.

We have a DI container. Let's use it. Both examples logically do the same, merely the time of injection during bootstrap differs.

Even though dependency injection and service location solve similar problems, they are different. See http://martinfowler.com/articles/injection.html#ServiceLocatorVsDependen.... What matters is how client code gets what it needs. If it receives the objects it needs directly via dependency injection, then from its perspective, there's no container involved (whether or not the code doing the injection uses a container is a private implementation detail that the client code should have no awareness of). OTOH, if the client code needs to ask a container/locator for an object by referring to a static ID, that's a totally different pattern from the perspective of the client code.

I certainly agree that we should not make 2 different containers available to client code, and expect the client code to know which one to ask for what. Code like

$language = drupal_container()->get('language.interface');
$node = $some_other_container->get('context.node');

is a non-starter. But that's not what's being proposed here. I'm not yet clear on exactly what's being proposed in terms of the actual block code that receives $node, but if it's something like:

$language = drupal_container()->get('language.interface');
$node = $this->node;

then I don't see a reason to rule out the use of a private ContainerBuilder object as a way for the layout/context system to populate a block's $this->node property.

Now, I could see some benefit to code that looks like this:

$language = drupal_container()->get('language.interface');
$node = drupal_container()->get('context.node');

But only if static ids like that make sense and can be made to work (and work performantly with the total number of ids we would need for a typical Drupal site). I would see no point to code like:

$language = drupal_container()->get('language.interface');
$context_id = ? // How is $context_id injected?
$node = drupal_container()->get($context_id);

By the way, this discussion reminds me a bit of neclimdul's comment in #1233232-136: Add unified context system to core. For a "display a node" block, it makes sense to me that $node is explicit context, and therefore injection seems logical, whereas $interface_language is implicit context, and therefore location seems logical. But there's a huge gray area of stuff in between, and I think that's where some real use-cases will help a lot in evaluating this stuff.

Also, in IRC, neclimdul and merlinofchaos were questioning why use a ContainerBuilder at all for the injected stuff: if the same subsystem (e.g., layout/context) both calls register() and get() on a private ContainerBuilder object, it's not totally clear how that helps in comparison with that system just instantiating objects directly. EclipseGc pointed out that it might help in that one function in the subsystem calls register() and another one calls get(), and the first doesn't then need to worry about the overhead of instantiating contexts that the administrator might have added in the UI but that no block needs, and especially when we get to contexts derived from "relationships", this can become a not that unlikely scenario. I have a far less thorough understanding of contexts/panels than these guys do, so can't comment on this without seeing more real example code, but if EclipseGc is the one about to write some of this code, and if he thinks using a private ContainerBuilder object will help him, then it makes sense to me to proceed with that.

We discussed this a bit in IRC earlier today. I want to tweak the perspective a bit, because I think this issue is solving the wrong problem.

Remember, everything we're calling a "block" will be, in actuality, a controller; That is, it will be a callable (99% of the time a method) that is called by the kernel, using the ControllerResolver logic routines. That means that information will get mapped from the request path to the explicitly defined parameters on the method. That is:

/node/{node} => myMethod(Node $node) {}
/foo/{article}/{user} => myMethod(Node $article, User $user) /* or */ myMethod(User $user, Node $article);

(Symfony matches on name, not position.)

Also, recall that all blocks will be (or are architected to be, even if in some cases for performance they may not be) subrequests. That is, the standard way to call a block from a layout will be (vis, call a subrequest from a controller):

$request = Request::create('/system/block/{$block_id}/{$nid}');
$response = $kernel->handle($request, Kernel::SUB_REQUEST); // This is what lets all the magical ESI ponies happen.
// Dissect $response and merge it with the current scope somehow.

So for the "normal" sort of parameters aka data sources aka contexts, the routing system will already do everythign for us. No DIC needed.

What remains then is two problems:
1) Figuring out what $nid we're allowed to put into that URL for the subreqest; those rules are more complex than the routing system supports.
2) For lack of a better term, "sideband" context; vis, we want to change the language for a given subrequest.

For problem 2, as effulgentsia notes one of the key reasons we're using the Symfony DIC rather than Pimple is its support for scoping. That is, it lets us do something like this

$request = Request::create('/system/block/{$block_id}/{$nid}');
$container->enterScope('request');
$request->attributes->set('language.content', 'de');
$response = $kernel->handle($request, Kernel::SUB_REQUEST); // This is what lets all the magical ESI ponies happen.
$container->leaveScope('request');
// Dissect $response and merge it with the current scope somehow.

(Note: I haven't actually tested the above, but I believe it's something along those lines.)

That lets anything that is requested from the DIC that depends on the request "reset" and get rederived. In this case, the DIC's language should be pulled off of the request's language attribute, unless overridden by the user's preferences, or whatever logic. In either case, if the controller cares about language, it gets that information from the DIC... the same way it would get anything else.

(In Symfony, that is done by making controllers into services, which means controllers are instantiated throgh the DIC and therefore can get whatever dependencies they need injected in their constructor or whatever... including things like the DB connection to use or the cache object to use. I don't know if we can go all the way to that, but it's definitely not a bad place to end up.)

Important to note: The controller will get those parameters passed to it naked; that is, there is no Context object that wraps everything. At least, not as far as the receiving end is concerned. The receiving end does not know that it's a subrequest rather than the original request direct from the browser. That is by design.

The whole concept of a "context pool" or "data source pool" (I'm still trying to avoid the word "context") exists exclusively to aid in the UI: That's the only purpose it serves. The only remaining question here, and it is still a doosey, is how we allow a controller to specify Drupal-sensitive rules ("this node must be of type Article and must be marked as an OG group" so that the UI only allows users to select sensible objects (a node of type article that has been marked as an OG group) to use to form that subrequest URL. It may or may not make sense to use a DIC container there, I'm not sure right now, but that is all it would be used for.

With the direction we're going with routing, and what Symfony gives us, I don't think we even need a fancier system than that.

As far as what gets passed where, I also don't like the idea of passing 3 objects to every function in all of Drupal. We shouldn't. We won't. As effulgentsia said, that's the basic difference between a Dependency Injection Container and a Service Locator. Originally, way back when WSCCI was still called Butler, I expected us to take a Service Locator approach; basically, it's all I figured I could get away with as a stepping stone to further separating out subystems, and I didn't fully grok DICs yet. As we've evolved the approach, I think we can take a DIC approach more often than not. It takes a bit more thought, but the resulting code is much cleaner and does not have the sort of "every frickin' function in Drupal has $context as its first parameter" problem that sun (rightly) freaks out at. Even if we do pass the DIC into a controller, that's something that should be handled by the DIC when we instantiate the controller object... not something that the path argument / context / data source / whatever it's called system should be dealing with.

Specifically, blocks/controllers will all receive values/data sources direct from the router layer, just like any other controller. One specific type of block/controller, which we're calling a layout block for now, will issue sub-requests to generate the blocks below it. The DIC instance here is an implementation detail of that layout block to help it derive and shuffle data sources around to get to the right sub-block. Since that is confined to the internal implementation of the layout block (and can be changed out for some other approach later without impacting the overall system), it shouldn't cause an issue for WSCCI/kernel/routing. So, carry on!