Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Title: Allow DrupalKernel to be swappable » Load 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.

Crell’s picture

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.

RobLoach’s picture

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.

Crell’s picture

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. :-)

RobLoach’s picture

Status: Needs work » Needs review
FileSize
1.8 KB

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.

cosmicdreams’s picture

FileSize
1.56 KB

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.

cosmicdreams’s picture

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.

cosmicdreams’s picture

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

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

cosmicdreams’s picture

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.

marfillaster’s picture

kernel service should be marked synthetic.

Dries’s picture

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

cosmicdreams’s picture

@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...

RobLoach’s picture

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.

cosmicdreams’s picture

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?

marfillaster’s picture

cosmicdreams’s picture

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

Crell’s picture

Title: Load the Kernel from the DI Container » Load 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.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
9.26 KB

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.

Crell’s picture

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.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
10.72 KB

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.

Crell’s picture

+++ 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.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
4.56 KB
11.29 KB

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.

Crell’s picture

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.

lsmith77’s picture

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

RobLoach’s picture

+++ 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.

Niklas Fiekas’s picture

@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.

cosmicdreams’s picture

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.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
1.89 KB
10.98 KB

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

cosmicdreams’s picture

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?

RobLoach’s picture

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.

Crell’s picture

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

Crell’s picture

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.

cosmicdreams’s picture

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

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
10.91 KB

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.

Niklas Fiekas’s picture

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.

cosmicdreams’s picture

In that case I vote for the patch in #32

Crell’s picture

Status: Needs work » Reviewed & tested by the community

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

RobLoach’s picture

FileSize
10.98 KB

Keeping confusion down by re-uploading #32.

webchick’s picture

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.

Crell’s picture

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.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
11.04 KB

Here's a reroll.

Crell’s picture

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

Bot approved.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

cosmicdreams’s picture

WOW WOW WOW Awesome!

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