Problem/Motivation

Currently, modules cannot register new services in the DIC. They also cannot register their own event subscribers. That was acceptable for the kernel patch to get it in place, but we cannot actually call the system working if it's not extensible. Modules need a low-cost way to register services and event listeners/subscribers.

Proposed resolution

We're going to steer as close to Symfony as possible here. To that end, what we need to do is:

- Switch from using EventDispatcher to using ContainerAwareEventDispatcher.
- Allow classes to provide a class implementing HttpKernel\BundleInterface. Bundles provide the touch point for "compiler passes" for the dependency injection container, which allow any bundle to add things to the DIC.
- Because later compiler passes can add information to previous passes, one of the things you can do there is add "extenders" to the dispatcher entry in the DIC. That is, "when you're doing the setup and instantiation for this object, do this stuff, too." "This stuff" is then registering additional subscribers out of the DIC.
- Modules create a service for each subscriber they are registering, then add that service as a subscriber to the dispatcher.

As it stands in 8.x we are using the HttpKernel class from Symfony, whose task is to handle the request and return a response. In this patch we subclass Symfony's Kernel class, which does a lot more besides handling the request and response (internally it uses an HttpKernel to do that part). It actually creates the dependency injection container and registers Bundle classes so that those bundles can then add services to the DIC. The patch provides a CoreBundle, which extends Symfony's Bundle class, and this registers all of the services that the core subsystems need. They get added in the bundle's build() method. Any services that need to subscribe to events are tagged as such, e.g.:

$container->register('router_listener', 'Drupal\Core\EventSubscriber\RouterListener')
  ->addArgument(new Reference('matcher'))
  ->addTag('kernel.event_subscriber');

The CoreBundle adds a compiler pass, RegisterKernelListenersPass, that goes through all services tagged with 'kernel.event_subscriber' and adds them as subscribers to the dispatcher service.

Any module wishing to get in on this action would provide its own MyModuleBundle class, subsclassing the Symfony Bundle class directly, not CoreBundle. In the build() method it would register services to the DIC, adding the 'kernel.event_subscriber' tag to any that need to be added as subscribers to the dispatcher.

Another significant change added in this patch is that the Request object is now handled in the DIC. This means we can have services in the DIC that depend on the Request, the one current example of this being the new LanguageManager class that returns a language object depending on type, e.g. LANGUAGE_TYPE_INTERFACE. This means that the actual language negotiation logic can use information from the request directly, like the path, to determine the language.

The patch removes the request() wrapper function.

Remaining tasks

Nail the one outstanding test failure.

Ensure sufficient test coverage for the new DrupalKernel/DIC/CoreBundle setup.

After this, actually compiling the DIC so that we don't have to reinitialize it every request load will become even more critical.

User interface changes

None, this is all code-level stuff.

API changes

All modules have (or can have, TBD) a bundle class that acts as a sort of info-hook-ish tool to add anything to the DIC.

Original report by Crell

Right now, the kernel has a whole bunch of hard-coded subscribers. That's well and good for now, but we need to allow modules to add their own listeners, too. And it needs to be done in a performant fashion.

The goal here is to allow any module to:

1) Add subscribers to a common EventDispatcher object.
2) Subscriber objects should NOT have to all be instantiated in advance. That would not be performant.
3) Subscriber objects should accept dependency objects, so they can be put into the DIC.
4) This process may *not* use hooks, because that binds two event systems together too tightly and hooks are not available until full bootstrap.

I don't know at the moment how best to do this, but it is a requirement for Drupal. I know that Symfony full stack has a solution points 2 and 3, but I don't know how it works yet. That's where we should start investigating first.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

These two sentences don't compute for me:

allow any module to:

4) This process may *not* use hooks, because that binds two event systems together too tightly and hooks are not available until full bootstrap.

If listeners/subscribers are going to be provided by actual modules, then there's already an inherent dependency on the module system (and the module system in turn depends on the hooks system), so whether it's explicitly using hooks or not it will still have a dependency in there somewhere. Not that hooks are necessarily the best mechanism but ruling them out without an alternative would run the risk of going with something superficially more decoupled without any impetus to refactor the module system so it doesn't have some of these problems.

Crell’s picture

Modules are not dependent on hooks; hooks are dependent on modules, and modules serve only two purposes: Packaging of somewhat related code and hook namespaces.

The dependency I want to avoid here is full bootstrap, which is required for arbitrary hooks to work. Since many listeners will fire before that (and we're trying to eliminate the concept of bootstrap phases in the first place) it would create a circular dependency of the sort that bite us quite often.

I'm still not sure what the alternative is, and part of this issue, certainly, is figuring out that better alternative. Until I have time to dig into Symfony in more detail to see how it does it, I can't say more than that.

catch’s picture

Modules are not dependent on hooks; hooks are dependent on modules

No there's a recursive dependency since so much of the module system has hard dependencies on hooks implemented by system module.

I haven't reviewed exactly how Symfony does this, but have a feeling you need to wire up listeners in configuration somewhere, then it handles the rest from there.

moshe weitzman’s picture

Perhaps we write to configuration each time the module list is refreshed? I'm not sure what the contents of the config are, but it would be something that lets us find these non-core Subscribers.

marfillaster’s picture

Subscribers are defined as services in Symfony. They are automatically registered by the DI component in a mechanism called compiler pass (late injection after compilation of the container). Services in sf can be "tagged" so all the compiler pass needs to do is retrieve services with a tag say "kernel.event" and add them as subscribers to the event dispatcher component.

aspilicious’s picture

Ok thank you for your response. Got any more info on this? Example code?

aspilicious’s picture

marfillaster’s picture

I would imagine a process like this:

1. Kernel boots prior to request handling. As of now, kernel still has no idea which module to boot because database service has yet to start. After this, retrieving enabled modules from database should be implemented as a compiler pass with the highest priority.

2. Once active modules are identified, a secondary compiler pass will call bootstrap of each module which will only accept DI container. Since module bootstrap has access to DI, it can already do all sorts of magic such as adding subscribers to event dispatcher service, and even replace core services and DI parameters. Take note that this is still part of DI compilation phase so no request handling must be done in module bootstrap.

Since determination of active modules will only happen during DI compilation there is no need to access database on every request. Major perf win.

Bonus:
The core system can be implemented as module and can be fully replaced by another module.

cosmicdreams’s picture

Thrilling!

currently we do this in index.php (the first file that is executed)

require_once DRUPAL_ROOT . '/core/includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_CONFIGURATION);

// Create a request object from the HTTPFoundation.
$request = Request::createFromGlobals();

// Set the global $request object. This is a temporary measure to keep legacy
// utility functions working. It should be moved to a dependency injection
// container at some point.
request($request);

// Bootstrap all of Drupal's subsystems, but do not initialize anything that
// depends on the fully resolved Drupal path, because path resolution happens
// during the REQUEST event of the kernel.
// @see Drupal\Core\EventSubscriber\PathSubscriber;
// @see Drupal\Core\EventSubscriber\LegacyRequestSubscriber;
drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE);

$dispatcher = new EventDispatcher();
$resolver = new ControllerResolver();

$kernel = new DrupalKernel($dispatcher, $resolver);
$response = $kernel->handle($request)->prepare($request)->send();
$kernel->terminate($request, $response);

Does this mean we move the instantiation of kernel above the request then find a way to pull the request from the kernel?

marfillaster’s picture

Does this mean we move the instantiation of kernel above the request then find a way to pull the request from the kernel?

Request should be created right after kernel initialization and prior to kernel handling request. Controller resolver and event dispatcher can be moved to DI so they can also be overridden by any module.

// Set the global $request object. This is a temporary measure to keep legacy
// utility functions working. It should be moved to a dependency injection
// container at some point.
request($request);

There is no need to move request initialization to DI. Request is a synthetic service thus needs to be injected to the container itself. Kernel::handle already handles injection of request to DI as in the case of FrameworkBundle of Symfony.

So I think this issue can only move forward once DI component is properly integrated to Kernel.

At the minimun, index should look like this

require_once DRUPAL_ROOT . '/core/includes/bootstrap.inc';

drupal_bootstrap(DRUPAL_BOOTSTRAP_CONFIGURATION);
// Bootstrap all of Drupal's subsystems, but do not initialize anything that
// depends on the fully resolved Drupal path, because path resolution happens
// during the REQUEST event of the kernel.
// @see Drupal\Core\EventSubscriber\PathSubscriber;
// @see Drupal\Core\EventSubscriber\LegacyRequestSubscriber;
drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE);

$kernel = new DrupalKernel(); 

// Create a request object from the HTTPFoundation.
$request = Request::createFromGlobals();
request($request);

$response = $kernel->handle($request)->prepare($request)->send();
$kernel->terminate($request, $response);
Crell’s picture

The kernel is irrelevant. We're talking about letting modules register listeners on the EventDispatcher object, which should, when we're done, get passed TO the kernel in its constructor (by the DI container). I don't see how the kernel is relevant to that.

pounard’s picture

I have to agree with Crell here.

cweagans’s picture

Crell’s picture

I spoke with Fabien about this subject in Paris, and also sat in on some related sessions. Here's a brain dump.

What we want to do is switch from the EventDispatcher class to the ContainerAwareEventDispatcher class. That adds another method, addSubscriberService(), which, lets you specify a class that lives at some container ID as a subscriber. That, in turn, can be queued up on the event.dispatcher DIC item. The DIC allows you to add "extenders", which is basically "oh yeah, when you instantiate this object, also call these methods in addition to the ones that are already listed there". That lets a module add a new subscriber without instantiating it. Something like:

$dispatcher = ContainerAwareEventDispatcher();
$container->set('some.service.id', new Definition...);
$dispatcher->addSubscriberService('some.service.id', 'Some\Class');

To do this approach properly we should, as suggested in another thread, leverage Symfony's Bundle interface and make each module a Bundle; that in turn will allow it to register whatever events/container entries it needs during the compile phase, which will then be available when they're needed.

donquixote’s picture

So, to understand this correctly:

In request 1, when we rebuild the di/registry/cache/etc, we ask the modules (*) which services they want to subscribe, and remember it.
Modules will not subscribe instantiated objects, but class names with optional arguments, which can all be remembered between requests.

In request 2, when we actually fire an event, Drupal will lazily instantiate (**) any subscribed classes, with arguments as defined, and invoke the relevant methods. The subscriber objects may be reused whenever the event fires again, and will die at the end of the request.

(**) On instantiation (which is usually not more than once per request), the subscriber objects can get their dependencies from the DI.

(*) "we ask the modules" would still need to be defined. If modules are bundles, then they have some kind of compile script, which is called by Symfony for exactly this purpose. So in fact, the "compile script" would be like a hook, but with a file instead of a function.
It would still be possible that this compile step would involve some magic naming detection.

Crell’s picture

Almost. Any subscriber classes would be lazy-loaded on every request to call their static subscribe method, but the object would not be instantiated until we actually call the event. Then because the objects are in the DIC, the same one will be kept around all-request and can also be injected with its dependencies from the DIC.

Also, the "compile hook" would be a method on a per-module class, not a file per se. That class also has other useful "build" methods. See the BundleInterface in the HttpKernel component.

katbailey’s picture

I spent some time looking into bundles and how they're used in the symfony-standard framework. I think I got my head about half-way around how this works. Basically, once a bundle gets registered, its services can automatically get loaded into the container through its extension class. See the demo bundle's extension class here: https://github.com/symfony/symfony-standard/blob/master/src/Acme/DemoBun.... I'm assuming that rather than using an xml file we would just do $container->register('some.service.id', 'SomeClass').

What I'm really not clear on though, is whether it's possible for us to do anything at all with bundles without switching over to using a subclass of the Kernel class (as opposed to just using the HttpKernel). The Kernel class knows how to deal with bundles (it has a registerBundles() method, and ensures all extensions are loaded when building the container) - so do we just steal code from that class in order to deal with bundles without having to change out the kernel architecture? Or are we intending to do the latter anyway and leave this issue blocked on that?

Crell’s picture

Subclassing the Kernel class and using THAT as DrupalKernel, then just using HttpKernel as-is, is the plan there I think. There's a few methods in Kernel we'd need to override to eliminate dependencies on the Config system, but that's easy enough to do.

katbailey’s picture

Just putting a link to this PoC patch here for now, still lots to figure out...
https://gist.github.com/f9c35b0b8eb64155b4ca

donquixote’s picture

@Crell (#16):

Any subscriber classes would be lazy-loaded on every request to call their static subscribe method

Lazy as in "loaded via PSR-0", but not lazy as in "only when needed". If we load it every request.

I am undecided if I like this or if I would prefer to have this information cached.
Probably the method will be quite short, and much faster than the magic naming detection that we have now. So maybe ok.
(if we actually say goodbye to magic naming)

If this is to replace the entire hook system, then maybe the solution will be that some subscriptions will be cached, while others won't.

static subscribe method

Isn't this equivalent with a procedural registration hook?
I suppose there will be a convention for the class name and method name of this subscribe method, so we move from magic-named procedural registration hook to a magic-named class + static method.

This will look nicer to people who are allergic to procedural, but it adds no architectural value.

Also, I wonder, will the subscriptions depend on reqest data such as user, permissions, etc?
Do we need to provide this information to the subscription method via DI ? Could it be a good idea to move from static subscription method to a DI-instantiated subscription object with object-level subscription methods?

Should the subscription method / object have all request info available, or just "static" information such as module enabled status, and persistent data in the db?

but the object would not be instantiated until we actually call the event.

"The object" = the subscriber/observer/listener
-> Ok.

So, the subscription method does specify class names and some constructor options, but does not instantiate anything.

Then because the objects are in the DIC, the same one will be kept around all-request and can also be injected with its dependencies from the DIC.

Ok.

This approach will break if we have different observers / subscribers for different use cases.
E.g. (bad example) if we would treat views plugins as listeners / subscribers, then we would have a different set of listeners / subscribers per views display. I suppose, for these cases we would simply not use this subscriber model.

Also, the "compile hook" would be a method on a per-module class, not a file per se. That class also has other useful "build" methods. See the BundleInterface in the HttpKernel component.

Ok, as said above, that's moving from procedural registration hooks (which could be in registry.inc) to magic-named static methods. Technically equivalent, just aesthetically different. It can be PSR-0 autoloaded instead of having to explicitly include the registry.inc. But otherwise, I see no architectural improvement.

Even the function_exists() will not go away: Instead of function_exists, we will now have method_exists, for the static registration method and for all the other 'useful "build" methods'.

If a majority of people is "allergic to procedural", then fine. Let's just not feel like we are the great inventors, only because we replace a function with a static method.

Disclaimer:
I am still not convinced that explicit registration is better than magic naming. I'm just setting that aside for the above comments.

Crell’s picture

don, I don't think you understand what is being discussed at all.

I am not offering a proposal. I'm describing how the EventDispatcher component works, today, that we're just not taking full advantage of yet. Fully implementing it is not a debatable point; until we do, one cannot register additional listeners on the http kernel and the entire system is hard coded.

Whether we fully replace hooks with this system, now or in a future version, is a separate question. Not implementing this mechanism is not an option that is on the table, because we are using EventDispatcher in the core page flow now as a result of using HttpKernel.

Your bad example is indeed bad; there is no reason whatsoever that anyone would try to put Views plugins into the Event Dispatcher. That's so much of a disconnect I cannot even come up with a cute analogy to illustrate how off-topic that is. :-)

The EventDispatcher system has no magic naming at all; A subscriber class has a method on it, defined by an interface, that is called statically. Interface-specified methods are anything but magic. That's the only static method involved, I believe.

Have a look at PathSubscriber. The static method at the bottom is called to build an index of what should get called when. Right now, we're instantiating that object and passing it to the EventDispatcher, which then calls that static method.

Instead, what we want to do is use the ContainerAwareEventDispatcher, register each subscriber in the DIC, and tell the dispatcher to load the subscriber from that DIC entry. It will then call the static method directly, without instantiating the object; that parses code, but doesn't create the object. Then when the listener method is actually called, The dispatcher instantiates the object out of the DIC before invoking the method... but because it's in the DIC, you can inject stuff into it, as well, like the DB connection, the actual path alias logic, etc.

Then we still need somewhere for modules to register those subscribers on the dispatcher. Which is, it turns out, exactly the same place that modules register new objects with the DIC! And the place for that, is an object that implements the bundle interface.

So every module has a bundle object. We just need to figure out how each module declares what it's bundle object is. Symfony full stack does that by matching the name of the class to the bundle that contains it. We may or may not do something similar. Kat's working on that. :-)

donquixote’s picture

Thanks for the clarifications.
After hours in the symfony docs, I hope I understand things better now.

Then we still need somewhere for modules to register those subscribers on the dispatcher. Which is, it turns out, exactly the same place that modules register new objects with the DIC! And the place for that, is an object that implements the bundle interface.

Isn't that actually the "Extension" class and not the "Bundle" interface? (*)
(following the link in #17)
That would be the load() method, where we register the services to the DIC (or we use a config file to do the same).

But:
- Where would the load() method get the dispatcher object from? Directly from the container? Is this allowed, during the compile phase?
- If the load() method is fired only during the compile phase, then will the dispatcher subscriber registration be remembered?

The "tagging" of services mentioned in #5 and #7 looks like a promising solution.

---------

(*) Extensions vs Bundles:
It seems that symfony allows more than one extension per bundle:
http://symfony.com/doc/current/cookbook/bundles/extension.html
The convention is to have "AcmeHelloExtension" for "AcmeHelloBundle", but you can register additional ones, e.g. "UnconventionalExtensionClass".
This seems like a fit for
bundle ~= drupal "project" (what you download with drush dl)
extension ~= drupal "module" (what you enable with drush en)
or we just say module = bundle = extension, and ignore the possibility to have more than one extension per bundle.

donquixote’s picture

I think we need a more general discussion "Allow modules to register services to the DIC".
Does such an issue already exist?

Crell’s picture

Issue summary: View changes

Switch summary to be about DIC registration, of which events are just one small part.

Crell’s picture

Title: Allow for registration of additional listeners/subscribers » Allow modules to register services and subscriber services (events)
Issue tags: +Dependency Injection (DI)

I'm not sure of the details of bundle vs. extension right now. I am hoping Kat will figure that out for us. :-)

I talked with Rob Loach briefly, and we decided this is the issue for discussing DI registration. Therefore, retitling, tagging, and updating the summary...

donquixote’s picture

I see two options, and a lot of space in between.
In both cases we can use event tagging or another technique for the subscriber services, so that the average module does not need to deal with the event dispatcher directly. (*)
In both cases we can move a lot of stuff into a per-module yml file, so that a module does not even need to know if it is a symfony bundle or extension, or just a Drupal module. (**)

1) Modules become bundles and/or extensions.
The extension implements the load() method, which receives a config object and the container as an argument.
This can be a lot of work and thinking, and we might have to remodel a lot of how Drupal deals with modules. Also we would need to figure out what the config object does.
Really awesome would be if the same contrib package can be used in full-stack Symfony as a bundle, and in Drupal as a module.

2) Modules can implement a hook_dic() and/or a hook_dic_alter().
This is very drupalish and may look ugly, but probably the easiest to squeeze into our current module architecture.
Technically, I regard this as equivalent with the symfony way: In Symfony, the extension is an instantiated object, but we don't do anything with it that could not also be done with the old-school hook.

-------------------

(*) The "other technique" I think of is to let the compiler pass look for services that implement the EventSubscriber interface, and tag each of them as an event subscriber.

(**) For modules that don't implement hook_dic(), or that don't provide an Extension or Bundle class, we could simply look if there is a module.yml file, and use that to wire up the DIC.
If we do this, we might want to allow the module (and optionally, other modules) to modify the config loaded from the yml file, and merge it with user-specified values.
This can be done (1) by letting the load() method load the yml explicitly and modify it, or (2) with a hook_config_alter().

I would strongly prefer a solution where we have an enforced convention for the location of the .yml file, so the module does not have to do the "FileLocator" stuff explicitly. The hook_config_alter() looks more promising for that, but a subclass of Extension might do just the same.

Crell’s picture

don, see the original post, or comments #2 and #3. We do not want EventDispatcher dependent on hooks. That makes hook_dic() a non-option.

katbailey’s picture

Ok I am just throwing this up here for now - work in progress: https://gist.github.com/f9c35b0b8eb64155b4ca
I had originally done this work on top of #1595146-32: Load the HttpKernel from the DI Container but this patch represents a squashed commit of everything rolled against 8.x. Not sure if that's more or less confusing :-/
Anyway, this patch uses a compiler pass to add event subscribers to the dispatcher. I had to go back to subclassing the ContainerBuilder class in order to override a bunch of methods that are too bound up with the Config component.
Oh, and overlay seems to be borked but I haven't yet figured out why.

Crell’s picture

Kat, can you post patches here rather than in a gist? That makes it easier for Drupalers to review, even if it's just in draft form. (Links to Git repos where we can see individual commit steps are good, too.)

katbailey’s picture

Status: Active » Needs work
FileSize
17.84 KB

Yep, sorry - I just have a bit of a penchant for gists.
Here is the patch as per normal protocol.
@Crell, would it make sense for me to do this work in a branch in your wscci sandbox?

Crell’s picture

katbailey: Yes it would! Go ahead and make a new branch, then go to town. I suggest "bundles", branched from upstream/8.x. (The 8.x branch in the wscci repo is rather old; don't use it.)

donquixote’s picture

#26

Disclaimer:
I do not want to absolutely defend hooks, I just disagree with the argument.

The dependency I want to avoid here is full bootstrap, which is required for arbitrary hooks to work.

It should be enough to include the *.registry.inc (or *.install or whatever) of every enabled module, and fire a hook_dic(). With a custom version of module_invoke_all(), if you want.
This should not be any more costly or dependency-heavy than what we plan to do with bundles/extensions.

In any case, we need to know about the enabled/disabled status of each module, and its filesystem location. We need this for autoload setup, and for bundles/extensions. I suppose that we regard a bundle/extension as "enabled" if and only if the respective module is set as "enabled".

This said, the bundle/extension system might simply be more elegant. This would be an argument I can live with :)

Crell’s picture

Quite simply, given the choice between:

1) Writing our own junior-hook one-off for dic/event registration

2) Leveraging the already-existing infrastructure that is already in our repository by virtue of being in the HttpKernel and that will be recognizable to the few thousand Symfony developers already out there using this code, and opens up the possibility of potentially supporting some Symfony2 bundles in the future

I'll take door number 2, Monty.

sun’s picture

Note: I'm only loosely following this issue.

The dependency I want to avoid here is full bootstrap, which is required for arbitrary hooks to work.

Given how things are currently progressing and converting, I could very well see most of the bootstrap phases to "collapse" very soon. I was already very positively surprised when I was able to make the entire Configuration system work (including active store) right at the very first bootstrap phase already, DRUPAL_BOOTSTRAP_CONFIGURATION. The more subsystems we're moving to PSR-0 classes and the DI container, the more I can imagine that drupal_bootstrap() might even totally vanish for D8. The very last barriers to cross for ultimately eliminating the sense of "full bootstrap" are going to be #1331486: Move module_invoke_*() and friends to an Extensions class and #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config.

Of course, I totally do not want to hold up or block progress on this work here, so please go ahead! ;) My intention is merely to mention that it's no longer impossible or beyond imagination that the module/hook system would require a thing called "last/full bootstrap phase" — which might change perspectives on the architectural design goals here.


That said, since the term "ContainerAwareEventDispatcher" was mentioned here, I'd like to point out that various benchmarks so far showed that the performance of ContainerBuilder is very poor and means a serious performance regression. I still have to create a separate issue for that, so I also don't want to hold your progress off on that. ;) However, in light of the numbers I've seen thus far, I'd rather recommend to think about implementation ways that don't involve ContainerBuilder. (Lastly, note that Container is most probably fine, as it doesn't involve the massive code and internal function calls for dynamically registering and resolving service definitions and parameter registrations, only ContainerBuilder has that code. Since this issue talks about modules [which are kinda dynamic by design], I assume that ContainerAwareEventDispatcher deals with ContainerBuilder.)

Crell’s picture

ContainerBuilder is not fast to build, that is certainly true. That's why any non-dev site dumps its built information and just a quick load on it later. Most Symfony apps dump to PHP code. If we can't get away with that for security reasons, YAML, JSON, or something else along those lines is still much faster than rebuilding every request. (This is a problem we are going to face with Twig, too. We need to address it head on sooner or later.)

For this issue, I don't think we should be worrying about the dump part. That by design can be handled separately in another issue; I agree entirely though that such an issue needs to happen.

effulgentsia’s picture

For this issue, I don't think we should be worrying about the dump part.

I skimmed through #29, and from what I see, I think we need to confront the dump part (even if a placeholder implementation) in this issue. This patch seems to be all about confronting the difference between building/compiling a container, and using it. Therefore, we need drupal_container() to return a built container, but not a ContainerBuilder. We need to make sure that this patch only builds a container rarely (e.g., when a module is enabled/disabled), and that for all other requests, we can simply instantiate an already built container.

But, I think we can start with a simple implementation. Perhaps just output the compiled PHP file to config_get_config_directory()? For security reasons, that clearly won't be the final solution, but IMO, starting with that will help bring clarity to what steps are done during building and what steps are done during normal requests. Thoughts?

In summary, this part of #29 needs an answer:

+++ b/core/includes/bootstrap.inc
@@ -2446,7 +2446,7 @@ function drupal_container(ContainerBuilder $reset = NULL) {
-    $container = new ContainerBuilder();
+    // HALP!!
sun’s picture

Ouch. ;) I really didn't want to hinder progress here...

For now, I'd recommend to move forward with the architectural design progress to figure out how it could work - potentially ignoring the detail of whether ContainerBuilder is being used as base or not. Most likely, you probably do not need most of the ContainerBuilder functionality anyway, so it could possibly be replaced with something else.

FWIW:

Perhaps just output the compiled PHP file to config_get_config_directory()?

As kinda mentioned before, the refactored config system is able to fully work after the first bootstrap phase already; i.e., as soon as the class autoloader is available. Thus, do not think about "files". Instead, just call config() as usual.

That said, while calling config() will work, the intended usage here doesn't seem to be actual configuration, as far as I understand. It's actually a system "state" that is built depending on other actual configuration (e.g., the enabled list of extensions). A closely comparable state is the list of bootstrap modules (which might happen to be obsolete in D8, but just trying to provide an example). In turn, the final implementation here might require and depend on #1175054: Add a storage (API) for persistent non-configuration state

But again, anyway, in terms of overall architectural design, all of these things are most probably nitty-gritty details anyway. ;)

(At least, I assume that the current patch is an early architectural prototype...?)

effulgentsia’s picture

That said, while calling config() will work, the intended usage here doesn't seem to be actual configuration, as far as I understand.

Right. What I'm suggesting putting here is a PHP file, containing a dynamically created (compiled) class that's a subclass of Container (that's what ContainerBuilder compilation produces). This should absolutely not be its final home. But this folder is the most secure writable location we currently have, so I'm suggesting that we abuse it temporarily.

I know there's some concern that there will be no properly secure solution for writing out a PHP file in response to UI action (like enabling or disabling a module). I'm worried about this as well. But I think it's better for us to develop this issue as though we can do that, and later change to a different approach of dumping and loading a compiled container if we have to, than to delay the decoupling of how we use Container vs. ContainerBuilder, especially given that this issue is the first one that is starting to use CompilerPass objects.

katbailey’s picture

Yes, this patch is most certainly still just at the prototype stage. I had to override various methods just to rip out code that we couldn't use for various reasons (e.g. reliant on the Config component) and so what remains is a fairly bare bones proof of concept that is just allowing us to figure out the flow.
I won't be able to work on this again until Wednesday - in the meantime I've committed the patch above to the bundles branch of the wscci sandbox:
http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/bundles

katbailey’s picture

FileSize
26.84 KB

I made a few more commits to the bundles branch today. I went with effulgentsia's suggestion of dumping the container to a file in the config directory for now. This then meant that the registration of language-related services needed to move out of language_initialize() because at that point you're working with a Container instance, not a ContainerBuilder, and so you can't register anything to it.

I'm attaching an up-to-date patch.

Regarding Bundles vs Extensions, here is my understanding of extension classes in bundles: you use them if you want to provide configurable services to the container. For example, if your bundle defines a 'my_bundle_service' service and you want the parameters that get passed in to be configurable, then you use an extension class. The load() method of the extension class takes a configs array as its first parameter and these values can be used to set parameters on the service.
You declare extensions and their configurations in the main config file for the app, which is specified by the registerContainerConfiguration() method of your Kernel instance. This method is part of the KernelInterface so we have to define it in DrupalKernel. However, it takes a Symfony\Component\Config\Loader\LoaderInterface object as its only parameter, and we are not using the Config component.
Given all this, I don't think extensions are where we should be focusing our attention right now.

It would be good to get a few concrete use cases to help focus the rest of the work on this. I know that #1594870: Decide what KernelEvents::VIEW subscribers should be used for, document it somewhere, and make ViewSubscriber comply is one, and I think I have a rough idea of how that would work, but what are some more straight-forward examples of where we want modules to register services and event subscribers?

aspilicious’s picture

Well I think katbailey is doing an amasing job. Everything she did maps to the stuff I was thinking off while looking at the code. I wondered how she surpassed the config component thingie. She just copied some functions from that component.

I still think they should move the file resource stuff to a different component in the future but for now this should work (in theory, didn't test it yet).

BTW it's a good thing we have to pull stuff out of our regular bootstrap process...

aspilicious’s picture

*Sigh*

Whatever those symphony wizards say, it's almost impossible to NOT use the config component. We are making our lives misserable by copy pasting and adjusting symphony code. That LoaderInterface thingie in the kernelinterface says it all.

I grepped for "use Symfony\Component\Config" in drupal core: 56 matches in 35 files

So if we are not going to add the config component today it will force us eventually...
So I think we should go forward with #1632930: Add Symfony/Component/Config despite Symfony potentially releasing BC-breaking updates after 2.3.

EDIT:
-----
BUT

I grepped through core and we are overriding the only function that calls "registerContainerConfiguration".
There are other function in core (from symphony) that have type hinted classes from components we don't use. See "public function registerCommands(Application $application)" in bundle.php as an example.

I think we just should ignore that but add a comment that calling this function is a BAD idea :)
BTW, I'm going to ask the symfony people if they can remove the function from their interface...

SO we don't need the config component yet...

Crell’s picture

Another use case: #1269742: Make path lookup code into a pluggable class - The path alias lookup logic should be a service that has a dependency on the database, that way the request listener that does path alias dereferencing can take that object as a dependency.

Which means, actually, we need to make the database connection into a service, too. :-) There's likely some fiddling we should do there, but for now just use the Database::getConnection as a factory for the 'database' and 'database.slave' service keys. We can refactor that later.

(Although, what do we do with alternate keys? Eh, punt on that for now; that's for a follow-up patch.)

donquixote’s picture

Database and path lookup -> yep.

One question I have though: Will the DIC be aware of current user and current request? So we could make e.g. the "current theme" a service, too?
I remember in the gdo discussions we had the idea of "context" and "request context" as separate things..

chx’s picture

As for writing PHP files, that's inevitable in this cycle. I wanted this for very long but now it needs to happen for reals: Drupal will have a compile stage and a run stage.

We do have the secure mechanism for that , used currently to update modules. For a lot of people FileTransferLocal will do it so it won't be a major hassle (whatever you people renamed it to in D8, it's impossible to find anything any more so I won't even bother trying to figure it out what it's called nowEdit: because webchick beseeched me to be nice, I looked it up, now it's in core/lib/Drupal/Core/FileTransfer/Local.php ) and there surely will be drush commands.

chx’s picture

Also such a split would have enormous advantages of breaking the current really rotten ways where we try to rebuild on demand and yet keep a system running.

Crell’s picture

There's 2 challenges to a separate compile mode:

1) Changes in the GUI can trigger compile changes, and those in turn then run into the security issue. And while we have FileTransferLocal, it's a huge PITA to use and is bad UX. Should people have to enter their SHELL password every time they tweak certain settings in the UI? Eeew.

2) Those problems all go away when run from the CLI... but then we are forcing people to use the CLI.

I don't know how we make this process not suck. That's worth its own issue to figure out, I think, but I agree it needs to be Solved(tm) once and for all.

pounard’s picture

We could go for a really strict model: disallow any module (install, update or disable) state change while the folder where we write all this code is not writable, with a nice message in status report saying that it's bad to keep it writable when it's writable. It's only 0.000001% of the UX, but that's a start :)

cweagans’s picture

Also? Who the hell uses shell passwords? I use SSH keys for all of my servers and PasswordAuthentication is turned off. FTP is not even on my radar. Requiring FileTransferLocal for settings changes sounds like a bad plan to me..

I've never been quite clear on why we can't write a PHP file and execute it...I realize that having the server write code to the disk and then execute it is considered poor practice, but I've never heard why that's the case. If we're just listing all enabled modules in the form of PHP, I don't see a problem with it, especially if it's a self-contained function and there's no alter hook or any place where a malicious module could start causing problems. Now that I think about it, any module could just write out its own PHP file and execute it, so that probably doesn't even matter.

Sorry if the answer to this is really obvious - it just seems like a really easy way to go.

cweagans’s picture

(To clarify, I'm talking about writing a file directly to the disk, not going through FileTransferLocal)

pounard’s picture

I suppose the most problematic environment are shared hosting boxes, where people may not have a shell access at all, just some oldschool FTP access. People that are not power user will probably never write protect their files by themselves.

sun’s picture

I'm not sure whether it makes sense to try to bend Drupal into a statically compiled system. For some stuff, compilation might make sense, but I doubt it will make sense and be good for the overall product as a whole.

What we're essentially doing here, is to implant configuration for the DI container. I've almost foreseen that this is going to happen in the recent configuration system re-architecture discussions, but no one wanted to listen ;) The revamped config system code registers itself as a service and depends on the DI container now. So this kinda leaves us with a chicken-and-egg situation.

I'm also entirely not sure whether it is a good idea to compile the DI container in the first place. Doing so will disallow dynamic service replacements and extensions. I have the impression that we're only investigating the compilation direction, because ContainerBuilder [not Container] is too slow in handling service registrations and resolving them. We already said in the original issue which added the DependencyInjection component that we will have to investigate and check whether the added code and functionality is actually suitable for Drupal's concepts. I'd therefore suggest to think about the "opposite" direction, too; i.e., replacing ContainerBuilder with a stripped-down implementation that is optimized for performance, instead of a registry that is optimized for compiling. After all, ContainerBuilder's basic functionality is not really rocket science.

Regardless of that, I'd imagine that our ideal architecture would look like this:

  1. The system.extensions config object contains the configuration/definition of enabled extensions. This configuration can be changed through the UI [admin/modules].
  2. DrupalKernel initializes the DI container, which has a couple of minimal services hard-coded - among which is the config system.
  3. DrupalKernel fetches system.extensions from config's active store and registers the services and event subscribers of extensions/bundles on the DI container.
  4. Since registering the extension services requires to instantiate extensions/bundles, and the instantiated extensions itself may not be needed (yet), we can inject another layer of caching/indirection in between; namely, recording the to be registered services as state information.

Related issues:
1) #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config
4) #1175054: Add a storage (API) for persistent non-configuration state

pounard’s picture

While I was on sun's side before (a long time ago) I actually saw what improvements brings this compilation in both Symfony 2 and Zend Framework 2, and it's something that is really significant. Some low level stuff (various (kernel only now) listeners, cache configuration, system list, etc...) could really benefit from this. On the other side, I also think that this is overkill if we stop at a few services and do not leverage this for a whole larger set of components, I'm afraid this way we would experience all the bad consequences (file writing and UI problems mostly) and only a minor (almost non significant) performance boost.

Considering that katbaley's experimentations are really nice to see and appealing, and that, for performances (and a bit of security too) reasons it's a good choice (no wonder why both SF2 and ZF2 go down that road) I still think this should be either optional, either in its own branch until we have a lot more stuff to put into it.

Another problem is that by multiplying the services source (some procedural wrapper remains, config provide its own for whoever wants to do it manually, plugins will bring yet another method to spawn components, a lot of people will still use the good old hooks, plus this new method) is that the whole framework may look totally arbitrary, unstable, inconsistent, complex and random to people when they'll start to search "WhyTF is this component here, and WhereTF does it come from?".

Crell’s picture

The compilation question has been split off to a separate issue: #1668892: Enable secure compiled information on disk

Let's keep this issue focused on getting bundles in place so that we can use them; we can figure out where to put that stuff in the other issue.

Crell’s picture

Issue tags: +wscci-hitlist

Tagging.

Anonymous’s picture

i really don't like where this conversation is going.

seems people are saying "if we don't compile php, performance will suck".

i think a requiring Drupal to compile php, then execute it later should be our absolute last option.

IMO we should find another design before making Drupal require #1668892: Enable secure compiled information on disk.

Anonymous’s picture

just to elaborate a bit on why this is bothering me.

as someone who often works on performance / bootstrap issues, one of the things i really like about drupal 8 based on symfony2 is the idea that we should be able to load and execute less code than we have in the past, and have a much simplified / non-existent bootstrap phase.

and now i'm reading "oh, we need to write out php because otherwise this is going to be too slow".

this is what i'm having trouble reconciling, and why i think we should stop and consider other options for this issue before just assuming a compile-php step.

Crell’s picture

Beejeebus: Please keep that in the other issue; let's not derail this thread again. It needs reviews right now.

Anonymous’s picture

Crell: i absolutely wish to block this issue if we're going with "compiling php is required to make drupal's performance not suck".

if i'm alone on the question, so be it, i'll withdraw the objection and we can continue on making drupal require compiling php.

for now, i don't consider "we're going to require compiling php, go and discuss making that sane over there" a convincing or useful reply to "if this design requires compiled php, we should consider a different way".

pounard’s picture

@beejeebus I'm not even sure performance will suck that much if we don't compile, kernel run migth be a little slower but ideally most of our services will still be lazy loaded by DIC (this is still possible with no compilation I hope) and make the whole rest of Drupal runtime being a bit more efficient.

catch’s picture

if i'm alone on the question, so be it

Nope. I'd like to understand better why exactly the container is so slow without compiled PHP.

To me compiling PHP makes lots of sense for Twig (since it neatly skips the performance/dx trade-off of theme functions vs. templates), but I'm less happy about it if it's necessary for lots of other things.

Also since 'enable secure writing of PHP files' is now not talking about writing straight PHP it's not necessarily going to solve the performance issue anyway, and that seems like something that needs to be discussed at a higher level, not just implementing something which might possibly fix things for us if it works.

effulgentsia’s picture

I'd like to understand better why exactly the container is so slow without compiled PHP.

#1673162: Analyze performance of uncompiled and compiled DI containers

katbailey’s picture

FileSize
24.19 KB

OK, here's a new patch to keep this moving along... it gets rid of the whole dumping to php part as we don't know how (whether?) that will be dealt with.

The one really gross thing about it is the necessity to call system_list in the register_bundles() method of the DrupalKernel because we need some way to know which bundles exist. In a previous patch I had it bootstrapping to a higher phase prior to booting the Kernel so that we could call system_list(), but I want to isolate the problem so now the Kernel gets booted very early on and I'm just explicitly requiring the necessary include files prior to calling system_list(). I've left a TODO about this.

There shouldn't be a problem with switching to the compiled option because there's currently no code that's incompatible with using a Container object as opposed to a ContainerBuilder.

katbailey’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1599108.bundles.62.patch, failed testing.

aspilicious’s picture

+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.phpundefined
@@ -7,113 +7,38 @@
+    public function addCompilerPass(CompilerPassInterface $pass, $type = PassConfig::TYPE_BEFORE_OPTIMIZATION)

4spaces is symfony standards, not drupal standards

+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.phpundefined
@@ -7,113 +7,38 @@
+        $this->compiler->compile($this);

Hmmm, I thought we wouldn't compile anymore?

+++ b/core/lib/Drupal/Core/DrupalBundle.phpundefined
@@ -0,0 +1,217 @@
\ No newline at end of file

Add a newline

+++ b/index.phpundefined
@@ -29,6 +30,9 @@ $request = Request::createFromGlobals();
+$kernel = new DrupalKernel('prod', FALSE);
+$kernel->boot();
+
 // Bootstrap all of Drupal's subsystems, but do not initialize anything that
 // depends on the fully resolved Drupal path, because path resolution happens
 // during the REQUEST event of the kernel.
@@ -36,6 +40,6 @@ request($request);

@@ -36,6 +40,6 @@ request($request);
 // @see Drupal\Core\EventSubscriber\LegacyRequestSubscriber;
 drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE);
 
-$kernel = drupal_container()->get('httpkernel');

mmmm, I think this is incorrect. You probably have to add the code to the drupal container...

-7 days to next Drupal core point release.

katbailey’s picture

Status: Needs work » Needs review
FileSize
25.86 KB

This patch should at least solve the install problem, though I dread to see how many test failures there are :-(

It also fixes the whitespace issues mentioned in #65.

Hmmm, I thought we wouldn't compile anymore?

Compiling in Symfony means more than just the dumping to php side of things; it's also about running compiler passes. We need compiler passes because they allow us to add subscriber services to the event dispatcher. So it's this narrower meaning of "compiling" that we are still doing in this patch.

mmmm, I think this is incorrect. You probably have to add the code to the drupal container

Can you elaborate? I'm not sure what you mean.

Niklas Fiekas’s picture

Thanks @katbailey!

Let's keep in mind that we'll eventually have to mirror to changes in index.php in core/modules/system/tests/http[s].php - having that out of sync has been a pain in the past.

Note that the current patch will probably no longer pass as we removed DrupalKernel in #1595146: Load the HttpKernel from the DI Container. Oh ... I couldn't be more wrong. This is a real kernel ... not the old HTTP kernel.

katbailey’s picture

FileSize
24.82 KB

Let's try this one, just for kicks - will testbot actually manage to run a test..?

Status: Needs review » Needs work

The last submitted patch, 1599108.bundles.68.patch, failed testing.

Crell’s picture

Edit: This is a review of #66, because that's what was posted when I started reviewing it. :-)

+++ b/core/includes/bootstrap.inc
@@ -2469,7 +2471,39 @@ function drupal_container(ContainerBuilder $reset = NULL) {
+    // During a normal page request, as opposed to during installation, this will only
+    // ever happen if an error has been thrown. We need to build a new ContainerBuilder
+    // with only the language_interface service.
     $container = new ContainerBuilder();
+    // An interface language always needs to be available for t() and other
+    // functions. This default is overridden by drupal_language_initialize()
+    // during language negotiation.
+    $container->register(LANGUAGE_TYPE_INTERFACE, 'Drupal\\Core\\Language\\Language');

This may be OK for now, but I wonder if after we have more things in place it would be better to ship a "maintenance mode" DIC precompiled. Same for the installer. Hm. (Just thinking out loud.)

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterKernelListenersPass.php
@@ -0,0 +1,36 @@
+      // We must assume that the class value has been correcly filled, even if the service is created by a factory
+      $class = $container->getDefinition($id)->getClass();

Why is that? Is that a Symfony thing, or a "the DIC ensures that it is, worry not" thing, or...? (I don't know if that needs a comment, but I don't quite get it at the moment.)

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterKernelListenersPass.php
@@ -0,0 +1,36 @@
+      $refClass = new \ReflectionClass($class);
+      $interface = 'Symfony\Component\EventDispatcher\EventSubscriberInterface';
+      if (!$refClass->implementsInterface($interface)) {
+        throw new \InvalidArgumentException(sprintf('Service "%s" must implement interface "%s".', $id, $interface));
+      }
+      $definition->addMethodCall('addSubscriberService', array($id, $class));
+    }

Drupal coding standards say to never reference a global class directly but to "use" them instead.

That said, I agree this looks like a good place to check for interfaces. Is an Exception the right way to handle it, though, or should we do something less fatal (eg, log an error and skip this class, then register everything else). Or do we assume that it will blow up on dev first and never get to production, and a developer will know how to fix it when he gets an exception in his face? Hm, not sure. Thoughts?

+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
@@ -7,113 +7,38 @@
+    if (null === $this->compiler) {
+      $this->compiler = new Compiler();
+    }

Shouldn't this be setup in the constructor, not here? That's what the constructor is for. (If there's a reason we can't do that, it should be documented in a comment.)

+++ b/core/lib/Drupal/Core/DrupalBundle.php
@@ -0,0 +1,217 @@
+class DrupalBundle extends Bundle
+{
+  public function build(ContainerBuilder $container)
+  {
+    parent::build($container);

So, I think what you're trying to do here is replicate what's in FrameworkBundle in Symfony. That is, register services provided by Core, and then let other modules extend Symfony's Bundle class themselves and do their own thing. True?

If so, then we may not want to call this class DrupalBundle. That to me suggests a Drupal-specific base class. Others are free to disagree here if it's obvious to them.

Either way, this class needs a docblock to clear up which it is. I'm fine with us telling module developers to extend from Symfony's Bundle class directly if that makes the most sense, but we should be sure to lead them to the correct conclusion.

+++ b/core/lib/Drupal/Core/DrupalBundle.php
@@ -0,0 +1,217 @@
+  function getDefinitions() {
+    return array(
+      // Register configuration storage dispatcher.
+      'config.storage.dispatcher' => array(
+        'class' => 'Drupal\Core\Config\StorageDispatcher',
+        'parameters' => array(
+          'conifg.storage.info' => array(
+            'Drupal\Core\Config\DatabaseStorage' => array(
+              'connection' => 'default',
+              'target' => 'default',
+              'read' => TRUE,
+              'write' => TRUE,
+            ),
+            'Drupal\Core\Config\FileStorage' => array(
+              'directory' => config_get_config_directory(),
+              'read' => TRUE,
+              'write' => FALSE,
+            ),
+          ),
+        ),
+      ),

I don't really see the value of turning this into another info array, just so it can get dumped back into method calls in build(). Why don't we just throw everything into build() directly? Or do what FrameworkBundle does and wrap them all into compiler passes that we register? (Eg, put the routing-related stuff in one pass object, the database stuff in another, etc.)

+++ b/core/lib/Drupal/Core/DrupalBundle.php
@@ -0,0 +1,217 @@
+      'router_listener' => array(
+        'class' => 'Drupal\Core\EventSubscriber\RouterListener',
+        'references' => array('matcher'),
+        'tags' => array('kernel.event_subscriber')
+      ),
+      'content_negotiation' => array(
+        'class' => 'Drupal\Core\ContentNegotiation',
+      ),
+      'view_subscriber' => array(
+        'class' => 'Drupal\Core\EventSubscriber\ViewSubscriber',
+        'references' => array('content_negotiation'),
+        'tags' => array('kernel.event_subscriber')
+      ),

Is there a reason to use underscores here instead of dots, which we seem to be using elsewhere, as does Symfony?

+++ b/core/lib/Drupal/Core/DrupalBundle.php
@@ -0,0 +1,217 @@
+  function registerLanguageServices($container) {
+
+    $types = language_types_get_all();
+
+    // Ensure a language object is registered for each language type, whether the
+    // site is multilingual or not.
+    if (language_multilingual()) {
+      include_once DRUPAL_ROOT . '/core/includes/language.inc';
+      foreach ($types as $type) {
+        $language = language_types_initialize($type);
+        // We cannot pass an object as a parameter to a method on a service.
+        $info = get_object_vars($language);
+        $container->set($type, NULL);
+        $container->register($type, 'Drupal\\Core\\Language\\Language')
+          ->addMethodCall('extend', array($info));
+      }
+    }
+    else {
+      $info = variable_get('language_default', array(
+        'langcode' => 'en',
+        'name' => 'English',
+        'direction' => 0,
+        'weight' => 0,
+        'locked' => 0,
+      ));
+      $info['default'] = TRUE;
+      foreach ($types as $type) {
+        $container->set($type, NULL);
+        $container->register($type, 'Drupal\\Core\\Language\\Language')
+          ->addMethodCall('extend', array($info));
+      }
+    }
+  }

This also seems like it belongs in a compiler pass object, if I'm understanding them correctly, and the object could then even take a few parameters in its constructor to remove the function calls.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -0,0 +1,90 @@
+  public function registerBundles()
+  {
+    $bundles = array(
+      new DrupalBundle(),
+    );

Symfony vs. Drupal coding standards. :-)

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -0,0 +1,90 @@
+      $class = "\Drupal\\{$module}\\{$module}Bundle";

We should probably discuss this as the naming convention. Technically this is going to result in classes with a leading lowercase, which is not allowed.

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

See note above about http_kernel vs. http.kernel. How does Symfony decide when to do each of those?

+++ b/core/lib/Drupal/Core/Language/Language.php
@@ -44,8 +44,8 @@ class Language {
-  public function extend($obj) {
-    $vars = get_object_vars($obj);
+  public function extend($info) {
+    $vars = is_array($info) ? $info : get_object_vars($info);

I don't understand what this is for.

+++ b/index.php
@@ -29,6 +30,9 @@ $request = Request::createFromGlobals();
+$kernel = new DrupalKernel('prod', FALSE);
+$kernel->boot();

How long until we can move bootstrap phases into boot(), I wonder? :-)

And of course there's that whole 0 tests pass issue... :-)

Nice work, Kat! Let's keep this moving.

Crell’s picture

Some relevant documentation from the Symfony side: http://symfony.com/doc/master/components/dependency_injection/index.html

katbailey’s picture

Just wanted to update briefly here as I did some more work on this yesterday but am not putting up a patch until I have more success with tests. I've recreated the bundles branch in the sandbox as I had rebased it against upstream/8.x and committed what I did yesterday. One thing that had been broken and was responsible for many tests assploding was the ExceptionController and ExceptionListener Services not being added correctly to the DIC. I have a fix in place which I wouldn't mind getting some eyes on - see the first method of the ExceptionController class (as I point out in the comment this probably isn't where it belongs): http://drupalcode.org/sandbox/Crell/1260830.git/blob/7cb76bffa0c8ebd9582...

But there are a huge number of tests failing because user login doesn't work during tests and I have no idea why. It seems that any test that calls $this->drupalLogin($some_user) fails with "Sorry, unrecognized username or password" and so most of the subsequent tests then fail also since they are assuming stuff to be in place that isn't (nodes created, etc...)

So I just thought I'd mention that here in case anyone can think of what could be causing this problem (and yes, logging in to the site, outside of the context of simpletest, works fine).

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
24.99 KB

Let's see how we perform after fixing that problem.

Edit: Well, that's at least no fatal errors and a finite number of test failures.

The last submitted patch, 1599108-bundles-73.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
26.32 KB

More testbot ping-pong after a more minor fix.

Edit: Yay! That was more than I thought. All other failures are related to something going on with the language system.

Status: Needs review » Needs work

The last submitted patch, 1599108-bundles-75.patch, failed testing.

podarok’s picture

Status: Needs review » Needs work
Issue tags: +D8MI

tagging for D8MI disqussion

katbailey’s picture

Status: Needs work » Needs review
FileSize
81.21 KB

Curious to see how this one fares against testbot...

Status: Needs review » Needs work

The last submitted patch, 1599108.bundles.78.patch, failed testing.

katbailey’s picture

FileSize
82.85 KB

Ugh, this should at least get rid of the fatal error...

katbailey’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1599108.bundles.80.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
82.91 KB

Did some cleanup, fixed some tests, possibly broke others... we'll see...

Status: Needs review » Needs work

The last submitted patch, 1599108.bundles.83.patch, failed testing.

chx’s picture

Bewildering. What's 'prod', what other options are available and where is this documented?

katbailey’s picture

Yes, I agree this looks a little odd there, it's the $environment parameter... see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Htt...

One place I know it gets used in Symfony is in constructing the filename for the compiled DIC but we don't need to use it at all of we don't want to.

Crell’s picture

That's a built in dev/prod switch that Symfony uses, so you can have different settings or behavior depending on whether you're in dev or prod, even things like swapping your cache backends in the DIC. It's a concept that Drupal lacks but arguably should have, but not something we're actively trying to leverage here.

Still, down to 5 fails! :-)

katbailey’s picture

Status: Needs work » Needs review
FileSize
87.97 KB

A bit more cleanup: I got rid of most calls to request() but it's still needed in the installer.
My fixes for the LanguageDependencyInjectionTest are possibly a bit if a cheat, though realistically I think I'm going to have to redo those tests anyway and add various other tests for the new kernel + DIC set-up in general.

The failing PathAlias test has me stumped, but I'm hoping that's the only one outstanding... unless my changes have broken other stuff.

Niklas Fiekas’s picture

Green with 0 passes!? #88: 1599108.bundles.88.patch queued for re-testing.

pounard’s picture

Nice!

katbailey’s picture

:-(
Yes I really should have tested it with run-tests.sh before putting up this patch. Will fix tonight...

katbailey’s picture

Status: Needs review » Needs work

Actually, come to think of it, I've probably broken the installer as well - I was in too much of a hurry to get this up last night :-/. I think I'm just going to have to put back all those calls to the wrapper function request() for now.
Sad panda.

katbailey’s picture

Status: Needs work » Needs review
FileSize
88.91 KB

OK, let's try this one... This actually gets rid of the request() wrapper function entirely with no detrimental effects, it seems. I tested the installer and also ran run-tests.sh and there were no problems. I also rejiggered the language_manager stuff so I really hope I have not broken any more tests...

Status: Needs review » Needs work

The last submitted patch, 1599108.bundles.93.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
89.44 KB

Ugh, I had missed some calls to request() in overlay.module and comment.module...

Status: Needs review » Needs work

The last submitted patch, 1599108.bundles.95.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Minor tweak.

moshe weitzman’s picture

I reviewed this patch and it looks solid to me. It would be great to document Bundles somewhere, or link to Symfony's docs.

We need to discuss a lot about bootstrap modernization, but I'm wondering if this patch should do away with hook_boot(). Thats not used by core today, and now Bundles can optionally implement boot() method . As a practical example, we'd have to get rid of bootstrap_invoke_all('language_init'); in drupal_language_initialize(). And replace with what? I don't know what contrib modules do in language_init() now, but I'd like to know how they would do same without hook_boot().

RobLoach’s picture

Status: Needs work » Needs review
FileSize
89.44 KB

Needed an update.

We need to discuss a lot about bootstrap modernization, but I'm wondering if this patch should do away with hook_boot(). Thats not used by core today, and now Bundles can optionally implement boot() method . As a practical example, we'd have to get rid of bootstrap_invoke_all('language_init'); in drupal_language_initialize(). And replace with what? I don't know what contrib modules do in language_init() now, but I'd like to know how they would do same without hook_boot().

That's the plan. Components just lazy-load from the container whenever they're needed. We'll need to make that transition slowly though. Once we have Bundles, contributed modules can pack in configuration to the container, which are compiled and cached appropriately.

Status: Needs review » Needs work

The last submitted patch, 1599108.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
97.41 KB

OK, I've added tests that show that this patch actually achieves the goal of this issue ;-)
Also, I've addressed Crell's point in #70 about providing a precompiled contiainer required for any full bootstrap.
And, Crell found the root of the problem behind the last outstanding test fail, and I've added a fix for it.

Fingers crossed...

katbailey’s picture

Status: Needs review » Needs work

Oh crap, I'm pretty sure I just broke more tests than I fixed :-(

Crell’s picture

moshe: I agree, hook_boot and hook_init can go away after this patch, but I'd rather save them for separate patches. As for language_init, probably that sort of logic would end up in the kernel.request listener, I think. That's where we have the core language logic ported to.

kat: What did you do??? :-(

katbailey’s picture

FileSize
96.77 KB

After chatting with sun in irc I have removed the precompiled bootstrap container - he had some concerns about the config services being added there, so I've left that for a follow-up issue: #1703266: Use a precompiled container for the minimal bootstrap container required outside of page requests.

For the PathAliasTest, I think the real fix needs to be done when we overhaul the path alias logic, per this issue: #1269742: Make path lookup code into a pluggable class. I have added a temporary fix to make the test pass and a todo that explains what the problem is. Crell had suggested one option which was do the caching earlier, i.e. at the end of the response rather than during $kernel->terminate() but that seems too drastic a move and it just doesn't feel to me like the right way to go.

There are a couple of outstanding questions in Crell's review from #70:

Why is that? Is that a Symfony thing, or a "the DIC ensures that it is, worry not" thing, or...?

referring to a comment in the RegisterKernelListenersPass. I had copied this verbatim from here: https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/Framew...
So, what's the protocol for when we steal code from Symfony like this?

And lastly,

Is there a reason to use underscores here instead of dots, which we seem to be using elsewhere, as does Symfony?

Actually Symfony uses underscores for service IDs. See
https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/Framew...

katbailey’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1599108.bundles.103.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
97 KB

*sigh*
I had attempted to deWTFify the language_manager() function in the last patch, didn't think it would break anything. When will I learn...

aspilicious’s picture

Green! Time to nitpick

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -0,0 +1,39 @@
\ No newline at end of file

Needs a newline

+++ b/core/modules/system/tests/modules/bundle_test/lib/Drupal/bundle_test/BundleTestBundle.phpundefined
@@ -0,0 +1,19 @@
\ No newline at end of file

newline

+++ b/core/modules/system/tests/modules/bundle_test/lib/Drupal/bundle_test/TestClass.phpundefined
@@ -0,0 +1,28 @@
\ No newline at end of file

again ;)

-27 days to next Drupal core point release.

Crell’s picture

Mostly nitpicky stuff. This overall looks awesome!

+++ b/core/lib/Drupal/Core/CoreBundle.php
@@ -0,0 +1,76 @@
+<?php
+
+namespace Drupal\Core;

Needs a @file declaration.

+++ b/core/lib/Drupal/Core/CoreBundle.php
@@ -0,0 +1,76 @@
+/**
+ * This is where Drupal core registers all of its services to the Dependency
+ * Injection Container. Modules wishing to register services to the container
+ * should extend Symfony's Bundle class directly, not this class.
+ */

DocBlock doesn't conform to standards. :-) Needs a one line summary.

+++ b/core/lib/Drupal/Core/CoreBundle.php
@@ -0,0 +1,76 @@
+    $container->register('dispatcher', 'Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher')
+      ->addArgument(new Reference('service_container'));

This is referencing the service_container service, but one does not exist. I assume that's the magic "this" reference in the container to itself? (We probably don't need to document that if it's the case, since it's probably documented on the Symfony side.)

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -0,0 +1,91 @@
+  protected function buildContainer() {
+    $container = $this->getContainerBuilder();
+
+    // Merge in the minimal bootstrap container.
+    if ($bootstrap_container = drupal_container()) {
+      $container->merge($bootstrap_container);
+    }
+    foreach ($this->bundles as $bundle) {
+      $bundle->build($container);
+    }
+    $container->compile();
+    return $container;
+  }

I thought you said you removed the bootstrap container?

I agree that we probably don't want a split container. I'm hoping that bootstrap goes away as a concept so the question here does, too.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -0,0 +1,91 @@
+  /**
+   * This method is part of the KernelInterface interface, but takes an object
+   * implementing LoaderInterface as its only parameter. This is part of the
+   * Config compoment from Symfony, which is not provided by Drupal core.
+   *
+   * Modules wishing to provide an extension to this class which uses this
+   * method are responsible for ensuring the Config component exists.
+   */

Bad Docblock. Needs one line summary.

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -30,7 +30,7 @@ class FinishResponseSubscriber implements EventSubscriberInterface {
     // Set the Content-language header.
-    $response->headers->set('Content-language', drupal_container()->get(LANGUAGE_TYPE_INTERFACE)->langcode);
+    $response->headers->set('Content-language', language_manager(LANGUAGE_TYPE_INTERFACE)->langcode);

Can we not get the language manager direct from the DIC at this point? If we can, then it should be a dependency for this subscriber class. That eliminates the function call.

+++ b/core/lib/Drupal/Core/EventSubscriber/PathListenerBase.php
@@ -21,9 +21,5 @@ abstract class PathListenerBase {
-    // @todo Remove this line once code has been refactored to use the request
-    //   object directly.
-    _current_path($path);

Yay!

+++ b/core/lib/Drupal/Core/EventSubscriber/PathSubscriber.php
@@ -80,11 +77,7 @@ class PathSubscriber extends PathListenerBase implements EventSubscriberInterfac
-    _current_path($path);
     drupal_language_initialize();
-    $path = _current_path();
-
-    $this->setPath($request, $path);

Wait a tick. How is this working? drupal_language_initialize() as far as I can tell is not setting the path, so if we have to change the path to support language this should break.

+++ b/core/lib/Drupal/Core/ExceptionController.php
@@ -27,6 +29,22 @@ class ExceptionController extends ContainerAware {
   /**
+   * Factory method for getting an Exception Listener. Since this needs to be
+   * instanciated with a controller callable, i.e. an ExceptionConroller object
+   * and the name of the method to call, we can't just register it to the DIC
+   * the regular way.
+   *
+   * @todo: This probably doesn't belong here, but I'm not sure where would be a better
+   * place to put it... in a class of its own?
+   */

Bad docblock. Needs one line summary.

Crell’s picture

FileSize
239.09 KB

I pushed a new commit with fixes for the obvious docblock and whitespace fixes. It looks like the patch that was posted was a bit old though, since the bootstrap container stuff is already gone.

Here's a new patch off of the sandbox, for testbot.

Status: Needs review » Needs work

The last submitted patch, 1599108-bundles.patch, failed testing.

katbailey’s picture

Sorry - I had forgotten to update the remote branch :-(
I will fix things later on and explain the bootstrap container thing, have no no time right now, just wanted to quickly explain why the last patch failed.

chx’s picture

+++ b/core/includes/path.incundefined
@@ -213,6 +213,14 @@ function drupal_cache_system_paths() {
+    // @todo Because caching happens during $kernel->terminate(), i.e.

There is no mention whether this makes the cache miss or not.

+++ b/core/includes/path.incundefined
@@ -358,6 +366,9 @@ function drupal_match_path($path, $patterns) {
+  if (drupal_container()->has('request')) {
+    return drupal_container()->get('request')->attributes->get('system_path');
+  }
   return _current_path();

I think a comment is needed here, something like "if there is no request (because....)"

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterKernelListenersPass.phpundefined
@@ -0,0 +1,37 @@
+        throw new \InvalidArgumentException(sprintf('Service "%s" must implement interface "%s".', $id, $interface));

This needs a use, doesn't it? That's what Crell told me, anyways.

katbailey’s picture

Status: Needs work » Needs review
FileSize
98.61 KB

This is referencing the service_container service, but one does not exist.

The container gets set on itself as 'service_container' in the constructor. See https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Dep....

I thought you said you removed the bootstrap container?

The patch in #103 includes a change that I then took out again and created as a patch on #1703266: Use a precompiled container for the minimal bootstrap container required outside of page requests. Here's the change: http://drupal.org/files/precompiled_bootstrap_dic.patch
I had then forgotten to update the remote branch so this commit was left in (and the fix for the PathAliasTest left out of course).

Sun suggested there may be problems with having that stuff precompiled because of needing to have some of it configurable in settings.php. Rather than have CMI considerations hold this patch up I moved it to its own issue. But we still do need a minimal container that has what's needed any time a full bootstrap is required but there's no DrupalKernel involved which builds the full DIC. The idea of grabbing this minimal container and merging it in when building the full one was purely to avoid having the exact same lines of code in two places. But perhaps that would in fact be less dirty than what I'm doing here. Alternative suggestions welcome!

Can we not get the language manager direct from the DIC at this point? If we can, then it should be a dependency for this subscriber class.

Yes indeed, I have made this change in the latest version of the patch.

Wait a tick. How is this working? drupal_language_initialize() as far as I can tell is not setting the path, so if we have to change the path to support language this should break.

drupal_language_initialize() calls language_manager() on each of the language types. When this happens within request scope, the LanguageManager calls language_types_initialize($type, array('request' => $this->request)); and so the request is being sent to the language negotiation function, which gets the path from the request, splits it into prefix and system path, then sets the system_path attribute on the request. I've left a todo in the LanguageManager class about OOPifying the language system because it's a bit nasty at present.

This patch addresses chx's comments in #112 as well.

Crell’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -16,6 +17,12 @@ use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 
+  private $language_manager;

This should be protected $languageManager. :-(

That's the only thing I can find here. Let's fix that and call it done. Awesome work, Kat!

katbailey’s picture

Status: Needs work » Needs review
FileSize
98.61 KB

Done :-)

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Done indeed. :-)

Committers: I'd love to see this one merged rather than simply applied. The branch should be quite clean right now (and I also have some other downstream branches that are now based on this work, so committing it directly would make rebasing my code more difficult).

This is the branch you'll want to merge from: http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/bundles

Crell’s picture

See #1269742-28: Make path lookup code into a pluggable class for a follow-up. This issue does regress path alias caching temporarily, but per discussion with Dries at MWDS we're OK with that given that follow-up issue.

andypost’s picture

How this issue will affect subsystem to ban users? Suppose registering should have ability to interupt request. Can someone comment this in #1161486: Move IP blocking into its own module

catch’s picture

OK this isn't a proper review yet, will try to have a better look in the next day or so.

Fine with regressing path caching but let's bump the other issue to critical so it definitely gets done once this is in. edit, that was done ten hours ago :)

This @todo:

+    // TODO: Somehow remove the necessity of calling system_list() to find out which
+    // bundles exist.
+
+    $modules = array_keys(system_list('module_enabled'));
+    foreach ($modules as $module) {

Looks like this #1331486: Move module_invoke_*() and friends to an Extensions class or very similar.

Also just from scanning the patch I can't immediately tell when this code runs, is it going to happen for cached page requests too? And if we switch to HttpCache is it going to need to run for cached pages then? If so let's do some profiling before the patch is committed to see what we're losing there, or that might be worth doing anyway.

sun’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs architectural review

In addition to @catch's comment, I'm afraid but I think the final proposed code could use some more architectural reviews. This is a massive change to Drupal's architecture. I perfectly understand that the changes are key for enabling/unblocking other issues, but I don't think it is appropriate to rush this into core.

I spared some hours to have some quality time with reviewing the bundles branch. Here's what I found:

  1. It is not entirely clear to me how this impacts tests.

    I have the "gut feeling" that at least UnitTestBase would have to be adjusted to run with a fresh/new container? (since they are not supposed to run within a request, or rather, with the compiled container of the parent request that executes the test, if any)

    Contrary to that, WebTestBase enables additional modules within a running request. Something very similar actually also happens with regard to module_enable() and module_disable() outside of tests. If I get these changes right, then 1) a module that is enabled later in the request is not able to participate in events of that request, and 2) a module that is disabled in a request will still participate in events of that request, despite being disabled?

  2. Drupal\Core\DependencyInjection\Container is replaced with a new ContainerBuilder implementation. The bare essentials of the original code are moved and embedded into drupal_container() (for runtime execution without a Request). The inlined/embedded code feels a bit icky.

    Wouldn't it make sense to move that code into a new (old) Drupal\Core\DependencyInjection\NonRequestContainer? (give or take a better name)

  3. It is still not entirely clear to me why Drupal\Core\Language\LanguageManager has a hard dependency on the Request. It looks like if that request parameter would be optional, then LanguageManager could be re-used and shared as initialization and sorta factory for both the request-based and non-request container, eliminating the new procedural language_manager() helper. What am I missing?

    Furthermore, I've the impression that these changes could really use some more detailed reviews from language subsystem maintainers. (erm, so consider this the first ;) ...not sure whether @plach is up to speed with the new kernel stuff, but we should ping him)

  4. DrupalKernel::registerBundles() executes this code:

          $camelized = ContainerBuilder::camelize($module);
          $class = "\Drupal\\{$module}\\{$camelized}Bundle";
    

    However, in #1550680: Automatically convert PSR-0 namespaces for extensions into upper CamelCase the idea of auto-camelizing module names in classes and namespaces was rigorously shot down. Therefore, this seems to be a quite massive change in direction. I'd like to have some more clarity on that.

  5. The event subscriber names that are registered on the container in CoreBundle::build() look a bit arbitrary to me;

    dispatcher
    resolver
    http_kernel
    matcher
    router_listener
    content_negotiation
    view_subscriber
    access_subscriber
    maintenance_mode_subscriber
    path_subscriber
    ...
    database
    database.slave
    exception_listener
    request
    language_manager
    

    — is there no standard naming/pattern for these? Some are using single words, some are using underscores, some others dots, some are prefixing the subject/type/topic, some others suffixing it. The database* names look more appropriate to me.

    I also wonder whether all the *subscribers shouldn't be in the form of 'subscriber.[type]', possibly even with an additional request. prefix?

    The primary goal of this issue is to allow modules to register services and events, so there will be many more and thus I think a proper naming pattern should be an essential part of this patch.

  6. I think we should add a @todo to index.php/install.core.inc or the DrupalKernel class to clarify that we need to figure out how to handle the Kernel($environment, $debug) constructor parameters.

  7. It does not seem we are compiling the container into a static file/cache with this patch, but re-compiling it on every request. Because of that, I'm not sure I understand why the changes to compile the container are actually part of this patch? Can you help me out?

pounard’s picture

About 1) It's obvious that unit tests must use their own container, which is not compiled. By doing this we can have a minimal container for each unit test, registering only the services needed for the active test.

For functional tests (web tests) it doesn't imply any change, since the site under is supposed to be as close as possible to a real site. Tests will probably be slower because the tested site may not cache the compiled container, but regarding the performance issue, having a unit test specific container will allow to move a lot of existing web tests as unit tests, thus making them run a lot faster, so I guess it's a good tradeoff here.

I'm just guessing here, I might totally wrong because I don't know much about simpletest internals. I can't answer the others, but those are good questions.

Crell’s picture

Re #120:

1) Actually, Unit tests in most cases would not use the DIC at all. They should be testing individual objects, which means the test itself can create whatever dependencies, or better yet mock dependencies, are needed. See the latest patch in #1606794: Implement new routing system for an example of several tests taking that approach.

If we're testing something that does need a container for some reason, then we can prepare a container as part of that particular test (uncompiled) and pass it into the object being tested right there.

That sort of separation and ease of testing is exactly the point of a DIC.

For WebTest, I'm not entirely sure. There's some really funky stuff that goes on there. At least in the typical case it seems we don't need to do anything differently, as most tests we have now are still passing fine.

2) Thinking about it, I'm not entirely certain why that's hard coded there. Kat, is there a reason we don't just have the config system in the CoreBundle?

3) LanguageManager depends on the request because the active language frequently comes from the request via the path, domain, or HTTP headers. As for getting reviews from the language folks, it's here waiting. :-) The language system has all sorts of spaghetti that we're slowly peeling apart in this process, so I expect it will get refactored several more times before we're done.

4) Module names for namespaces, yes. For classes, our coding standards do specify CamelCase. I'm a bit leery of that here, but I am more leery of putting underscores in class names as that triggers an extra directory per PSR-0. (That's not true of the namespace part of the class name, only the last part.) So we cannot have underscores in class names unless we also want extra directory layers, which I don't believe we do.

5) The best I could find for a Symfony standard for service names is a list of framework tag names: http://symfony.com/doc/current/reference/dic_tags.html

It looks like they use . for quick-n-dirty namespacing, and _ for multi-word strings. That's what we're doing here. We probably could bikeshed the names at length, but I'd rather not do that here when there is other work dependent on getting this in place. (Also, request.subscriber.* is a no-go because one subscriber can, by design, listen to both request and non-request events. I'd be OK with subscriber.* or subscriber.$module.*, though. Really, those IDs will virtually never be used directly.)

6) No objection. Perhaps we should also note this issue in #1537198: Add a Production/Development Toggle To Core

7) The setup for a compiled DIC is different than for an all-runtime DIC. We know from #1675260: Implement PHP reading/writing secured against "leaky" script that we're going to be using a compiled DIC in some form, most likely PHP as soon as that's sufficiently secured. Preparing for a compiled world now means that it's not an API change when we start storing the compiled DIC later.

I've opened a new issue for when we are able to do that: #1706064: Compile the Injection Container to disk

katbailey’s picture

FileSize
99.11 KB

From @catch in #119:

I can't immediately tell when this code runs, is it going to happen for cached page requests too?

I just made the following change to index.php that cuts out the explicit call to $kernel->boot():
http://drupalcode.org/sandbox/Crell/1260830.git/commitdiff/6bb79c3953c84...
This had been a hold-over from when there was code that had needed to run prior to the handling of the request but that was reliant on the fully initialized DIC. Now we can just let the booting of the kernel happen when the request is handled. What this means is that when #1597696: Consider whether HttpCache offers any significant benefit over the existing page cache lands, the answer to @catch's question above will be no, i.e. when the HttpCache is handling the request, and there is a fresh page available in the cache, the process of booting the kernel, which entails calling the build() method of all the bundles, will not happen. As long as we're still using the old caching system though, this booting will happen on all requests.

To Sun's points:

If I get these changes right, then 1) a module that is enabled later in the request is not able to participate in events of that request, and 2) a module that is disabled in a request will still participate in events of that request, despite being disabled?

I'm not sure how serious an issue this will be, it's hard to come up with a scenario where this would be really problematic, but it could well be something we'll need to deal with. I'm just not sure it should hold up this patch..?

Wouldn't it make sense to move that code into a new (old) Drupal\Core\DependencyInjection\NonRequestContainer?

Well, I'm actually hoping that we'll make this change instead: #1703266: Use a precompiled container for the minimal bootstrap container required outside of page requests. In the meantime, drupal_container() seems as good a place as any to leave that code, but I don't feel terribly strongly about it.

It is still not entirely clear to me why Drupal\Core\Language\LanguageManager has a hard dependency on the Request

Here's a transcript of the irc discussion we had with igorw about this a couple of weeks ago: https://gist.github.com/8d58d47bd7a3c15de207. In particular, this comment from igorw sums it up: "if you need a reference to the request, and check that over and over again, you'll need a scoped service". So, LanguageManager is a scoped service, and so it simply cannot be used outside of the scope of a request. I am hoping that as we OOPify more of core, we will gradually be able to remove many of the calls to the language_manager() wrapper.

Re point 4, I don't really have an opinion on this and was not involved in the PSR-0 discussion. We just need to be able to register modules' bundles, so being able to identify them is obviously key.

The event subscriber names that are registered on the container in CoreBundle::build() look a bit arbitrary to me

For the most part, the underscored names correspond to a camelized class name (e.g. 'access_subscriber' for AccessSubscriber). The 'service_container' id is decreed by Symfony because that's what it registers the container to itself as. I guess 'dispatcher' and 'resolver' should probably be changed to 'event_dispatcher' and 'controller_resolver' respectively, to match the base class names. As for 'database' and 'database.slave', in both cases the class name is actually Connection, but that is not a very helpful name for a service :-/ And the dot serves to show that it is not a different class but that the object is instantiated differently. OK, this is a bit of post-hoc confabulation on my part :-P I mean, sure we can work on nailing this down, but could it maybe go into a follow-up issue?

Point 6, done.

I'm not sure I understand why the changes to compile the container are actually part of this patch

Can you point to exactly which changes you mean? It's possible that my response in #66 might answer your question here.

moshe weitzman’s picture

Contrary to that, WebTestBase enables additional modules within a running request. Something very similar actually also happens with regard to module_enable() and module_disable() outside of tests.

How come the WebTestCase tests are passing now (even with run-tests.sh)?

Drush does a lot of module_enable() inside a request as well. We don't have to solve this in this issue, but I do think we need a plan before this is committed. There has to be a way for code to enable/disable a module and then tell Drupal to recalculate some or all of its "environment", including writing a new secure PHP file. And this should be doable from inside same request.

katbailey’s picture

I cross-posted with Crell last night.

is there a reason we don't just have the config system in the CoreBundle?

Because it is needed at times when there is no CoreBundle, e.g. any time a full bootstrap of the codebase is done outside of the context of a page request. One example of this is run_tests.sh. I have given no thought to whether the rest of the services provided by the CoreBundle ought to be made available to stuff that runs without a kernel, e.g. drush :-/ Another reason I had needed it was when exceptions were being thrown during the booting of the kernel, which resulted in additional exceptions being thrown while handling the exception. It was painful. But this was mainly due to the language services that were in the container and were needed to handle exceptions. This is no longer the case. And of course there will never ever be another exception thrown during the booting of the kernel ;-)

How come the WebTestCase tests are passing now (even with run-tests.sh)?

Here's my limited understanding of what's happening. If you were to call drupal_container() from within a WebTestCase test, you would get the container of the main environment, not the test environment. So, if using run_tests.sh all that would be in it would be the config system services. However, when an actual request is made, e.g. using drupalGet() or drupalPost(), the full container for the test environment gets booted up for that request. That's why I had to build the BundleTestCase the way I did, i.e. using a menu callback in the test module that calls drupal_conatiner().

If this is all complete insanity, it would be good to have some use cases to work with so as to figure out exactly what's lacking in the current approach.

Dries’s picture

Status: Needs review » Needs work

First review.

+++ b/core/includes/install.core.incundefined
index 0892cc4..48ba3d6 100644
--- a/core/includes/language.inc

--- a/core/includes/language.inc
+++ b/core/includes/language.incundefined

+++ b/core/includes/language.incundefined
+++ b/core/includes/language.incundefined
@@ -110,7 +110,7 @@ const LANGUAGE_NEGOTIATION_DEFAULT = 'language-default';

@@ -110,7 +110,7 @@ const LANGUAGE_NEGOTIATION_DEFAULT = 'language-default';
  * @return
  *   The negotiated language object.
  */
-function language_types_initialize($type) {
+function language_types_initialize($type, $args = array()) {
   // Execute the language negotiation methods in the order they were set up and

1. $args is not documented. Would be good if we could give it a less generic name as well. Should it be $request?

2. As a developer, how do I know what to pass as $args? Depending on the callback, it may need to have different things? So, it feels like I need to know what callback is going to be invoked.

3. Any chance the different callbacks take the same arguments? If so, we may be able to clean this up further.

+++ b/core/includes/path.incundefined
@@ -213,6 +213,17 @@ function drupal_cache_system_paths() {
+    // the key depends on when the first call to url() is made for that path.
+    // If this is within the request scope, it uses the system_path and so will
+    // be a cache miss. There is a separate issue for fixing up the path alias

Let's make sure this issue is marked as critical. (Update: it is currently marked as critical.)

+++ b/core/includes/path.incundefined
@@ -358,6 +369,11 @@ function drupal_match_path($path, $patterns) {
 function current_path() {
+  if (drupal_container()->has('request')) {
+    return drupal_container()->get('request')->attributes->get('system_path');
+  }
+  // If we are outside the request scope, e.g. during $kernel->terminate, fall
+  // back to using the path stored in _current_path().
   return _current_path();

This seems like a bit of a hack.

Is the fallback going stay ? Should we throw an exception instead and see what breaks, and evolve things like our cache writing to happen sooner? It may not be too much work to fix this properly. At a minimum, we may want to add a @todo?

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterKernelListenersPass.phpundefined
@@ -0,0 +1,38 @@
+class RegisterKernelListenersPass implements CompilerPassInterface
+{
+  public function process(ContainerBuilder $container)
+  {
+    if (!$container->hasDefinition('dispatcher')) {

Curly brackets in the wrong place?

+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.phpundefined
index 0000000..6b20187
--- /dev/null

--- /dev/null
+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -0,0 +1,93 @@

@@ -0,0 +1,93 @@
+<?php
+
+/**
+ * @file
+ * Definition of Drupal\Core\DrupalKernel.
+ */

It feels like a good idea to describe what this class does; e.g. stating that each module can have a Bundle class.

+++ b/core/lib/Drupal/Core/HttpKernel.phpundefined
@@ -0,0 +1,51 @@
+    public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQUEST, $catch = true)
+    {
+        $request->headers->set('X-Php-Ob-Level', ob_get_level());
+
+        $this->container->enterScope('request');
+        $this->container->set('request', $request, 'request');
+
+        try {
+            $response = parent::handle($request, $type, $catch);
+        } catch (\Exception $e) {
+            $this->container->leaveScope('request');
+
+            throw $e;
+        }
+
+        $this->container->leaveScope('request');
+
+        return $response;

Would be good to explain why we do this scope management. I'm not sure I understand the purpose of this wrapping.

+++ b/core/modules/language/language.negotiation.incundefined
@@ -212,8 +212,22 @@ function language_from_url($languages) {
-      list($language, $path) = language_url_split_prefix(_current_path(), $languages);
-      _current_path($path);
+      if (isset($args['request'])) {
+        $request = $args['request'];
+
+        $current_path = $request->attributes->get('system_path');
+        if (!isset($current_path)) {
+          $current_path = trim($request->getPathInfo(), '/');
+        }
+      }
+      else {
+        $current_path = _current_path();
+      }
+
+      list($language, $path) = language_url_split_prefix($current_path, $languages);
+      if ($request) {
+        $request->attributes->set('system_path', $path);

Seems like a candidate for clean-up. A @todo might be appropriate.

effulgentsia’s picture

Status: Needs work » Needs review

If this is all complete insanity, it would be good to have some use cases to work with so as to figure out exactly what's lacking in the current approach.

I opened #1708692: Bundle services aren't available in the request that enables the module for this. I think we need the container to get rebuilt within module_enable() and module_disable() for reasons stated in that issue. Let's keep the discussion about that specific issue there, and update this issue when there's a resolution.

effulgentsia’s picture

Status: Needs review » Needs work

x-post.

effulgentsia’s picture

FileSize
1.12 KB

let's do some profiling before the patch is committed to see what we're losing there

Here's a patch that can be applied on top of #123 that implements a very insecure dumping of the DIC to a PHP file. Attaching as a txt file to skip bot. The issue to do this for real is #1706064: Compile the Injection Container to disk.

On my machine, APC enabled, XDebug disabled, fresh install of Standard profile, running "ab -c1 -n500" on the homepage (with no content, just the "No front page content has been created yet." message), I get:

HEAD: 41.7ms
#123: 50.9ms
#123 + this patch: 44.2ms

How concerning is the extra 2.5ms even with DIC on disk? Is that something we need to get to the bottom of before this patch lands, or can it be a follow up?

catch’s picture

Those numbers are quite frightening to be honest, given we're not even using this for anything.

sun’s picture

Status: Needs work » Needs review

I was getting similar results for the HEAD/Patch difference and wanted to post here, but got distracted by reviewing and searching for improvement options in the code of the bundles branch. I benched:

Fresh Minimal on HEAD vs. bundles branch, APC enabled, front page without any content.

HEAD:

$ ab -n 100 http://drupal8.test/scratch/

Concurrency Level:      1
Time taken for tests:   10.130 seconds
Complete requests:      100
Total transferred:      654200 bytes
HTML transferred:       602300 bytes
Requests per second:    9.87 [#/sec] (mean)
Time per request:       101.296 [ms] (mean)
Time per request:       101.296 [ms] (mean, across all concurrent requests)
Transfer rate:          63.07 [Kbytes/sec] received

Patch:

$ ab -n 100 http://drupal8.test/scratch/

Concurrency Level:      1
Time taken for tests:   13.652 seconds
Complete requests:      100
Total transferred:      653800 bytes
HTML transferred:       601900 bytes
Requests per second:    7.33 [#/sec] (mean)
Time per request:       136.518 [ms] (mean)
Time per request:       136.518 [ms] (mean, across all concurrent requests)
Transfer rate:          46.77 [Kbytes/sec] received

We'll definitely have to bump #1706064: Compile the Injection Container to disk to a critical bug after this patch lands.

In any case, I think we need to figure out what exactly is causing the slow-down. Obviously, based on a compiled container.

Nevertheless, based on @effulgentsia's benchmarks, it is very odd that the current dynamic and non-compiled ContainerBuilder is still faster than the compiled one. (?) This almost sounds like the new code triggers a code path for the compiled container that is not supposed to be triggered for it...?

We also have take into account that the initial HttpKernel commit decreased overall performance by 10% already. We cannot continue this direction forever - unless there's a patch in the queue that brings back a 20% performance improvement which I might have missed?

effulgentsia’s picture

Status: Needs review » Needs work
FileSize
1.86 KB

This one gives me parity between HEAD and #123+this. Approx 41.3ms on each, though approx 0.5ms stddev even across groups of -n500.

sun’s picture

Now, stupid question... why don't we incorporate eff's patch from #132, replace PhpDumper with YamlDumper, and supply config_get_config_directory() . '/container.yml' to it?

effulgentsia’s picture

Status: Needs work » Needs review

Back to needs review. Despite work needed for #120 and below, it would be good to get some more reviews here as well.

effulgentsia’s picture

Re #133: we can try that, but all the loaders for the non-php dumpers are notably slower than require_once, so I think it would be a temporary measure, and we'll need to do #1706064: Compile the Injection Container to disk anyway. Plus, YamlFileLoader and the other FileLoader classes depend on FileLocator which is in the Symfony\Component\Config component, which we don't have in Drupal yet and don't have any other use for yet. So I don't think it's worth it. Basically, I think all those loaders are designed for allowing Symfony bundle and application developers to write out their service entries in the file format of their choice, but not for any significant performance optimization.

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
@@ -7,113 +7,29 @@
   public function __construct() {
     parent::__construct();
+    $this->compiler = new Compiler();
   }

+  public function addCompilerPass(CompilerPassInterface $pass, $type = PassConfig::TYPE_BEFORE_OPTIMIZATION) {
+    $this->compiler->addPass($pass, $type);
+  }
 
+  public function compile() {
+    $this->compiler->compile($this);
+    $this->parameterBag->resolve();
   }

I love all the - signs in this class (not shown above), but was confused by the + signs, because it looks like we're overriding Symfony's compilation logic for reasons not documented. Ideally, we should be able to remove this class entirely and use Symfony's with no overrides, but we can't quite yet. #1711492: Clarify why Drupal requires a custom ContainerBuilder gets us close though.

effulgentsia’s picture

Status: Needs review » Needs work

In addition to #127 and #136, here's some nits.

+++ b/core/includes/bootstrap.inc
@@ -2633,34 +2631,67 @@ function get_t() {
+function language_manager($type = NULL) {

We need a PHPDoc for $type, especially since passing NULL invokes a reset. This function is a bit of a WTF, but we can deal with that in follow ups.

+++ b/core/includes/install.core.inc
@@ -1416,6 +1412,9 @@ function install_bootstrap_full(&$install_state) {
+  // @todo Figure out how best to handle the Kernel constructor parameters.

I don't know what this means. If there's an issue, please link to it. Otherwise, please explain the problem that needs to be figured out.

+++ b/core/includes/language.inc
@@ -110,7 +110,7 @@ const LANGUAGE_NEGOTIATION_DEFAULT = 'language-default';
-function language_types_initialize($type) {
+function language_types_initialize($type, $args = array()) {

We need a PHPDoc for the new parameter. It looks like all that $args contains currently is $request. If so, can we change this to $request, and wait for a need of another object before abstracting to $args. If in follow ups, we improve our language negotiation integration with the kernel, this function might go away before such a need arises, but meanwhile, it will help with clarity.

+++ b/core/includes/path.inc
@@ -213,6 +213,17 @@ function drupal_cache_system_paths() {
+    // @todo Because caching happens during $kernel->terminate(), i.e.
+    // after we have left the scope of the Request, the call to current_path()
+    // here does not return the system path, and instead calls out to
+    // _current_path(), which returns the alias. So the alias gets used as the
+    // key instead. At lookup time, whether the alias or the source is used as
+    // the key depends on when the first call to url() is made for that path.
+    // If this is within the request scope, it uses the system_path and so will
+    // be a cache miss. There is a separate issue for fixing up the path alias
+    // logic here: http://drupal.org/node/1269742

- Please add 2 spaces after the // in all but the first line.
- Does _current_path() always return the alias? We have several places that initialize _current_path() with the normalized path.

+++ b/core/includes/path.inc
@@ -358,6 +369,11 @@ function drupal_match_path($path, $patterns) {
+  // If we are outside the request scope, e.g. during $kernel->terminate, fall
+  // back to using the path stored in _current_path().
   return _current_path();

If terminate subscribers like the cache logic above are the only use case for this, let's remove this, and change the caller(s) to call _current_path() directly. Otherwise, let's give at least one other common example in this comment.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -0,0 +1,92 @@
+  public function registerBundles() {

Needs a PHPDoc.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -0,0 +1,92 @@
+    // TODO: Remove the necessity of calling system_list() to find out which
+    // bundles exist. See http://drupal.org/node/1331486

Please change to @todo, no colon, and indent 2nd line.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -0,0 +1,92 @@
+      $class = "\Drupal\\{$module}\\{$camelized}Bundle";

Double quotes so either the first slash should be doubled or removed. I prefer removed.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -0,0 +1,92 @@
+    // @todo We should be compiling the container and dumping to php so we don't
+    // have to recompile every time. There is a separate issue for this, see
+    // http://drupal.org/node/1668892.

Please indent 2nd and 3rd lines.

+++ b/core/lib/Drupal/Core/ExceptionController.php
@@ -27,6 +29,24 @@ class ExceptionController extends ContainerAware {
+   * @todo: This probably doesn't belong here, but I'm not sure where would be a better
+   * place to put it... in a class of its own?

No colon, line wrap at 80, indent 2nd line.

+++ b/core/modules/system/tests/http.php
@@ -32,10 +33,10 @@ if (!drupal_valid_test_ua()) {
+$kernel->boot();
 $response = $kernel->handle($request)->prepare($request)->send();

boot() isn't needed when the next line is handle().

+++ b/core/modules/system/tests/https.php
@@ -31,10 +32,10 @@ if (!drupal_valid_test_ua()) {
+$kernel->boot();
 $response = $kernel->handle($request)->prepare($request)->send();

ditto

pounard’s picture

@#129,#130 Please don't forget that this brings a whole new set of tools for managing early bootstrap events and HTTP request handling which wasn't really possible before, this is not for nothing. Plus, all those benchmarks are being done on developer's environments, where the disk may be faster than on some other environments (I bet that if I test on the Win64 environment I'm using right now, I would have a huge performance boost using the compiled version). Please also consider that all those micro benchmarks are not fully relevant because they all test a vanilla core, no contrib, no custom listeners, no custom services, etc...

Dries’s picture

I'm very concerned about the performance impact.

With the bundle patch, we should be able to remove _boot(), _init(), and hook_exit(), right? Could we do that in this patch to see if that would allow us to recoup (some of) the performance loss? I don't think that will make a material difference but it might be worth it.

(Also, it is not clear why this patch blocks us from moving to Symfony's routing system, which I believe was cited as the reason we need this, and why we need to work on this first/now.)

effulgentsia’s picture

Assigned: Unassigned » effulgentsia

I looked more into the performance impact and will post that info in the next day or two.

Crell’s picture

Dries: The new routing system needs to be wired into the system (I realized the other day it's not quite ready for that yet; pwolanin is working on a generator in the meantime), and the wiring method changes completely with this patch. So it's not that we can't work on routing without this, it's that some of the work to integrate everything changes after this patch.

I am reluctant to kill off several hooks in this patch, for the simple reason that it's already 100K and we don't want to harm any more kittens. One issue, one task.

effulgentsia’s picture

Assigned: Unassigned » effulgentsia

#1715940: Remove DIC compilation until it can be dumped to disk fixes the 2 major performance regressions:
- tripling the number of DIC entries and compiling the DIC without also dumping it to disk
- checking class_exists() for a bundle class for each module without skipping the autoloader

The patch there is an interim step to unblock this issue and the issues that depend on it. Assuming Crell commits that to the branch and updates the patch here, and that branch gets merged to core, the next step will be #1706064: Compile the Injection Container to disk which will allow adding entries to the DIC and taking advantage of compile passes without performance overhead.

Meanwhile, with that patch applied, the updated numbers for #129 are:
HEAD: 42.4ms (slower than #129's HEAD, but HEAD has changed since then)
patch: 43.4ms

Still a small regression, but an acceptable one I think, at this stage of the process.

effulgentsia’s picture

Assigned: effulgentsia » Unassigned
katbailey’s picture

Assigned: effulgentsia » Unassigned
Status: Needs work » Needs review
FileSize
111.22 KB

OK, I think this patch addresses everything with the exception of #1708692: Bundle services aren't available in the request that enables the module and #1716048: Do not boot bundle classes on every request. It includes effulgentsia's patches from #1711492: Clarify why Drupal requires a custom ContainerBuilder and #1715940: Remove DIC compilation until it can be dumped to disk. Both Dries and effulgentsia had commented about the passing of an $args parameter to the language negotiation functions and also the whole _current_path() vs current_path() mess with regard to the caching of system paths. Please see these commits for what I did there:
http://drupalcode.org/sandbox/Crell/1260830.git/commitdiff/b979e8aa93603...
http://drupalcode.org/sandbox/Crell/1260830.git/commitdiff/8ffe09eee5d91...

I have also changed the Drupal\Core\HttpKernel from being a class that only copies the handle() method from Symfony's FrameworkBundle to being a verbatim copy of it.

Let's see if testbot is ok with the new patch...

effulgentsia’s picture

#144: 1599108.bundles.144.patch queued for re-testing.

Dries’s picture

I brainstormed about this issue quite a bit and decided to merge it (commit it), as long as we make #1708692: Bundle services aren't available in the request that enables the module a critical.

Thanks all!

Dries’s picture

Status: Needs review » Fixed
tim.plunkett’s picture

Title: Allow modules to register services and subscriber services (events) » Change notification for: Allow modules to register services and subscriber services (events)
Priority: Major » Critical
Status: Fixed » Active
Issue tags: +Needs change record

I know that admin_menu switched to using request(), and there was a Views patch to do the same, and now it's gone.

That, and this commit really doesn't illustrate the changes module authors could and should make.

So, please can we have a change record for this?

Crell’s picture

Title: Change notification for: Allow modules to register services and subscriber services (events) » Allow modules to register services and subscriber services (events)
Priority: Critical » Major
Status: Active » Fixed

I've updated http://drupal.org/node/1539454 and http://drupal.org/node/1635626 based on this issue. I did not make a change notice yet for event registration since that's going to change considerably as soon as we get DIC compilation in. It's also just new functionality, not breaking existing functionality, so we can wait for that to settle a little bit.

fabpot’s picture

FYI, FrameworkBundle is actually already available as a Composer package: http://packagist.org/packages/symfony/framework-bundle

tim.plunkett’s picture

http://drupal.org/node/1539454 make no mention of language_manager(), isn't it replacing some of that example code?

Crell’s picture

fabpot: Bah! :-) If we're not using anything else from FrameworkBundle it seems like a lot of code to pull in for just one class. We can look into switching to that later if we find there's other stuff we want from there.

Tim: Mm, yes, it is. We should update that, too. I don't have time to at the moment, but can someone else jump in on that?

sun’s picture

I sorta predicted it and thus wanted #1215104-60: Use the non-interactive installer in WebTestBase::setUp() to land before this patch... did anyone test the non-interactive installer? :-/

effulgentsia’s picture

http://drupal.org/node/1539454 make no mention of language_manager()

Can we get #1719488: Rename language_manager() to language() and related cleanup in before we document a poorly named function?

katbailey’s picture

I've updated [#1539454] but have not made any mention of the language_manager() function yet.

Status: Fixed » Closed (fixed)

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

xjm’s picture

xjm’s picture

Issue summary: View changes

Explaining what the current patch actually does.

donquixote’s picture