Problem/Motivation

Following #1497230: Use Dependency Injection to handle object definitions , we need a common place to bootstrap objects in the drupal_container(). Currently, it registers the objects in drupal_container() itself, but we need a nicer place to register these bootstrap objects.

Proposed resolution

Have the Container build itself via one of the many Loaders.

Remaining tasks

The usual.... Write a patch, review a patch, patch a patch, wear eye patch.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

Title: Common interface to register object definitions » Bootstrap for the Dependency Injection Container
Status: Postponed » Active

Following #1497230: Use Dependency Injection to handle object definitions , we need a common place to register the very basic early configuration for the Drupal Container. These mappings arn't meant to perform any expensive operations, just meant as the initial mappings so that we can use the Container throughout Drupal's bootstrap sequence.

Symfony uses a configuration loader to build the Container:

use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\Config\FileLocator;
use Symfony\Component\DependencyInjection\Loader\XmlFileLoader;

$sc = new ContainerBuilder();
$loader = new XmlFileLoader($sc, new FileLocator(__DIR__));
$loader->load('bootstrap.xml');
<services>
    <service id="language" class="Drupal\Core\Language\Language">
        <argument>SomeStuffs</argument>
    </service>

    <service id="language_url" class="Drupal\Core\Language\Language">
        <argument>SomeOtherStuffs</argument>
    </service>
</services>

We probably don't want XML (there are a number of other options), but you get the idea. Again, this is just the base bootstrap objects that we need. Once the Kernel is in, we will likely extend it to use the event dispatcher so that these mappings can be built upon and modified.

Other option is to just stick all of it in something like drupal_container_bootstrap() or something, but it would be nice to have a configuration file for all this.

sun’s picture

Introducing new DI tag.

cosmicdreams’s picture

is _drupal_bootstrap_configuration a good place to include this logic?

RobLoach’s picture

FileSize
2.37 KB

Maybe something like the attached. We could move lots of the bootstrap in here.

Using one of the Dependency Injection Loaders would be great, but I don't think we need one of them at this stage. Would be nice to dump some of the dependency injection container configuration to a file during the site installation, but we're not quite there yet.

cosmicdreams’s picture

Status: Active » Needs review

There's a bit of code that runs that looks at every language type and registers it to the DI. I can't remember exactly where that code is but it's there.

Does this approach require that we manually register everything?

RobLoach’s picture

There's a bit of code that runs that looks at every language type and registers it to the DI. I can't remember exactly where that code is but it's there.

drupal_language_initialize() is what you're looking for?

Does this approach require that we manually register everything?

Anything that we want as early as possible that does not require any expensive processing, yes. I'm thinking this is just for the defaults of stuff that could be overriden later on.

What are your thoughts? It would be nice to dump some of this configuration somewhere on initial site set up, like Symfony does, but I'm not sure we're at that stage in Drupal yet.

RobLoach’s picture

Status: Needs review » Needs work

Ah, I understand what you mean. Move some/all of drupal_language_initialize() into the Language class so that the Container can lazy-load the Language system?

cosmicdreams’s picture

in response to #7: Yes, exactly

RobLoach’s picture

Status: Needs work » Needs review
FileSize
13.36 KB

We probably couldn't get rid of the language bootstrap all together, could we?

cosmicdreams’s picture

Status: Needs review » Needs work
FileSize
3.03 KB

Here's an attempt to do that, that is probably wrong:

Crell’s picture

Registering certain container objects in the DIC's constructor means that it is tightly coupled to the Drupal\Core\Language\Language class, and will fail without it. This strikes me as defeating the whole purpose of using a DIC, which is looser coupling.

cosmicdreams’s picture

+++ b/core/lib/Drupal/Core/Language/Language.phpundefined
@@ -41,13 +43,46 @@ class Language {
+      // Check if we are to bootstrap the Language system.
+      if (!$container->has('language.bootstrap')) {

Here we use 'language.bootstrap' where in other places we have language_url. It would be great to be consistent about how we name these things. The dot notation seems more robust. We should use that.

sun’s picture

I somewhat shared @Crell's concern at second sight, but since this merely registers the service but doesn't actually do something unless the service is accessed, I do not actually see a problem with the proposal.

It basically says: "Oh, you want to bootstrap Drupal? Well then, you need at least this. Feel free to mock/replace it, but Drupal requires it to be there."

sun’s picture

Also, I'm not sure why it failed, and even more so, I'm not sure whether it is technically correct, but I like the direction that @Rob Loach investigated with entirely replacing drupal_language_initialize() in #9. I'd imagine a DI-enabled bootstrap to work like that, or along the lines of that. However, I'd recommend to make that a follow-up issue.

cosmicdreams’s picture

from the automated patch review: most of the exceptions are from a the fact that the code is looking for language_content and can't find it.

Perhaps the it isn't being set in the DI code. Previously the list of all types was retrieved like this:

$types = language_types_get_all();

In #9 the set of types was manually defined.

RobLoach’s picture

However, I'd recommend to make that a follow-up issue.

#1550866: Remove obsolete drupal_language_initialize()

RobLoach’s picture

Status: Needs work » Needs review
FileSize
2.38 KB

Having the Container built from a PhpFileLoader accomplishes exactly what we have here. This just cleans up where the initial Drupal bootstrap configuration is set up. As sun pointed out, this is just the basic object mappings and no logic is actually taken place. It's just the mappings of what's required for a basic Drupal bootstrap.

If you have any other suggestions, I'm all ears. Having a dependency injection container that we can use throughout the Drupal bootstrap process is essential.

cosmicdreams’s picture

small quibble, but I think it's important.

If we're going to extend a class in another library named ContainerBuilder then our class should be named ContainerBuilder. At first glace I thought our Container was extending Symfony\Component\DependencyInjection\Container.php instead of Symfony\Component\DependencyInjection\ContainerBuilder.php

RobLoach’s picture

FileSize
2.36 KB
effulgentsia’s picture

Status: Needs review » Active

Moved #19 to #1552744-22: Bootstrap for the Dependency Injection Container and make sure SimpleTest abides to it. Resetting this to active to address the stuff that's in the issue summary and early comments.

RobLoach’s picture

Status: Active » Closed (duplicate)
RobLoach’s picture

Issue summary: View changes

gfd