Problem/Motivation

With our current system, it is difficult to define mappings and dependencies between certain systems. We require a system to handle not only the dynamic creation of objects, but also the definitions of what services we require.

Proposed resolution

Dependency Injection is a method to help decouple systems. Symfony provides a Dependency Injection component which we should use to handle our global objects, as well as define the mappings of which classes we want to use.

As a demonstration of how this API is used, this issue moves the $language_interface, $language_content, and $language_url global objects into this system everywhere they're used in core/includes/*, but defers to follow-up issues the cleanup of these global variables from the rest of core and the migration of other globals.

Remaining tasks

Review and get this patch RTBC... Once this patch is in, we can consider the following:

User interface changes

None.

API changes

This brings in a ContainerBuilder singleton which allows us to handle the mappings between service classes and objects. There is a drupal_container() function which returns the desired service object. We will probably get rid of that once the Kernel patch goes in.

Alternative solutions

We considered using Pimple as a simpler alternative to Symfony's container, but decided on Symfony's because it has two important features not used in this initial patch but that we expect to need in followup work:

  1. Its configuration can be serialized (Pimple uses closures that cannot be serialized), which makes it possible to dump configuration to disk/database/whatever and reload easily without a massive rebuild process on each request.
  2. It has support for scoping to force regeneration of various items that depend on other items, which allows for easy context-switching within a request (e.g., for per-block subrequests or other code that wants to context switch). Pimple does not have any such built-in capability.
CommentFileSizeAuthor
#142 drupal8.path-test.142.patch543 bytessun
#136 1497230-136.patch4.87 KBeffulgentsia
#128 translation_test.patch1.29 KBRobLoach
#124 1497230-124.patch1.25 KBeffulgentsia
#113 1497230-113-bootstrap-error-handling.patch561 bytesAnonymous (not verified)
#109 1497230-109-bootstrap-error-handling.patch2.6 KBAnonymous (not verified)
#87 1497230.patch323 KBRobLoach
#83 1497230-83.patch323.02 KBeffulgentsia
#70 1497230-68.patch322.78 KBeffulgentsia
#68 1497230-68.patch322.78 KBeffulgentsia
#68 1497230-interdiff-62-68.patch10.44 KBeffulgentsia
#66 1497230-66.patch322.05 KBeffulgentsia
#66 1497230-interdiff-62-66.txt9.71 KBeffulgentsia
#62 1497230-62.patch319.66 KBpdrake
#60 1497230-60.patch319.63 KBpdrake
#57 1497230-56-justdrupal.patch19.63 KBpdrake
#56 1497230-56.patch318.04 KBpdrake
#50 1497230-48.patch319.82 KBRobLoach
#48 1497230-48.patch319.79 KBRobLoach
#48 1497230-48-justdrupal.patch21.38 KBRobLoach
#38 1497230-37.patch323.07 KBRobLoach
#37 1497230-37.patch323.07 KBRobLoach
#37 1497230-37-justdrupal.patch24.67 KBRobLoach
#35 1497230-35.patch326.39 KBpdrake
#29 1497230-29.patch324.22 KBpdrake
#26 1497230-26.patch321.77 KBpdrake
#22 1497230-20.patch305.88 KBRobLoach
#20 1497230-20.patch305.88 KBRobLoach
#20 1497230-20-justdrupal.patch7.47 KBRobLoach
#18 1497230-18.patch305.91 KBpdrake
#17 1497230-17.patch305.79 KBpdrake
#15 1497230-15.patch305.74 KBpdrake
#15 1497230-15-DrupalOnly.patch5.41 KBpdrake
#12 1497230-JustDrupal.patch4.54 KBRobLoach
#11 1497230.patch302.94 KBRobLoach
#1 dependencyinjection-justdrupal.patch4.93 KBRobLoach
#1 dependencyinjection.patch303.33 KBRobLoach
Just Drupal Stuff.patch4.05 KBRobLoach
DependencyInjection.patch302.46 KBRobLoach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

Had the wrong Container object. This one should be it.

RobLoach’s picture

Issue tags: +D8MI

Adding D8MI since the initial example goal for this would be to replace the global $language_interface with $container-get('language_interface').

Crell’s picture

Status: Needs review » Needs work
+++ b/core/includes/bootstrap.inc
@@ -902,6 +902,23 @@ function variable_del($name) {
+function drupal_container($name) {
+  return Drupal\Core\Container::getInstance()->get($name);
+}

This should be use'd and then referenced with its short name.

I am also not sure I'd really subclass the container. The container should NOT manage its own singleton. It should be managed externally by... this very function, which should then be returning the container instance, not forwarding a method.

+++ b/core/lib/Drupal/Core/Config/Container.php
@@ -0,0 +1,22 @@
+  private static $instance;

Private variables are against coding standards. Use protected. Or, in this case just don't have this class, as we don't need it. :-)

+++ b/core/lib/Drupal/Core/Locale/Language.php
@@ -0,0 +1,34 @@
+  public $name;
+  public $langcode;
+  public $direction;
+  public $enabled;
+  public $weight;
+  public $method_id;
+  public $default = FALSE;

If we're going to turn the language information into a proper class, it should be a proper class with methods rather than public values that could change at any time without warning.

+++ b/core/lib/Drupal/Core/Locale/Language.php
@@ -0,0 +1,34 @@
+  function __construct($name = 'English', $langcode = 'en', $direction = 0, $enabled = 1, $weight = 0, $method_id = '') {

One of the reasons to use proper methods is because supplying both English and en (or whatever for the language in question) is needlessly redundant.

We also need to determine how one registers a service with the container. My thinking at present, at least short-term, is a module-specific class named for the module that contains a setup method that will register whatever is needed on the container. (Since it's not a hook we can use it as soon as the class loader is available.) That's similar to what Symfony does, but it has a cached multiple pass approach for better performance. We'll get there, but one step at a time.

plach’s picture

Issue tags: +sprint

Tagging as top priority D8MI stuff.

Gábor Hojtsy’s picture

Agreed with @Crell on language related points. language_save() takes a language object and fills in the missing pieces much like node_save() does, and it fills in language names based on language code even if language name is not provided. So we don't have a OO interface for this, but it would be a step back to have this constructor approach (again). Also, regarding the namespace placement, we have language.module in Drupal 8 handling all language stuff, and we might rename locale module as well to "interface_translation" or somesuch. So "Locale" as a namespace is not really D8-compliant. Suggested: Language. I can't vouch for the rest of the patch.

RobLoach’s picture

I am also not sure I'd really subclass the container. The container should NOT manage its own singleton. It should be managed externally by... this very function, which should then be returning the container instance, not forwarding a method.

I agree. I'd really opt to have as little public functions and static variables as possible. We want PSR-0 compatibility, and that's why I went with the Singleton approach until the Kernel patch is in. Once the Kernel is in, we'll be able to have the $container ContainerBuilder instance as an instance in the Kernel.

Once the Kernel

Private variables are against coding standards. Use protected. Or, in this case just don't have this class, as we don't need it. :-)

I had these as public variables to keep backwards compatibility and my sanity. I was assuming that once we get the initial dependency injection stuff in, we could clean it all up and use Language properly.

We also need to determine how one registers a service with the container. My thinking at present, at least short-term, is a module-specific class named for the module that contains a setup method that will register whatever is needed on the container.

This is another thing that's blocked by the Kernel patch. Symfony uses a dispatcher to register events to make the ContainerBuilder define and register the objects themselves. So, once the Kernel is in, we can have something like....

$container = $kernel->getContainer();
$container->get('language_instance');

Since the dispatcher would tell the dispatcher about the container, we could have the dispatcher register the services. #1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel would be super duper nice.

So "Locale" as a namespace is not really D8-compliant. Suggested: Language

Drupal\Core\Language sounds good. Thanks for taking a look, Gabor! The ContainerBuilder also has documentation about namespacing the variables in the container with a "." separator, so maybe something like...

$container->get("language.interface");

Again, the name of the service itself doesn't matter, as long as we decide something and stick with it.

Crell’s picture

Based on my conversations with Fabien, I'm not sure I'd make the container an attribute of the kernel, especialy since the kernel will likely live in the container. I also would not make the kernel events the place for other modules to register container objects, as that tightly couples the kernel and container. The container should already be built by the time the kernel does its thing.

As far as naming conventions go, Symfony uses dot-separated IDs. We should probably do the same.

RobLoach’s picture

Should we use a Factory class instead of a static "drupal_container()" function? Using a static means that there could only be one Container instance.

Crell’s picture

No, just one uber-container-instance, which in practice will be the case anyway. There's nothing stopping you from creating your own instance somewhere if you want. And, in practice, it's better practice to pass the container to objects that will need the container as an injection to the object rather than having them call container() anyway. It's a transition hack, even if it's one that survives D8 code freeze. :-)

EclipseGc’s picture

I'm 100% with crell on #9 here.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
302.94 KB

So nice...

    // Register the language types to the Drupal Container, making sure to set
    // them as default languages.
    $container = drupal_container();
    $definition = new Definition('Drupal\\Core\\Language\\Language');
    $definition->setProperty('default', TRUE);
    $container->setDefinition($type, $definition);
RobLoach’s picture

FileSize
4.54 KB

EclipseGC asked for a "Just Drupal" patch for review's-sake.

Status: Needs review » Needs work

The last submitted patch, 1497230-JustDrupal.patch, failed testing.

RobLoach’s picture

+++ b/core/includes/bootstrap.incundefined
@@ -2452,11 +2479,21 @@ function drupal_language_initialize() {
+    $definition = new Definition('Drupal\\Core\\Language\\Language');
+    $definition->setProperty('default', TRUE);
+    $container->setDefinition($type, $definition);

Should be $container->register()->setProperty() instead to save lines of code and the use statement above.

+++ b/core/lib/Drupal/Core/Language/Language.phpundefined
@@ -0,0 +1,27 @@
+  public $direction = 0;
+  public $enabled = 1;
+  public $weight = 0;
+  public $method_id;
+  public $default = FALSE;

For a constructor, we should merge in an $options array. This would let us use setArguments rather than setProperties.

2 days to next Drupal core point release.

pdrake’s picture

Status: Needs work » Needs review
FileSize
5.41 KB
305.74 KB

This version of the patch uses language_default to extend the language class with default values and adds a test to ensure the dependency injected language provider has all of the properties of the $GLOBALS language object. Drupal only patch included for easier review.

Status: Needs review » Needs work

The last submitted patch, 1497230-15-DrupalOnly.patch, failed testing.

pdrake’s picture

Status: Needs work » Needs review
FileSize
305.79 KB

Trying again. Hopefully this one passes tests. Previous patch apparently only passes tests on a multilingual site.

pdrake’s picture

FileSize
305.91 KB

#17 apparently still had the same problem of only passing on a multilingual site. This one should pass on both single language and multilingual sites.

effulgentsia’s picture

I posted just the Symfony component on #1465584-22: Review external library dependencies in core. I have some minor comments on the Drupal code in #18, but I'll wait until we can pass around 5k instead of 300k patches with each other. In general, +1 to the idea of using dependency injection, Symfony's implementation, and starting with the global language objects use-case.

RobLoach’s picture

  1. Moved the bootstrap.test into language.test since it makes more sense there
  2. Cleaned up the service definition registration
  3. Moved the initialize functional code out of the Language object as it should be functional code making use of OOP, and not the other way around
  4. Added more doc comments about how the Container works in context with the language system

Status: Needs review » Needs work

The last submitted patch, drupal-1497230-20.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
305.88 KB

http://drupal.org/files/drupal-1497230-20.patch is the patch without the component.

pdrake’s picture

If we are ultimately replacing the procedural ($_GLOBAL) language handling code with the Language object, is there a reason that procedural code should be setting the default state of the Language object, rather than the default state of the Language object being inherent? I realize the way I did that with initialize() in #18 wasn't pretty (since it depended on external procedural code), but the pattern follows Symfony\Component\HttpFoundation\Session, Symfony\Component\HttpFoundation\Request, etc objects in that a function on the object can be called to build the default state of the object, rather than procedural code explicitly setting the default state of the object.

RobLoach’s picture

Understood :-) . The language initialization is pretty gross to work with, and it's difficult to keep the coupled code coupled and the decoupled code decoupled. Pushing those decisions to the Container rather the Language object itself is probably the way to go.

If we are ultimately replacing the procedural ($_GLOBAL) language handling code with the Language object, is there a reason that procedural code should be setting the default state of the Language object, rather than the default state of the Language object being inherent?

That's a tough call to make. I built the Language class with the English defaults because that's what language_default() does (the only difference is that new Languages come with $language->default == FALSE). What we have to do is tell the Container to register the Language objects with the correct properties, and this is where your handy $language->extend() method comes in :-) .

Do you think there's anything else missing in this patch? It could use a review or two, but I think it's looking pretty good! What are your thoughts?

effulgentsia’s picture

I like this patch, but it doesn't demonstrate any (non test) code using it. Is there any client code out there that we can change from using $_GLOBALS['language_interface'] to drupal_container()->get('language_interface')? Even better, can we change all client code to do that and remove the global?

pdrake’s picture

FileSize
321.77 KB

Attached is a patch that replaces use of the language globals in core/include/*. It seems to work for tests, but drush doesn't like it (because of the way drush "boots" drupal).

Status: Needs review » Needs work

The last submitted patch, 1497230-26.patch, failed testing.

plach’s picture

Understood :-) . The language initialization is pretty gross to work with, and it's difficult to keep the coupled code coupled and the decoupled code decoupled. Pushing those decisions to the Container rather the Language object itself is probably the way to go.

If we are ultimately replacing the procedural ($_GLOBAL) language handling code with the Language object, is there a reason that procedural code should be setting the default state of the Language object, rather than the default state of the Language object being inherent?

Well, actually the initialization of the language globals for multilingual sites involves making use of the full language negotiation subsystem, so it's no surprise that a single method needs to call external code here.
Actually I don't see a true benefit in having the language negotiation code confined in the Language object for two reasons:

  • It's a fairly complex system, yet one that has lots of logic that might be abstracted into a generic chained service initializer. With chained I mean a system that can use different value providers (initialization methods) tied into a fallback chain: once a valid value is found we stop traversing it. The language subsystem needs this logic, so why not abstracting it to make it globally available? I spoke about this with @Crell and it seems there are no real objections about this plan, provided that we are able to properly leverage CMI the to get the fallback chain configuration.
  • Should we need to change the initalization logic we would be stuck with inheritance instead of being able to swap the implementation.

Obviously both of the two bullets imply porting the current initialization system to PSR-0 classes, which I'll be happy to work on ;)

Some remark on the last patch:

+++ b/core/includes/bootstrap.inc
@@ -1352,7 +1353,13 @@ function drupal_unpack($obj, $field = 'data') {
+  // Prior to dependency injection, this would use a global variable, ignoring
+  // the defined constant LANGUAGE_TYPE_INTERFACE and assuming the variable
+  // name.
+  // global $language_interface;
+
+  // Use the new dependency injection container to get the language object.

All these comments look pretty redundant. Also usually we don't keep track in comments about previous solutions, not sure about the real benefit here.

+++ b/core/includes/bootstrap.inc
@@ -2300,6 +2307,31 @@ function drupal_get_bootstrap_phase() {
+ * // Register the language.interface definition.

@Crell confirmed we want to switch to the dot notation. Do we want to postpone this until all the globals are gone or is this just an oversight?

+++ b/core/includes/bootstrap.inc
@@ -2452,11 +2487,26 @@ function drupal_language_initialize() {
   $default = language_default();

I think a @todo about removing this line when getting rid of globals might be useful here, since we have a different behavior wrt multilingual sites, where the language object is used to initialize the Drupal\Core\Language\Language object.

+++ b/core/includes/common.inc
@@ -1607,7 +1607,13 @@ function filter_xss_bad_protocol($string, $decode = TRUE) {
+  // Prior to dependency injection, this would use a global variable, ignoring
+  // the defined constant LANGUAGE_TYPE_CONTENT and assuming the variable
+  // name.
+  // global $language_content;
+
+  // Use the new dependency injection container to get the language object.

Here and below, see the first comment.

+++ b/core/modules/language/language.test
@@ -179,3 +179,60 @@ class LanguageListTest extends DrupalWebTestCase {
+        'description' => 'Compares the default language from $GLOBALS against the dependency injected language provider.',
...
+      $this->assertEqual($expected->$property, $result->$property, t('The dependency injected language provider %prop property equals the $GLOBAL language object %prop property.', array('%prop' => $property)));

Actually we got rid of the "provider" terminology in favor of the "negotiation method" one. However what is actually being injected here is the pure Language object.

+++ b/core/modules/language/language.test
@@ -179,3 +179,60 @@ class LanguageListTest extends DrupalWebTestCase {
+    $result = drupal_container()->get('language_interface');
...
+    $result = drupal_container()->get('language_interface');

We could use the constant here too.

+++ b/core/modules/language/language.test
@@ -179,3 +179,60 @@ class LanguageListTest extends DrupalWebTestCase {
+    foreach($expected as $property => $value) {
...
+    foreach($expected as $property => $value) {

Missing space before the parenthesis.

+++ b/core/modules/simpletest/drupal_web_test_case.php
@@ -1493,6 +1497,10 @@ class DrupalWebTestCase extends DrupalTestCase {
+    // Rebuild languages because of static variable reste

Typo and missing trailing dot.

-2 days to next Drupal core point release.

pdrake’s picture

FileSize
324.22 KB

I presume the language negotiation code will ultimately be performed elsewhere in the process (possibly during routing as part of a Negotiation library - I believe wamilton is working on that) and the Language object will simply inherit its default settings from somewhere. I agree that negotiation does not need to be part of the object.

Thanks for the patch review. I added some fixes for failing tests as well as resolving a number of the issues you pointed out.

The purpose of the somewhat-redundant comments was so that we can see the direction this is headed, comparing what I have done with what existed before and can be removed whenever it is appropriate (probably during the removal of all language globals). The comparison tests that ensure the DI Language object is the same as the GLOBAL language object may be removed at that time as well.

The switch to dot notation (eg language_interface to language.interface) should be trivial once all language globals are eliminated as it can be accomplished by updating the constants.

plach’s picture

Status: Needs work » Needs review
plach’s picture

I presume the language negotiation code will ultimately be performed elsewhere in the process (possibly during routing as part of a Negotiation library - I believe wamilton is working on that) and the Language object will simply inherit its default settings from somewhere. I agree that negotiation does not need to be part of the object.

Interesting :)

The purpose of the somewhat-redundant comments was so that we can see the direction this is headed, comparing what I have done with what existed before and can be removed whenever it is appropriate (probably during the removal of all language globals).

Ok, makes sense.

The switch to dot notation (eg language_interface to language.interface) should be trivial once all language globals are eliminated as it can be accomplished by updating the constants.

Changing the values in constants implies working on the upgrade path, since those values determine the name of the variables use to store the fallback configuration. Probably we should perform the switch while moving the negotiation system to PSR-0, since the Drupal-specific part will need an update function anyway.

+++ b/core/includes/bootstrap.inc
@@ -2300,6 +2307,31 @@ function drupal_get_bootstrap_phase() {
+ * // Register the language.interface definition.
+ * $container = drupal_container();
+ * $container->register('language_interface', 'Drupal\\Core\\Language\\Language');
+ *
+ * // Retrieve the language.interface object.
+ * $language_interface = drupal_container()->get('language_interface');

Minor thing, sorry, but I just realized also the example code above might use constants, unless we assume that we won't always have constants for service ids. In which case the example would be right in general, but misleading for language-specific usages.

-2 days to next Drupal core point release.

Status: Needs review » Needs work

The last submitted patch, 1497230-29.patch, failed testing.

cosmicdreams’s picture

A Dependency Injection container has a far reaching and super awesome impact on many parts of Drupal. The biggest improvement I think DI can help with is the Configuration Initiative, so I'm tagging this issue for that.

With a DI, we can have an active store for configuration and a single place to pull that active configuration during the process of generating exportable configuration files.

A DI also makes it easier to extend Drupal, because it provides a place to put pointers to functions. So if we had a tag for Plugins in Core I'd tag this issue for that too.

In short, this is HUGE.

cosmicdreams’s picture

Also, we need a DI if we're going to replace all the globals.
Only local images are allowed.

pdrake’s picture

FileSize
326.39 KB

Trying again. I believe this resolves the remaining failing tests - guess we'll see...

RobLoach’s picture

Status: Needs work » Needs review
RobLoach’s picture

Status: Needs review » Needs work
FileSize
24.67 KB
323.07 KB
+++ b/core/modules/simpletest/tests/upgrade/upgrade.testundefined
@@ -303,6 +306,10 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
     // Rebuild caches.
     drupal_static_reset();
+
+    // Initialize languages because they are lost during drupal_static_reset().
+    drupal_language_initialize();
+

If we reset the Container, we will loose all our objects that we want to keep persistent in the Container, and have to call all of our initialize functions whenever drupal_static_reset() is called. Because of this, I think using drupal_static() for drupal_container() is the wrong way to go. We should probably use drupal_container($reset) instead, until at least we have a proper place to keep the Container instance.

Once the Kernel patch goes in, the Container instance could become a property of the Kernel object. This means we'd have proper scope of all our Drupal objects depending on which Kernel instance we're using. For now though, it seems like drupal_container($reset) is the way to go.

-29 days to next Drupal core point release.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
323.07 KB

1497230-37-justdrupal.patch is the patch with only Drupal stuff. Here it is with the Dependency Injection component for the test bot.

pdrake’s picture

So, I definitely see how avoiding drupal_static for the dependency injection container will avoid numerous difficulties. I do think that ultimately there will need to be multiple containers, or the container will need to be context-aware. In this case (Language), the object should exist within the context of a request (or subrequest) but not across requests (or subrequests). So, it should live and die with the Request object. Other objects (Database? Session?) may rightfully persist across subrequests if they are handled within a single PHP process. Since the context of the container object is not the same as the context of drupal_static variables, removing the drupal_static is logical.

EclipseGc’s picture

And this is why there's a whole stacking system involved so that new containers can be used and populated across subrequests, giving you the ability to always get back to the request that started the whole thing. I think we're making this too complicated, I think symfony has a methodology for doing this stuff already. Crell is probably the right person to ask at this point though.

Eclipse

Crell’s picture

The Symfony container's "scope" system is for exactly that. I don't know at the moment how it works internally (fabpot, lsmith, or Stof could perhaps enlighten us?), but it is designed to handle exactly what #39 talks about.

RobLoach’s picture

Added another follow up to have objects register themselves: #1511482: Bootstrap for the Dependency Injection Container.

RobLoach’s picture

Issue summary: View changes

Updated

Crell’s picture

Status: Needs review » Reviewed & tested by the community

With the comments in the latest patch indicating the transitional state of the ugly bits, I am comfortable with #37/#38. Since we want that DIC sooner rather than later, I'll be aggressive with it. :-)

andremolnar’s picture

This isn't a blocker for getting DI in, and will ultimately be a follow up issue around the language system in general, but I have reservations with the default Language class having default name and langcode values.
The other defaults are arguably sensible, but make me uncomfortable to a lesser extent.

+class Language {
+  // Properties within the Language are set up as the default language.
+  public $name = 'English';
+  public $langcode = 'en';
+  public $direction = 0;
+  public $enabled = 1;
+  public $weight = 0;
+  public $default = FALSE;
+  public $method_id = NULL;
catch’s picture

Assigned: Unassigned » Dries
Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/bootstrap.incundefined
@@ -1352,7 +1353,13 @@ function drupal_unpack($obj, $field = 'data') {
-  global $language_interface;
+  // Prior to dependency injection, this would use a global variable, ignoring
+  // the defined constant LANGUAGE_TYPE_INTERFACE and assuming the variable
+  // name.
+  // global $language_interface;
+

I don't understand why these comments are everywhere. Once it's in core, it's de-facto not 'new', and we never have comments referring to what used to happen, that's what change notices are for.

+++ b/core/lib/Drupal/Core/Language/Language.phpundefined
@@ -0,0 +1,53 @@
+ * @todo To keep backwards compatibility with stdObject, we currently use
+ * public scopes for the Language class's variables. We will change these to

stdClass?

Instead of or as well as a @todo, can we open that follow-up issue as a major task so it's tracked? Same with the note in the test talking about removing the duplicate test.

+++ b/core/includes/bootstrap.incundefined
@@ -1352,7 +1353,13 @@ function drupal_unpack($obj, $field = 'data') {
-  global $language_interface;
+  // Prior to dependency injection, this would use a global variable, ignoring
+  // the defined constant LANGUAGE_TYPE_INTERFACE and assuming the variable
+  // name.
+  // global $language_interface;
+

I don't see any discussion here as to why we'd use Symfony's DIC instead of Pimple, did that discussion happen during a sprint or irc somewhere? Since Pimple was being considered before, it'd be good to know why Symfony's was chosen. If there's no particular reason, then it'd be good to document that too.

Generally +1 for trying to move towards dependency injection, although I'd be more comfortable with committing this if there were some follow-up issues linked from the issue summary. Also +1 for not worrying about scope/stacking etc. with the initial patch for now, I'd rather see efforts go towards properly injecting dependencies everywhere in the first place, then it's less of an issue anyway.

Also this is a small patch in terms of the Drupal changes, but it's going to touch just about everything in Drupal core by the time the process is done, even if it's not finished by Drupal 10 in the end, so I'm assigning to Dries to take a look.

RobLoach’s picture

Instead of or as well as a @todo, can we open that follow-up issue as a major task so it's tracked? Same with the note in the test talking about removing the duplicate test.

Two of them are up there, the one to remove the remaining $language_interface is: #1510686: Replace remaining references to global $language_interface with drupal_container().

I don't see any discussion here as to why we'd use Symfony's DIC instead of Pimple, did that discussion happen during a sprint or irc somewhere? Since Pimple was being considered before, it'd be good to know why Symfony's was chosen. If there's no particular reason, then it'd be good to document that too.

We talked about Pimple during the WSCCI sprint with Fabien during Drupalcon Denver, and before even considering Symfony's Dependency Injection component, I was under the impression that we'd use Pimple instead. I really do like Pimple's design, but there were some use cases brought up during the sprint where Symfony's Dependency Injection component was what we wanted. Instead of having useful functions like below, it comes down to callable anonymous functions, which might become cumber-sum to use. Because of this, Fabien was advocating we use Symfony instead of Pimple.

// Symfony's Dependency Injection
drupal_container()->register('user', 'Drupal\user\User')
  ->setArguments(array('Dries')); // constructor
$user = drupal_container()->get('user');

// Pimple example
$container = drupal_container();
$container['user_class'] = 'Drupal\user\User';
$container['user'] = $container->share(function ($c) {
    return new $c['user_class']('Dries'); // constructor
});
$user = drupal_container()['user'];

Again, that's a loose example, but you can see why Symfony's Dependency Injection is the preferred method.

Generally +1 for trying to move towards dependency injection, although I'd be more comfortable with committing this if there were some follow-up issues linked from the issue summary.

There's the one I listed above to clean up both the comments and remaining references to $language_interface, and then there's #1511482: Bootstrap for the Dependency Injection Container. I'll make a few more follow ups to clean $language_content and $language_url.

I don't understand why these comments are everywhere. Once it's in core, it's de-facto not 'new', and we never have comments referring to what used to happen, that's what change notices are for.

So we should remove those comments?

EclipseGc’s picture

Symfony's dependency injection container is a bit more capable than pimple, plus I don't think we were ever seriously considering pimple, we just hadn't had time to dig into Symfony's yet and it was easy to get Pimple running to play with and wave our hands and say "it'll be sorta like this". I think overall it makes better sense to utilize Symfony's though given the number of other Symfony components we're embracing, and there's no technical reason not to. I'm pretty sure that's the entirety of the logic on that decision :-D

EclipseGc’s picture

Issue summary: View changes

Updated issue summary.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
21.38 KB
319.79 KB

Status: Needs review » Needs work

The last submitted patch, 1497230-48-justdrupal.patch, failed testing.

RobLoach’s picture

Issue summary: View changes

Updated issue summary.

RobLoach’s picture

FileSize
319.82 KB

Setting this back to RTBC as catch's notes have been addressed. This patch also updates the @todo comment with a link directly to #1512424: Add a LanguageInterface, and property setters/getters to Language class.

Crell’s picture

Status: Needs work » Reviewed & tested by the community

Pimple's use of closures actually makes it a bit easier to work with than Symfony's, and it's a vastly more light-weight implementation. However, the Symfony container has two rather important benefits over Pimple:

1) Its configuration can be serialized (closures cannot be serialized), which makes it possible to dump configuration to disk/database/whatever and reload easily without a massive rebuild process on each request.

2) It has support for scoping to force regeneration of various items that depend on other items. Pimple does not have any such concept. (This serves the same purpose as the "stacking" logic from the Context API patch.)

While we're not using either of those capabilities in this patch, we know that we will want to sooner or later.

neclimdul’s picture

Actually there's a 3rd benefit that is the underpinnings of both of Crell's points and that's lazy instantiation. Because you can register class names and constructor arguments(and other startup stuff) Symfony's DI Container waits until its requested to instantiated the injected element which means if somehow you never use the language system, it would never be loaded. This is not true of pimple because you instantiate and register full objects.

Crell’s picture

neclimdul: Eh, that's completely untrue. The whole point of the closures in Pimple is to NOT instantiate anything until you need to. :-) Pimple is just as capable of lazy-instantiation as the Symfony component is.

Either way, let's stop talking about it and commit this thing. :-)

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/bootstrap.inc
@@ -1352,7 +1353,8 @@ function drupal_unpack($obj, $field = 'data') {
-  global $language_interface;
+  // Use the dependency injection container to get the language object.
+  $language_interface = drupal_container()->get(LANGUAGE_TYPE_INTERFACE);

Here and elsewhere, I don't think we need this code comment. The line of code should be clear on its own. If it's not, we should rename the function, but for now, I think we should just remove the comment, and defer function name bikesheds to a follow-up.

+++ b/core/includes/bootstrap.inc
@@ -2304,6 +2306,36 @@ function drupal_get_bootstrap_phase() {
+ * // Register the language.interface definition.
+ * $container = drupal_container();
+ * $container->register('language_interface', 'Drupal\\Core\\Language\\Language');

The comment and line of code disagree on "." vs. "_", and neither matches the actual code in the patch (which uses a constant).

+++ b/core/includes/bootstrap.inc
@@ -2304,6 +2306,36 @@ function drupal_get_bootstrap_phase() {
+  // We do not use drupal_static() here because we want objects stored in the
+  // Container to be persistent across drupal_static_reset() calls.
+  static $container = NULL;

This doesn't explain why we want this. How about "We do not use drupal_static() here because we do not have a mechanism by which to reinitialize the stored objects, so a drupal_static_reset() call would leave Drupal in a nonfunctional state."

+++ b/core/includes/bootstrap.inc
@@ -2445,22 +2477,44 @@ function get_t() {
+    $lang = array('default' => TRUE);
+    $container->register($type, 'Drupal\\Core\\Language\\Language')
+      ->setArguments(array($lang));

Here and the same code in update.php:

1. We usually don't use abbreviations like $lang in Drupal, and in this case, I don't think it describes what this variable actually is. If we leave Language::_construct() alone, this variable should be $options.

2) If variable_get('language_default') returns something other than the Language class's default public property values, then this results in a mismatch between $container->get($type) and language_default(). So, perhaps this should be replaced with "->addMethodCall('extend', array($default));"?

+++ b/core/modules/language/language.test
@@ -179,3 +179,59 @@ class LanguageListTest extends DrupalWebTestCase {
+    // The language system creates a language with the default property set to
+    // TRUE. This will test to make sure the Container constructs the object
+    // correctly.
+    $expected = new Drupal\Core\Language\Language(array(
+      'default' => TRUE,
+    ));

Based on my comment for the previous code block, we should test more here: one test after a variable_del('language_default') where $expected contains all the default property values in the Language class, and one after a variable_set('language_default') to something other than English and making $expected equal to that.

effulgentsia’s picture

Issue summary: View changes

Updated issue summary.

effulgentsia’s picture

Issue summary: View changes

Updated issue summary.

effulgentsia’s picture

Assigned: Dries » Unassigned

I updated the "Proposed resolution" part of the summary with the info from #51. Unassigning Dries until #54 is addressed.

pdrake’s picture

FileSize
318.04 KB

I've done my best in this version of the patch to address the additional issues raised by effulgentsia. This version significantly changes the tests, but does not introduce a single test in which the variable language_default is modified and the Language object is re-tested. The reason for this is that the Language object is implicitly scoped to the Container. Adding such a test and expecting the Language object to respect the new defaults would require implementing Scope(s) within this patch. If that is in-scope for this issue, it can certainly be done.

pdrake’s picture

FileSize
19.63 KB

For easier review, this is the above patch with just the Drupal changes (Symfony classes excluded).

pdrake’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1497230-56-justdrupal.patch, failed testing.

pdrake’s picture

Status: Needs work » Needs review
FileSize
319.63 KB

I sure messed that patch up. Trying again.

Status: Needs review » Needs work

The last submitted patch, 1497230-60.patch, failed testing.

pdrake’s picture

Status: Needs work » Needs review
FileSize
319.66 KB

Sorry for spamming everyone on this issue. Trying again.

Status: Needs review » Needs work
Issue tags: -D8MI, -sprint

The last submitted patch, 1497230-62.patch, failed testing.

pdrake’s picture

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

#62: 1497230-62.patch queued for re-testing.

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Overall, I believe this is good to go. With the different default language test, cleaned up code comments, and a defined path of what follows, we're now fully accommodating catch's and effulgensia's notes. Back in the RTBC queue! Took some notes when reviewing:

+++ b/core/includes/bootstrap.incundefined
@@ -2304,6 +2305,38 @@ function drupal_get_bootstrap_phase() {
+ * // Register the LANGUAGE_TYPE_INTERFACE definition.  Registered definitions
+ * // do not necessarily need to be named by a constant.
+ * $container = drupal_container();
+ * $container->register(LANGUAGE_TYPE_INTERFACE, 'Drupal\\Core\\Language\\Language');
+ *
+ * // Retrieve the LANGUAGE_TYPE_INTERFACE object.
+ * $language_interface = drupal_container()->get(LANGUAGE_TYPE_INTERFACE);

Looks a bit weird with the constant, but that'll be fixed with #1510686: Replace remaining references to global $language_interface with drupal_container().

+++ b/core/includes/bootstrap.incundefined
@@ -2304,6 +2305,38 @@ function drupal_get_bootstrap_phase() {
+  // We do not use drupal_static() here because we do not have a mechanism by
+  // which to reinitialize the stored objects, so a drupal_static_reset() call
+  // would leave Drupal in a nonfunctional state.

This comment does make more sense than what we had before. Thanks for the note, Alex.

+++ b/core/modules/language/language.testundefined
@@ -179,3 +179,90 @@ class LanguageListTest extends DrupalWebTestCase {
+    // Change the language default object to different values.
+    $new_language_default = (object) array(
+      'langcode' => 'fr',
+      'name' => 'French',
+      'direction' => 0,

Nice job on putting the different default language test together.

-1 days to next Drupal core point release.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
9.71 KB
322.05 KB

Some more cleanup.

Status: Needs review » Needs work

The last submitted patch, 1497230-66.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
10.44 KB
322.78 KB

Status: Needs review » Needs work

The last submitted patch, 1497230-interdiff-62-68.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
322.78 KB

D'oh. I uploaded the #68 interdiff with the wrong extension which testbot didn't like. Here's #68 again to keep bot happy.

RobLoach’s picture

One note, but other than that, I think it's good. What are your thoughts?

+++ b/core/includes/common.incundefined
@@ -2519,9 +2515,7 @@ function drupal_deliver_html_page($page_callback_result) {
-  // Send appropriate HTTP-Header for browsers and search engines.
-  global $language_interface;
-  drupal_add_http_header('Content-Language', $language_interface->langcode);
+  drupal_add_http_header('Content-Language', drupal_container()->get(LANGUAGE_TYPE_INTERFACE)->langcode);

Not sure how I feel about using the short-hands when they cross the 80 character width mark, but I think I can cope.

-1 days to next Drupal core point release.

effulgentsia’s picture

I'm happy to reroll if people want shorter lines, but my opinion is that as much as possible, 1 line of code should express 1 unit of thought. Setting a variable in 1 line that is only used by the immediately following line and nowhere else in the function is taking 2 lines to express 1 unit of thought. I also assume that most people develop on relatively large screen sizes, so 80 chars is not a magic limit. It is for documentation, but not for code, IMO, unless we have coding standards that say otherwise.

In most places where I shortened what was in #62, it was for performance (don't get the object until it's needed) rather than reducing line count, so if desired, we can re-add lines as long as we do so in the innermost possible code blocks.

yched’s picture

Errs on the side of nitpicking, but one thing I actually like about globals is the convention of scoping them at the beginning of a function. Makes dependencies clearer, close to the function signature.
I suggest maybe we adopt the same convention with stuff from the DIC, assign them to vars at the beginning of a function for the rest of the code to use ?

effulgentsia’s picture

one thing I actually like about globals is the convention of scoping them at the beginning of a function

Drupal HEAD doesn't follow this convention very consistently. Grepping the codebase for "global $" results in 325 matches in 99 files. Grepping for "$GLOBALS" results in 137 matches in 45 files. So if our intent is to be tracking dependencies at the beginning of functions, our current success rate is only 70%.

I suggest maybe we adopt the same convention with stuff from the DIC, assign them to vars at the beginning of a function

That's not so good for performance in functions like t(), l(), and many others where micro-optimizations matter and where the service object might only be needed within some code blocks. If we want a good way of tracking service dependencies, how about a PHP docblock directive?

Crell’s picture

The use of the drupal_container() wrapper itself I understand to be a temporary measure. Temporary for the entire Drupal 8 lifecycle, maybe, but still temporary. :-) Any objects that are spawned by the container that have dependencies should have those provided by the container to their constructors, by design. So I wouldn't worry about optimizing the function approach too much. We should handle that case by case. Having drupal_container()->get('whatever') randomly in the middle of a function is no worse than having whatever() in the middle of function, which is what we do now just about everywhere.

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Short-hand or not, drupal_container() or not, we should probably just get this in and clean up any follow up stuff in one of the many remaining tasks. RTBC again.

effulgentsia’s picture

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

Thanks for bearing with me guys. Ok, back to Dries. I'm now feeling pretty confident he'll accept this.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community
cosmicdreams’s picture

cosmicdreams’s picture

Issue summary: View changes

Updated issue summary.

RobLoach’s picture

Issue tags: -D8MI, -sprint

#70: 1497230-68.patch queued for re-testing.

RobLoach’s picture

Issue summary: View changes

add follow up issue so we can keep track of the reorganization of code as per the comments of http://drupal.org/node/1497230#comment-5827086

RobLoach’s picture

Issue summary: View changes

a

Tor Arne Thune’s picture

#70: 1497230-68.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +D8MI, +sprint

The last submitted patch, 1497230-68.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
323.02 KB

Just a reroll of #68 for moving of files common.test and locale.inc. No substantive changes here. "needs review" for bot, but if it passes, should go back to RTBC.

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

per 83

tstoeckler’s picture

Two minor cosmetic issues. Because this issue blocks a lot of progress in other areas, I am leaving this at RTBC. I will post a follow-up patch once this is committed.

+++ b/core/includes/bootstrap.inc
@@ -2445,27 +2479,44 @@ function get_t() {
   if (language_multilingual()) {
...
+      $language = language_types_initialize($type);
...
+  else {
+    $default = language_default();

This code could be simplified if we initialize $language = language_default(); up-front and then override $language if language_multilingual() is TRUE. Then we would not have to duplicate the $container->register() block.

+++ b/core/modules/language/language.negotiation.inc
@@ -274,7 +273,8 @@ function language_url_fallback($language = NULL, $language_type = LANGUAG
-    return $GLOBALS[$language_type]->langcode;
+    $language = drupal_container()->get($language_type);
+    return $language->langcode;

Is there a reason this is split into multiple lines?

RobLoach’s picture

This code could be simplified if we initialize $language = language_default(); up-front and then override $language if language_multilingual() is TRUE. Then we would not have to duplicate the $container->register() block.

I agree, but we were trying to keep the language system exactly the same as it is now, and just stick the Container in as a direct swap in replacement for $GLOBALS. We could clean up the language system initialization itself later on in #1510686: Replace remaining references to global $language_interface with drupal_container().

Is there a reason this is split into multiple lines?

It's hard to find the line where the short-hand should be used vs using a simple $language definition. Do you know if we have any documentation on that? In general, I lean towards the long-hand, but I'm okay with using the short-hand. I could re-roll this one tonight if we want to short-hand this one.

RobLoach’s picture

FileSize
323 KB

Here's a re-roll with the short-hand that tstoeckler mentioned.

RobLoach’s picture

Priority: Normal » Major

Pushing the priority to major as we have some stuff in the plugin system that could benefit from having the container.

effulgentsia’s picture

For the past month, I've been a major proponent of using the dependency injection container for some of the plugin system work, but in a conversation yesterday with neclimdul, EclipseGc, and merlinofchaos, we found a way to make progress without it. We may yet need it before we're done, but at least for now, it's not a blocker. Regardless, I agree that this is major priority.

EclipseGc’s picture

There are various other improvements waiting in the wings that could use this, and could also use as much time as possible to play with it. The sooner this goes in, the better.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

I reviewed this patch twice now, had a chance to think about it more, and decided it looks good. I just committed it to 8.x.

We should proceed with it for now; if we want to switch to a simpler dependency injection system we can later. It is abstracted out enough; it shouldn't be hard if that is what we decide to do. For now, it is nice to have the extra functionality available.

Thanks all!

RobLoach’s picture

Status: Fixed » Reviewed & tested by the community

Whoop! Doesn't look like this was pushed :-)
http://drupal.org/node/3060/commits

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -sprint
Gábor Hojtsy’s picture

Title: Use Dependency Injection to handle object definitions » Change notice for: Use Dependency Injection to handle object definitions
Assigned: Dries » Unassigned
Priority: Major » Critical
Status: Fixed » Active

Still needs a change notice :) Reopened for that. Rob do you want to take it away? :)

RobLoach’s picture

Status: Active » Needs review

Hmmm, probably needs some work and some more examples. What are your thoughts? http://drupal.org/node/1539454

Gábor Hojtsy’s picture

Title: Change notice for: Use Dependency Injection to handle object definitions » Use Dependency Injection to handle object definitions
Priority: Critical » Major
Status: Needs review » Fixed

Looks good IMHO. Added a little more notes there.

webchick’s picture

Priority: Major » Normal
Status: Fixed » Needs work
Issue tags: +Needs documentation

This went in without any documentation of those properties. Tsk, tsk. :)

Gábor Hojtsy’s picture

Based on chat with @webchick, she meant the Language class properties.

webchick’s picture

To clarify:

  // Properties within the Language are set up as the default language.
  public $name = 'English';
  public $langcode = 'en';
  public $direction = 0;
  public $enabled = 1;
  public $weight = 0;
  public $default = FALSE;
  public $method_id = NULL;

...is a no-no. We need PHPDoc above each one of those that explains what it does. http://api.drupal.org/api/drupal/globals/8 hopefully has some copy/pasteable language.

RobLoach’s picture

Status: Needs work » Fixed
webchick’s picture

Hm. I'm not really comfortable with side-stepping gates on core patches. There's no reason documentation couldn't be added here and then adjusted as necessary over at the other issue.

RobLoach’s picture

Should we merge that issue into this one then? In the follow up issue, those properties will become protected and be replaced with getName(), getDirection(), etc with actual proper documentation. The major point of this issue was drupal_container(), not Language, to avoid bikesheds.

RobLoach’s picture

Title: Use Dependency Injection to handle object definitions » [META] Use Dependency Injection to handle object definitions
Status: Fixed » Active

Actually, let's turn this in a META parent.

ebeyrent’s picture

I agree with webchick in #100 - at the very least, there should be documentation for each var. To do this properly though, there should be getters and setters for each of those data members.

Anonymous’s picture

/**
 * Root directory of Drupal installation.
 */
define('DRUPAL_ROOT', getcwd());
require_once DRUPAL_ROOT . '/core/includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_DATABASE);

this broke scripts that look like that. not sure how this fits, like whether we care at all, but yeah. seems ungood to me. digging around now trying to figure out why.

Anonymous’s picture

ok, so this seems to have broken error handling early in bootstrap. not setting $_SERVER['REMOTE_ADDR'] causes a notice in this script:


/**
 * Root directory of Drupal installation.
 */
define('DRUPAL_ROOT', getcwd());

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

try {
  drupal_bootstrap(DRUPAL_BOOTSTRAP_DATABASE);
}
catch (Exception $e) {
  print $e->getMessage() . "\n";
  print print_r(array_reverse($e->getTrace()), TRUE);
}

and here's the output:

The service definition "language_interface" does not exist.
Array
(
    [0] => Array
        (
            [file] => /var/www/drupal8/webroot/snippet.script.php
            [line] => 11
            [function] => drupal_bootstrap
            [args] => Array
                (
                    [0] => 2
                )

        )

    [1] => Array
        (
            [file] => /var/www/drupal8/webroot/core/includes/bootstrap.inc
            [line] => 2027
            [function] => _drupal_bootstrap_page_cache
            [args] => Array
                (
                )

        )

    [2] => Array
        (
            [file] => /var/www/drupal8/webroot/core/includes/bootstrap.inc
            [line] => 2179
            [function] => ip_address
            [args] => Array
                (
                )

        )

    [3] => Array
        (
            [file] => /var/www/drupal8/webroot/core/includes/bootstrap.inc
            [line] => 2774
            [function] => _drupal_error_handler
            [args] => Array
                (
                    [0] => 8
                    [1] => Undefined index: REMOTE_ADDR
                    [2] => /var/www/drupal8/webroot/core/includes/bootstrap.inc
                    [3] => 2774
                    [4] => Array
                        (
                            [ip_address] => 
                        )

                )

        )

    [4] => Array
        (
            [file] => /var/www/drupal8/webroot/core/includes/bootstrap.inc
            [line] => 2093
            [function] => _drupal_error_handler_real
            [args] => Array
                (
                    [0] => 8
                    [1] => Undefined index: REMOTE_ADDR
                    [2] => /var/www/drupal8/webroot/core/includes/bootstrap.inc
                    [3] => 2774
                    [4] => Array
                        (
                            [ip_address] => 
                        )

                )

        )

    [5] => Array
        (
            [file] => /var/www/drupal8/webroot/core/includes/errors.inc
            [line] => 88
            [function] => _drupal_log_error
            [args] => Array
                (
                    [0] => Array
                        (
                            [%type] => Notice
                            [!message] => Undefined index: REMOTE_ADDR
                            [%function] => ip_address()
                            [%file] => /var/www/drupal8/webroot/core/includes/bootstrap.inc
                            [%line] => 2774
                            [severity_level] => 5
                        )

                    [1] => 
                )

        )

    [6] => Array
        (
            [file] => /var/www/drupal8/webroot/core/includes/errors.inc
            [line] => 250
            [function] => t
            [args] => Array
                (
                    [0] => %type: !message in %function (line %line of %file).
                    [1] => Array
                        (
                            [%type] => Notice
                            [!message] => Undefined index: REMOTE_ADDR
                            [%function] => ip_address()
                            [%file] => /var/www/drupal8/webroot/core/includes/bootstrap.inc
                            [%line] => 2774
                            [severity_level] => 5
                        )

                )

        )

    [7] => Array
        (
            [file] => /var/www/drupal8/webroot/core/includes/bootstrap.inc
            [line] => 1361
            [function] => get
            [class] => Symfony\Component\DependencyInjection\ContainerBuilder
            [type] => ->
            [args] => Array
                (
                    [0] => language_interface
                )

        )

    [8] => Array
        (
            [file] => /var/www/drupal8/webroot/core/vendor/Symfony/Component/DependencyInjection/ContainerBuilder.php
            [line] => 338
            [function] => getDefinition
            [class] => Symfony\Component\DependencyInjection\ContainerBuilder
            [type] => ->
            [args] => Array
                (
                    [0] => language_interface
                )

        )

)
Anonymous’s picture

Priority: Normal » Major
Status: Active » Needs work

talked to catch in IRC, he suggested this be a major because it's a regression.

Anonymous’s picture

Category: task » bug
Status: Needs work » Needs review
FileSize
2.6 KB

here's a fairly hacky test-only patch that shows the regression.

Status: Needs review » Needs work

The last submitted patch, 1497230-109-bootstrap-error-handling.patch, failed testing.

Crell’s picture

Do we actually support such scripts? Really? And if so, are we better off just punting on it since there are already active patches in progress that will totally change the way you'd write such scripts to begin with?

cosmicdreams’s picture

Two questions here

1) Even though drupal_container()->get(LANGUAGE_TYPE_INTERFACE) is the only service that seems to be registered (in update.php) other services such as: drupal_container()->get(LANGUAGE_TYPE_CONTENT) and drupal_container()->get(LANGUAGE_TYPE_URL) are called. Should those be added to this patch?

2) Where in the code should they be registered?

Anonymous’s picture

Status: Needs work » Needs review
FileSize
561 bytes

re #111: this is not about the snippet script anymore, i just started looking when stuff that used to work blew up in my face.

re #112: two excellent questions.

so the general issue is that if you call drupal_container() looking for something that's not yet registered, we blow up. so any system, like, say, translating strings, that relies on stuff in the container, can blow up when called early during bootstrap.

did t() used to work early in the bootstrap?

as it is now t() doesn't work until LANGUAGE_TYPE_INTERFACE is registered with our DIC, which doesn't happen until late in the bootstrap cycle. so we can either move the registration earlier, or say that you have to be careful calling t() early in the bootstrap, or ...

new patch just adds a call to t() in hook_boot() in the translation_test module, which blows up.

Status: Needs review » Needs work

The last submitted patch, 1497230-113-bootstrap-error-handling.patch, failed testing.

moshe weitzman’s picture

We worked very hard during D7 to make t() safe to call anytime in Drupal (installer excepted). This is a regression.

Anonymous’s picture

thanks moshe.

so, one option is to make t() aware that the DIC may blow up in it's face and add some code like this:

  // Merge in default.
  if (empty($options['langcode'])) {
    try {
      $options['langcode'] = drupal_container()->get(LANGUAGE_TYPE_INTERFACE)->langcode;
    }
    catch (InvalidArgumentException $e) {
      // If t() is called before the language system is initialized, the
      // dependency injection container may throw this execption. In that case,
      // just use the default for $langcode.
      $options['langcode'] = language_default()->langcode;
    }
  }

seems really ugly, but works.

it seems unsafe to move drupal_language_initialize() before _drupal_bootstrap_database(), because the list of enabled languages is fetched from db.

lotyrin’s picture

This same thing blows up drush:

# drush status
PHP Fatal error:  Class 'Symfony\Component\DependencyInjection\ContainerBuilder' not found in /home/lotyrin/drupal/core/includes/bootstrap.inc on line 2335
PHP Stack trace:
PHP   1. {main}() /home/lotyrin/drush/drush.php:0
PHP   2. drush_main() /home/lotyrin/drush/drush.php:14
PHP   3. _drush_bootstrap_and_dispatch() /home/lotyrin/drush/drush.php:59
PHP   4. drush_bootstrap_to_phase() /home/lotyrin/drush/drush.php:79
PHP   5. drush_bootstrap_max() /home/lotyrin/drush/includes/bootstrap.inc:291
PHP   6. drush_bootstrap() /home/lotyrin/drush/includes/bootstrap.inc:345
PHP   7. _drush_bootstrap_drupal_root() /home/lotyrin/drush/includes/bootstrap.inc:185
PHP   8. dt() /home/lotyrin/drush/includes/bootstrap.inc:696
PHP   9. t() /home/lotyrin/drush/includes/output.inc:139
PHP  10. drupal_container() /home/lotyrin/drupal/core/includes/bootstrap.inc:1361
PHP Fatal error:  Class 'Symfony\Component\DependencyInjection\ContainerBuilder' not found in /home/lotyrin/drupal/core/includes/bootstrap.inc on line 2335
PHP Stack trace:
PHP   1. {main}() /home/lotyrin/drush/drush.php:0
PHP   2. drush_main() /home/lotyrin/drush/drush.php:14
PHP   3. _drush_bootstrap_and_dispatch() /home/lotyrin/drush/drush.php:59
PHP   4. drush_bootstrap_to_phase() /home/lotyrin/drush/drush.php:79
PHP   5. drush_bootstrap_max() /home/lotyrin/drush/includes/bootstrap.inc:291
PHP   6. drush_bootstrap() /home/lotyrin/drush/includes/bootstrap.inc:345
PHP   7. _drush_bootstrap_drupal_root() /home/lotyrin/drush/includes/bootstrap.inc:185
PHP   8. dt() /home/lotyrin/drush/includes/bootstrap.inc:696
PHP   9. t() /home/lotyrin/drush/includes/output.inc:139
PHP  10. drupal_container() /home/lotyrin/drupal/core/includes/bootstrap.inc:1361
PHP  11. drush_shutdown() /home/lotyrin/drush/includes/bootstrap.inc:0
PHP  12. dt() /home/lotyrin/drush/includes/bootstrap.inc:1060
PHP  13. t() /home/lotyrin/drush/includes/output.inc:139
PHP  14. drupal_container() /home/lotyrin/drupal/core/includes/bootstrap.inc:1361
lotyrin’s picture

The try-catch from #116 doesn't seem to resolve the issue for drush.

cosmicdreams’s picture

It makes more sense to me to have the DI service registrations be performed earlier in the bootstrap. As early as possible. But I don't know where that would be.

plach’s picture

With respect specifically to language the plans are to migrate the list of enabled languages to CMI. The negotiation process might need to acess the DB, depending on the configured negotiation methods, but assuming that usages of the language service will happen only after the DB is initialized might make sense. Hence we could rely on lazy-initialization to cope with this.

Hence on the language negotiation side we could try to move everything up in the bootstrap process, right after CMI/variables are available. Obviously this is a mid-term solution, not a quick fix.

Anonymous’s picture

CMI in core requires the db to work. active store === the database.

this is in flux, but just want to make clear that as of now, 'move to CMI' does not break the dependency on a loaded db.

plach’s picture

Mmmh, you are right, forget #120 then.

RobLoach’s picture

The easy fix is to ->register() the default languages early. We could take it further later on and have the Container load its initial bootstrap configuration from a file:

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'll need some early default bootstrap stuff to take advanced of the container during the bootstrap, having it load from a file might make it easier to handle later on.

effulgentsia’s picture

FileSize
1.25 KB

I suspect #123 will be correct in the long run (not necessarily XML based), along with compiling. But there's a lot we need to figure out to make that happen. In the meantime, how's this?

effulgentsia’s picture

Status: Needs work » Needs review
effulgentsia’s picture

Re #116, you can also call drupal_container()->has(), or pass ContainerInterface::NULL_ON_INVALID_REFERENCE as a 2nd param to get(). But I think setting up the container with necessary services (e.g., #124) is better than making functions have to worry about lack of service availability.

RobLoach’s picture

Status: Needs review » Needs work
+++ b/core/includes/bootstrap.incundefined
@@ -2333,6 +2333,11 @@ function drupal_container($reset = FALSE) {
+    // 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')
+      ->addMethodCall('extend', array(language_default()));

I'm not sure about calling language_default() directly as we don't want it to call variable_get() so early. The default new Language() parameters might actually be okay to live with, until the Language system is initialized. If multilingualization is on, then calling ->register() again will just swap out the definition. So, maybe just:

    // 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');

-18 days to next Drupal core point release.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
1.29 KB

Put in some inline comments in the test about why we're doing it.

lotyrin’s picture

Status: Needs review » Reviewed & tested by the community

This looks good and solves the boot t() case, as well as the minimal script and drush. Even includes a test.

lotyrin’s picture

I was mistaken, drush is not resolved by #128.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

:(

clemens.tolboom’s picture

Drush is expecting the t() function to be valid too early in the bootsstrap.

In drush #1540110: Drush calling t() before D8 dependency injection construct is available I patched drush to make sure the classloader has at least some namespaces registered.

drush test for the existence only of the t-function but t() now depends on the classloader being properly initialized.

(my 2 cents)

cosmicdreams’s picture

In that case #128 sounds like a proper follow up patch to this issue. Since the tests pass this issues sounds like it is RTBC

clemens.tolboom’s picture

Should drush use get_t() instead? Or one could argue the t-function should not be available yet: that is move it to another file?

Or we need to adjust the bootstrap having the class loader configured properly?

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community

Yes, so back to RTBC from #129. It doesn't 100% solve drush's problem, but it is needed.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.87 KB

What do you all think of this? This ensures that as long as drupal_classloader() is called, it has core/vendor/Symfony and core/lib/Drupal registered. This still doesn't fully solve the Drush issue, because Drush is calling t() prior to bootstrapping to DRUPAL_BOOTSTRAP_CONFIGURATION, but this would allow Drush to simply call drupal_classloader() early in its process and then have t() work. I considered making drupal_container() call drupal_classloader() so that Drush wouldn't even have to do that, but that would be wrong, because drupal_classloader() relies on variable_get(), so Drupal should not call it prior to DRUPAL_BOOTSTRAP_CONFIGURATION: that really does need to be Drush's choice to do that.

sun’s picture

Title: [META] Use Dependency Injection to handle object definitions » Use Dependency Injection to handle object definitions
Status: Needs review » Reviewed & tested by the community

I like that patch. Simplifies things.

Lastly, let's please create a new issue for whatever-meta discussion/container is required. This issue is/was about a concrete functional change and the comments in here are anything but meta. ;) Thanks. Removing the prefix from the issue title.

RobLoach’s picture

Lastly, let's please create a new issue for whatever-meta discussion/container is required.

Great idea! Let's hit that up in #1511482: Bootstrap for the Dependency Injection Container.

sun’s picture

Component: wscci » base system

All DI follow-up issues mentioned in this issue are now tagged accordingly:
http://drupal.org/project/issues/search/drupal?issue_tags=Dependency%20I...

If there's a need for a META issue, please create one.

sun’s picture

d'oh, sorry for the noise.

catch’s picture

Status: Reviewed & tested by the community » Fixed

OK it looks like the drush issue is in progress, and the follow-up here looks fine. Committed/pushed to 8.x and moving this back to fixed.

sun’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
543 bytes

The original commit in #87 caused a fatal regression:

drupal_language_initialize() cannot be invoked more than once anymore.

This causes a fatal error on test(bot) clients, which is not caught due to very unfortunate testing system circumstances.

Running PathMonolingualTestCase locally exposes the error - the test "passes", but Simpletest (UI) ends with a PDOException.

Attached patch fixes the trigger, but not the cause. I'm fine with committing this stop-gap fix to unblock other patches, but we need to resolve the actual cause.

RobLoach’s picture

Confirmed... And with #1550866: Remove obsolete drupal_language_initialize(), drupal_language_initialize() will likely just go away.

jthorson’s picture

Note to committers: Please let me know when this is in.

I've got a testbot patch which should detect something like and prevent it from sneaking through in the future, but don't want to apply it until the PDO Exception is gone from HEAD tests ... as adding it now would cause HEAD to fail and freeze all other D8 patch testing.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

catch is AFK all weekend, and presumably this qualifies as the kind of 8.x patch I can commit, since it's fixing totally broken stuff.

So, committed and pushed to 8.x. Thanks! :)

sun’s picture

cosmicdreams’s picture

Don't see this commit with git log. Is this someone I just need to wait and see later or is the fact I'm not seeing it mean that it wasn't really committed to Drupal 8 yet?

tim.plunkett’s picture

cosmicdreams’s picture

Ah thanks Tim I see it now.

Status: Fixed » Closed (fixed)

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

sun’s picture

Issue tags: +Configuration system
sun’s picture

Issue summary: View changes

fdsfd