Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#47 | 1595146-kernel-di-47.patch | 11.04 KB | Niklas Fiekas |
#43 | 1595146-kernel-di-32.patch | 10.98 KB | RobLoach |
#38 | 1595146_38_kernel_di.patch | 10.91 KB | cosmicdreams |
#32 | 1595146-kernel-di-32.patch | 10.98 KB | Niklas Fiekas |
#32 | 1595146-kernel-di-32-interdiff.txt | 1.89 KB | Niklas Fiekas |
Comments
Comment #1
Crell CreditAttribution: Crell commentedI 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.
Comment #3
Crell CreditAttribution: Crell commentedAfter 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.
Comment #4
RobLoachShouldn'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.
Comment #5
Crell CreditAttribution: Crell commentedPassing 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. :-)
Comment #6
RobLoachWas 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.
Comment #7
cosmicdreams CreditAttribution: cosmicdreams commentedThis patch revises #6 by removing the use statement for DependencyInjection/Reference and removes an extra blank line.
Comment #9
cosmicdreams CreditAttribution: cosmicdreams commented#7: 1595146_6_kernel.patch queued for re-testing.
Comment #11
cosmicdreams CreditAttribution: cosmicdreams commented#6: kernel.patch queued for re-testing.
Comment #12
cosmicdreams CreditAttribution: cosmicdreams commentedOK, then I guess that use statement was important after all. #6 is RTBC in my book.
Comment #13
marfillaster CreditAttribution: marfillaster commentedkernel service should be marked synthetic.
Comment #14
Dries CreditAttribution: Dries commentedRob, care to give a bit more explanation? It's not clear what failed based on your comment.
Comment #15
cosmicdreams CreditAttribution: cosmicdreams commented@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...
Comment #16
RobLoachSeems 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.Comment #17
cosmicdreams CreditAttribution: cosmicdreams commentedAlso:
the boot function demonstrates a way to share a container between multiple items.
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?
Comment #18
marfillaster CreditAttribution: marfillaster commentedhere's an explanation by a symfony code dev https://groups.google.com/forum/#!msg/symfony-devs/Uq6dC09O8cg/G6aOeGVaQ-IJ
Comment #19
cosmicdreams CreditAttribution: cosmicdreams commentedInteresting information about scopes as well: https://groups.google.com/d/msg/symfony-devs/Uq6dC09O8cg/iU_q3GwMhRUJ
Comment #20
Crell CreditAttribution: Crell commentedRe #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.
Comment #21
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThis 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.
Comment #22
Crell CreditAttribution: Crell commentedLet'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.
Comment #23
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedLooks 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?
Comment #25
Crell CreditAttribution: Crell commentedThis 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.
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.
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.
Comment #26
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedEasy 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.
Comment #27
Crell CreditAttribution: Crell commentedThat 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.
Comment #28
lsmith77 CreditAttribution: lsmith77 commentedis there a reason to longer "use HttpKernelInterface" to reference "HttpKernelInterface::SUB_REQUEST"?
Comment #29
RobLoachNot 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.
Comment #30
Niklas Fiekas CreditAttribution: Niklas Fiekas commented@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.
Comment #31
cosmicdreams CreditAttribution: cosmicdreams commentedCan we make this be:
... for consistency's sake.
Comment #32
Niklas Fiekas CreditAttribution: Niklas Fiekas commented@cosmicdreams: Oh, yes, thanks, we can (or almost). Didn't know that sugar.
Comment #33
cosmicdreams CreditAttribution: cosmicdreams commentedThanks, 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?
Comment #34
RobLoachI imagine with #1599108: Allow modules to register services and subscriber services (events), there will be more stuff registered to the container via an event.
Comment #35
Crell CreditAttribution: Crell commentedAll in-constructor registration I view as a stopgap until the better mechanism is in place.
Comment #36
Crell CreditAttribution: Crell commentedAs 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.
Comment #37
cosmicdreams CreditAttribution: cosmicdreams commentedAre we sure that onHtml405 works? Where does $event get defined?
Comment #38
cosmicdreams CreditAttribution: cosmicdreams commentedHere's the patch with an update to what Crell's asking for in #36:
Comment #40
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOh, sorry. Forgot about that:
Basically I switched to HttpKernel, because we were already using it here.
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.
Comment #41
cosmicdreams CreditAttribution: cosmicdreams commentedIn that case I vote for the patch in #32
Comment #42
Crell CreditAttribution: Crell commentedI don't care either way as long as we're consistent. So sure, #32 RTBC.
Comment #43
RobLoachKeeping confusion down by re-uploading #32.
Comment #44
webchickThis looks like a pretty straight-forward moving around of code, so no worries there. However, it seems to need a re-roll.
Comment #45
Crell CreditAttribution: Crell commented#43: 1595146-kernel-di-32.patch queued for re-testing.
Comment #47
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedHere's a reroll.
Comment #48
Crell CreditAttribution: Crell commentedBot approved.
Comment #49
webchickCommitted and pushed to 8.x. Thanks!
Comment #50
cosmicdreams CreditAttribution: cosmicdreams commentedWOW WOW WOW Awesome!