Inspired by http://groups.drupal.org/node/232188. This can be moved to a core issue follow-up if we don't want to tackle it now, but posting here initially, since the kernel patch isn't in yet.

Files: 
CommentFileSizeAuthor
#47 1595146-kernel-di-47.patch11.04 KBNiklas Fiekas
PASSED: [[SimpleTest]]: [MySQL] 37,058 pass(es).
[ View ]
#43 1595146-kernel-di-32.patch10.98 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1595146-kernel-di-32_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#38 1595146_38_kernel_di.patch10.91 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 37,014 pass(es), 11 fail(s), and 8 exception(s).
[ View ]
#32 1595146-kernel-di-32.patch10.98 KBNiklas Fiekas
PASSED: [[SimpleTest]]: [MySQL] 37,015 pass(es).
[ View ]
#32 1595146-kernel-di-32-interdiff.txt1.89 KBNiklas Fiekas
#26 1595146-kernel-di-26.patch11.29 KBNiklas Fiekas
PASSED: [[SimpleTest]]: [MySQL] 37,002 pass(es).
[ View ]
#26 1595146-kernel-di-26-interdiff.txt4.56 KBNiklas Fiekas
#23 1595146-kernel-di-23.patch10.72 KBNiklas Fiekas
FAILED: [[SimpleTest]]: [MySQL] 36,884 pass(es), 5 fail(s), and 1 exception(s).
[ View ]
#21 1595146-kernel-di-21.patch9.26 KBNiklas Fiekas
PASSED: [[SimpleTest]]: [MySQL] 36,888 pass(es).
[ View ]
#7 1595146_6_kernel.patch1.56 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#6 kernel.patch1.8 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 36,693 pass(es).
[ View ]
kernel-swappable.patch2.04 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch kernel-swappable.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Title:Allow DrupalKernel to be swappableLoad the Kernel from the DI Container
Project:WSCCI» Drupal core
Version:» 8.x-dev
Component:Code» base system
Issue tags:+WSCCI, +Dependency Injection (DI), +kernel-followup

I do not expect the kernel to be swappable, and in fact we may go as far as eliminating it entirely in favor of the Symfony native kernel once we can refactor stuff out to the DIC. :-) That said, I do want to see the kernel in the DIC. So, refiling and retitling.

Status:Needs review» Needs work

The last submitted patch, kernel-swappable.patch, failed testing.

After reading the additional comments in the linked post, I should add: Any replacement of the kernel would still have to conform to the kernel interface. If it didn't, all sorts of things would break horribly, and you'd also be bypassing all of Drupal. At that point, don't use Drupal. Go use Symfony directly, or Silex, or go all the way to Lithium, etc. Moving the kernel into the DIC is a cleanliness thing only, not an extension point, and should not be confused for one.

Shouldn't it be the other way around? Have a $container property within the Kernel. That way, we could have two Kernel instances running at a time without conflicting container services.

$kernel->getContainer()... or similar.

If you look at Silex, you see the main Silex class inherits from the container. Having the container be an instance within the Kernel would accomplish the same thing.

Passing the container into an object from the container makes it a Service Locator, which has a different set of ups and downs. There are cases where we could do that (pass the DIC into the kernel in the constructor), but I'd rather not if we can avoid it as it makes unit testing harder.

Silex is a different design and architecture than Symfony full stack or Drupal, and frankly I don't agree with the way it merges the kernel and DIC anyway. :-)

Status:Needs work» Needs review
StatusFileSize
new1.8 KB
PASSED: [[SimpleTest]]: [MySQL] 36,693 pass(es).
[ View ]

Was attempting to make a ContainerSubscriber event, but we need the container throughout the bootstrap, so it was failing at DRUPAL_BOOTSTRAP_CODE. For now, I believe this is the only way it makes a little bit of sense.

StatusFileSize
new1.56 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

This patch revises #6 by removing the use statement for DependencyInjection/Reference and removes an extra blank line.

Status:Needs review» Needs work
Issue tags:-WSCCI, -Dependency Injection (DI), -kernel-followup

The last submitted patch, 1595146_6_kernel.patch, failed testing.

Status:Needs work» Needs review

#7: 1595146_6_kernel.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1595146_6_kernel.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+WSCCI, +Dependency Injection (DI), +kernel-followup

#6: kernel.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

OK, then I guess that use statement was important after all. #6 is RTBC in my book.

kernel service should be marked synthetic.

Rob, care to give a bit more explanation? It's not clear what failed based on your comment.

@marfillaster: can you point to an example of how that's done. This is the best I could find:
http://api.symfony.com/v2.0.6/Symfony/Component/DependencyInjection/Defi...

Status:Reviewed & tested by the community» Needs work

Seems like there's still some things we have to discuss here. It would be great if we could pass the Kernel to objects that need it rather than depending on it being in the container.

I was having a look at Symfony's Kernel and see that it also has the Container object within their Kernel itself, which leads me to believe that's the correct architecture behind it. When it comes to SimpleTest, we will want multiple kernels running simultaneously, which means we'll want a one-to-one kernel->container relation.

drupal_container() is nice, but it's a static container. If we have multiple kernels, we'll still have one container. If the container object is within kernel, we avoid that issue. The container even references 'kernel' within it, so it is similar to what was originally suggested here, but more architecturally sound.

Also:

the boot function demonstrates a way to share a container between multiple items.

/**
     * Boots the current kernel.
     *
     * @api
     */
    public function boot()
    {
        if (true === $this->booted) {
            return;
        }
        // init bundles
        $this->initializeBundles();
        // init container
        $this->initializeContainer();
        foreach ($this->getBundles() as $bundle) {
            $bundle->setContainer($this->container);
            $bundle->boot();
        }
        $this->booted = true;
    }

By putting the kernel into a DIC container are we introducing a Drupalism? Judging by this level of architectural support for the kernel to have a DI container seems to indicate that the Kernel is not a service to be registered but the heart / root of a website that should be relied on to control it's operation. How do Symfony developers handle this problemset?

Interesting information about scopes as well: https://groups.google.com/d/msg/symfony-devs/Uq6dC09O8cg/iU_q3GwMhRUJ

Title:Load the Kernel from the DI ContainerLoad the HttpKernel from the DI Container

Re #16, @Rob Loach:

I spend some time talking to Fabien about this issue at Symfony Live. There is in fact some confusion here, as there are two different classes involved:

HttpKernel: This is the core kernel routing logic that ties everything together. We are extending it, but really we shouldn't be. Everything that's currently in the constructor we should move out and then inject via a DIC.

Kernel: This is a completely separate class that is also in the HttpKernel package. It is the "operating system" of the program. It is itself responsible for creating the DIC object, for various other setup routines, etc. In essence, this class replaces the global scope allowing a complete "reboot" of everything without any globals whatsoever. That's useful for functional testing.

What we want to do is put the HttpKernel into the DIC, and remove our customizations. They should just be injected. Then we arguably want to subclass Kernel and override the buildContainer() and dumpContainer() methods, which assume use of the Symfony Config component. Instead we replace them with whatever our container managing logic is going to be. Probably they'd serve an equivalent purpose to drupal_flush_all_caches().

So step 1 is to clean up the HttpKernel and factor our our customizations, then put that all into the DIC where it is now. Then we can talk about modules registering additional listeners through the DIC (#1599108: Allow modules to register services and subscriber services (events)), and possibly wrapping it all in a Kernel subclass.

Status:Needs work» Needs review
StatusFileSize
new9.26 KB
PASSED: [[SimpleTest]]: [MySQL] 36,888 pass(es).
[ View ]

This one uses a factory method to create a HttpKernel with a few subscribers and removes DrupalKernel. However I am not sure if there's a reasonable way to avoid creating a factory method with the ContainerBuilder or (if not) if it would be better to make the factory a different class.

Status:Needs review» Needs work

+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
@@ -27,5 +44,46 @@ class ContainerBuilder extends BaseContainerBuilder {
+    $dispatcher = new EventDispatcher();
+    $resolver = new ControllerResolver();

Let's go ahead and move these to their own container entries. We'll have to do that anyway, so let's do it now. Then we configure the DIC to inject both of them into the kernel on instantiation.

Status:Needs work» Needs review
StatusFileSize
new10.72 KB
FAILED: [[SimpleTest]]: [MySQL] 36,884 pass(es), 5 fail(s), and 1 exception(s).
[ View ]

Looks like in order to do that, we have to remove the kernel argument from the ExceptionController constructor and use the DI container instead. Otherwise that would make a recursive dependency: Kernel requires event dispatcher, event dispatcher wants the exception controller attached, exception controller wants the kernel.

I am now using a factory method just for the dispatcher, to attach all the listeners. Should we add all the listeners to the DI container, too, instead, and reference them?

Status:Needs review» Needs work

The last submitted patch, 1595146-kernel-di-23.patch, failed testing.

+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
@@ -27,5 +43,51 @@ class ContainerBuilder extends BaseContainerBuilder {
+    $this->register('drupal.kernel', 'Symfony\Component\HttpKernel\HttpKernel')

This should be called drupal.httpkernel, not drupal.kernel, because the Kernel is something else that we're not using yet. (It's confusing, unfortunately.)

Also, I don't know that we really need to prefix everything with drupal. It's all in the Drupal DIC, so that should make it obvious. Someone needs to explain to me, with examples, why we should still prefix everything under the sun with an extraneous five characters.

+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
@@ -27,5 +43,51 @@ class ContainerBuilder extends BaseContainerBuilder {
+    $matcher = new LegacyUrlMatcher();
+    $dispatcher->addSubscriber(new RouterListener($matcher));
+
+    $negotiation = new ContentNegotiation();
+
+    // @todo Make this extensible rather than just hard coding some.
+    // @todo Add a subscriber to handle other things, too, like our Ajax
+    //   replacement system.
+    $dispatcher->addSubscriber(new ViewSubscriber($negotiation));
+    $dispatcher->addSubscriber(new AccessSubscriber());
+    $dispatcher->addSubscriber(new MaintenanceModeSubscriber());
+    $dispatcher->addSubscriber(new PathSubscriber());
+    $dispatcher->addSubscriber(new LegacyRequestSubscriber());
+    $dispatcher->addSubscriber(new LegacyControllerSubscriber());
+    $dispatcher->addSubscriber(new FinishResponseSubscriber());
+    $dispatcher->addSubscriber(new RequestCloseSubscriber());

Just highlighting that we will move these out later when we add Bundle support, as then we can use a container aware dispatcher as documented elsewhere in #1599108-14: Allow modules to register services and subscriber services (events) . This is fine for now, but will change further later.

+++ b/core/lib/Drupal/Core/ExceptionController.php
@@ -119,7 +106,7 @@ class ExceptionController {
+      $response = drupal_container()->get('drupal.kernel')->handle($subrequest, HttpKernel::SUB_REQUEST);

I wonder if we can avoid this by using ContainerAwareInterface. Can you check if the DIC will auto-inject itself if that interface is present? That seems like the better way to do it. (Even if it's not automatic, we can wire that up in the DIC manually.)

I really really want to stay firm on avoiding singletons and function calls in objects. That makes testing way harder.

Status:Needs work» Needs review
StatusFileSize
new4.56 KB
new11.29 KB
PASSED: [[SimpleTest]]: [MySQL] 37,002 pass(es).
[ View ]

Easy things first: Fixed the naming. The drupal. prefix is (probably) indeed unnescessary.

No magic happening with ContainerAwareInterface, so I am manually injecting the container into the exception controller. During the research I found ContainerAwareEventDispatcher, which looks like an intresting consideration for the next step, because it's made to make the event listeners services.

That looks better but I'd like to get someone from Symfony in here to verify that we're not abusing the DIC horribly before we commit this.

is there a reason to longer "use HttpKernelInterface" to reference "HttpKernelInterface::SUB_REQUEST"?

+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.phpundefined
@@ -27,5 +43,53 @@ class ContainerBuilder extends BaseContainerBuilder {
+   * Creates an EventDispatcher for the HttpKernel. Factory method.
+   *
+   * @return Symfony\Component\EventDispatcher\EventDispatcher
+   *   An EventDispatcher with the default listeners attached to it.
+   */
+  public static function getKernelEventDispatcher($container) {
+    $dispatcher = new EventDispatcher();
+
+    $matcher = new LegacyUrlMatcher();
+    $dispatcher->addSubscriber(new RouterListener($matcher));
+
+    $negotiation = new ContentNegotiation();

Not sure how I feel about using a static factory for this, but am warming up to it as it doesn't hold any static properties, and is only invoked through the Container itself.

@Rob Loach: Yes. Also, it is supposed to be an itermediate step, so that we can get other issues that depend on using the container to get the kernel unblocked. There seam to be various follow-up options to get the event dispatcher created in a better way.

Status:Needs review» Needs work

+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.phpundefined
@@ -27,5 +43,53 @@ class ContainerBuilder extends BaseContainerBuilder {
+    $this->setDefinition('dispatcher', new Definition('Symfony\Component\EventDispatcher\EventDispatcher', array(new Reference('service_container'))))

Can we make this be:

$this->register('dispatcher', 'Symfony\Component\EventDispatcher\EventDispatcher')
  ->addArgument(new Reference('service_container')
  ->setFactory('Drupal\Core\DependencyInjection\ContainerBuilder')
  ->setFactoryMethod('getKernelEventDispatcher');

... for consistency's sake.

Status:Needs work» Needs review
StatusFileSize
new1.89 KB
new10.98 KB
PASSED: [[SimpleTest]]: [MySQL] 37,015 pass(es).
[ View ]

@cosmicdreams: Oh, yes, thanks, we can (or almost). Didn't know that sugar.

Thanks, that makes it much easier to read.

So in the future are we just going to be registering more to the DI right in the constructor for the DI container, or are we going to be passing the DI container around so that each function can register it's new stuff to the DIC?

I imagine with #1599108: Allow modules to register services and subscriber services (events), there will be more stuff registered to the container via an event.

All in-constructor registration I view as a stopgap until the better mechanism is in place.

Status:Needs review» Needs work

+++ b/core/lib/Drupal/Core/ExceptionController.php
@@ -184,7 +172,7 @@ class ExceptionController {
-      $response = $this->kernel->handle($subrequest, HttpKernelInterface::SUB_REQUEST);
+      $response = $this->container->get('httpkernel')->handle($subrequest, HttpKernel::SUB_REQUEST);

As Lukas noted, why is this switching to HttpKernel instead of HttpKernelInterface? (I suppose it doesn't greatly matter, but...)

Otherwise I think this is good to go.

Are we sure that onHtml405 works? Where does $event get defined?

Status:Needs work» Needs review
StatusFileSize
new10.91 KB
FAILED: [[SimpleTest]]: [MySQL] 37,014 pass(es), 11 fail(s), and 8 exception(s).
[ View ]

Here's the patch with an update to what Crell's asking for in #36:

Status:Needs review» Needs work

The last submitted patch, 1595146_38_kernel_di.patch, failed testing.

Oh, sorry. Forgot about that:

+++ b/core/lib/Drupal/Core/ExceptionController.phpundefined
@@ -119,7 +107,7 @@ class ExceptionController {
+      $response = $this->container->get('httpkernel')->handle($subrequest, HttpKernel::SUB_REQUEST);

Basically I switched to HttpKernel, because we were already using it here.

+++ b/core/lib/Drupal/Core/ExceptionController.phpundefined
@@ -184,7 +172,7 @@ class ExceptionController {
+      $response = $this->container->get('httpkernel')->handle($subrequest, HttpKernelInterface::SUB_REQUEST);

But not here.

That's why we can't remove the use statement for one of them, unless we decide for one way. Thus the tests are failing.

In that case I vote for the patch in #32

Status:Needs work» Reviewed & tested by the community

I don't care either way as long as we're consistent. So sure, #32 RTBC.

StatusFileSize
new10.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1595146-kernel-di-32_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Keeping confusion down by re-uploading #32.

Status:Reviewed & tested by the community» Needs work

This looks like a pretty straight-forward moving around of code, so no worries there. However, it seems to need a re-roll.

Status:Needs work» Needs review
Issue tags:-WSCCI, -Dependency Injection (DI), -kernel-followup

#43: 1595146-kernel-di-32.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+WSCCI, +Dependency Injection (DI), +kernel-followup

The last submitted patch, 1595146-kernel-di-32.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new11.04 KB
PASSED: [[SimpleTest]]: [MySQL] 37,058 pass(es).
[ View ]

Here's a reroll.

Assigned:Unassigned» webchick
Status:Needs review» Reviewed & tested by the community

Bot approved.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

WOW WOW WOW Awesome!

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