#1497230-142: Use Dependency Injection to handle object definitions added a stop-gap fix for:

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.

This failure can be cleanly reproduced by reverting the commit http://drupalcode.org/project/drupal.git/commit/56ded311cbe2f23f03dce50b...

git revert 56ded311cbe2f23f03dce50bbaea3240fe78eeff

The testbot may not report the failure.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul’s picture

Some additional context, this sounds terrible but its possibly less so because of the next comment by RobLoach:

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

RobLoach’s picture

I think for now, we need to stick a kill-switch in drupal_language_initialize(). The language system should probably not be bootstrapped twice. Not sure why this is DI doing the blowing up though, other tests have passed fine.

RobLoach’s picture

Status: Active » Needs review
FileSize
620 bytes

Maybe this?

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Active

Please note that the OP states that the testbots are currently not able to catch this test failure. The patch referenced in #2 actually did fail (though not on this test case, but another). Check the detailed review log containing the command line output to see the error(s). I already spent some time with @jthorson to debug this, and a hack-ish stop-gap fix is likely deployed very soon.


re #3: Hm. Not really.

So what you can see from the diff context:

- The test actually sets up multiple languages, and during the test, it removes the second, so only one remains.

- In turn, the test (intentionally or not) verifies that the language system is correctly re-initialized after the site transitioned from being multilingual to monolingual.

- Admittedly, the test is a pretty weird mix of operations happening in the parent site (executing the test) and the child site (accessed via internal browser). The final call to drupal_language_initialize() is pretty pointless, since no assertions are following. But all of that is not the point.

- In the end, the test asserts that the language system can be re-initialized later on in a request. As far as I'm able to overlook language system as well as framework/bootstrap developments for D8, this re-initialization feature is required and has to be retained.

effulgentsia’s picture

The problem is in DrupalWebTestCase::tearDown(), we have this:

global $language_interface;
...
$language_interface = $this->originalLanguage;

So, what's happening is that prior to DI, after the test was torn down, the translations needed by _simpletest_format_summary_line() were happening in the original language (English), thereby bypassing locale(). After DI, the tests are still torn down, and this global variable is reset to English, but what's in drupal_container() is still the 'fr' object, so when formatting the summary line, locale() is called, but now on the original tables (not the 'simpletest' prefixed ones, since those have been torn down) where {locale_source} doesn't exist.

Basically, we need to fix the tearDown logic to revert drupal_container() to its original state.

sun’s picture

Easiest and consistent way to do that would be to backup the DI in setUp() like the other backups:

  $this->originalContainer = drupal_container();

and in tearDown(), find (some) way to "re-inject the entire dependency injection container into drupal_container()." ;)

sun’s picture

Status: Active » Needs review
Issue tags: +Testing system, +Dependency Injection (DI)
FileSize
2.95 KB

Alright. Attached patch works for me.

RobLoach’s picture

Status: Needs review » Needs work
+++ b/core/includes/bootstrap.incundefined
@@ -2311,18 +2311,19 @@ function drupal_get_bootstrap_phase() {
  *   The instance of the Drupal Container used to set up and maintain object
- *   instances.
+ *   instances. Returned by reference.
  */
-function drupal_container($reset = FALSE) {
+function &drupal_container($reset = FALSE) {
   // We do not use drupal_static() here because we do not have a mechanism by

Objects in PHP5 are always passed by reference. We probably don't need the explicit & on the return.

sun’s picture

Status: Needs work » Needs review

Objects in PHP5 are always passed by reference.

Yes, they are. But variables are not returned by reference. The calling code replaces the returned variable.

The returned reference only takes effect, when the calling code explicitly assigns the returned variable by reference.

effulgentsia’s picture

We probably don't need the explicit & on the return.

It's needed for this to work:

+++ b/core/modules/simpletest/drupal_web_test_case.php
@@ -1559,6 +1560,10 @@ class DrupalWebTestCase extends DrupalTestCase {
+    // Restore dependency injection container.
+    $container = &drupal_container();
+    $container = $this->originalContainer;

However, would it be better for drupal_container() to either accept $reset as a boolean or as a ContainerBuilder object, or else add a 2nd param that can be type hinted as a ContainerBuilder, instead of this kind of reference magic?

sun’s picture

I had a separate argument first, but that looked very poor from an API perspective. And, of course, -1 to argument overloading à la bool|ContainerBuilder.

As of now, the container only needs to be replaced by SimpleTest. Once the new kernel is in, and once we've converted all code so that $request, $container, and $context are passed forward instead of being accessed through a singleton, we'll no longer have this singleton and thus the need for replacing the container will entirely go away, too.

neclimdul’s picture

I mentioned to Rob on IRC that Symfony's scope might be a novel solution to this. I think he was looking into if it was feasible.

effulgentsia’s picture

I don't think it is. I think we'll want to use scopes for subrequests, and maybe other things, but not for tests, because many services should be able to use the default "container" scope, but tests need to be able to swap out even those.

effulgentsia’s picture

BTW, I'm good with #8, but waiting on feedback from Rob before RTBC'ing.

sun’s picture

This patch might resolve some weird testbot issues we've had over the past days, so it would be good to get this done.

RobLoach’s picture

FileSize
3.21 KB

Maybe something like what effulgentsia was suggesting? We could just have $reset be a Container itself if we want to swap it out. With #1511482: Bootstrap for the Dependency Injection Container, resetting it to a clean Drupal bootstrapped container would be even easier. Again, the docs might need some work, but I lean more towards this than the referencing tricks.

[EDIT] In this patch, does $this->originalContainer = drupal_container(); still need to be $this->originalContainer = clone drupal_container(); ?

Status: Needs review » Needs work

The last submitted patch, 1552744-17.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
3.7 KB

I'd still prefer getting #1511482: Bootstrap for the Dependency Injection Container in first so that we could do the following to get a clean Drupal bootstrap container in prepareEnvironment()...

  drupal_container(new Drupal\Core\DependencyInjection\ContainerBuilder());
sun’s picture

Status: Needs review » Reviewed & tested by the community

1) The clone is still required to ensure the container object is not altered in any way.

2) While this patch backs up the current container in prepareEnvironment(), the container isn't actually reset and replaced with a fresh and clean instance in setUp() before the Drupal installer is invoked. I'm going to leave that detail to #1511482: Bootstrap for the Dependency Injection Container or some other follow-up issue though, since it appears that this patch is sufficient for now to fix the environment confusion in tearDown(). (I do not see a "FATAL" string in the detailed test review log.)

3) Also OK with passing in a new container instead of a Boolean $reset, if that's in line with aforementioned issue.

Thus, this is RTBC for me.

effulgentsia’s picture

+++ b/core/includes/bootstrap.inc
@@ -2336,18 +2337,21 @@ function drupal_get_bootstrap_phase() {
-function drupal_container($reset = FALSE) {
+function drupal_container(ContainerInterface $reset = NULL) {

Eventually, we'll probably want a drupal_container_builder() function that's separate from drupal_container() so that the latter can return a ContainerInterface rather than a ContainerBuilder. But that's part of figuring out compilation. For now, we just have this one function, and therefore, it requires a full ContainerBuilder.

+++ b/core/modules/language/language.test
@@ -178,10 +178,6 @@ class LanguageDependencyInjectionTest extends DrupalWebTestCase {
-
-    // Set up a new container to ensure we are building a new Language object
-    // for each test.
-    drupal_container(TRUE);

We shouldn't remove this here, because this patch's DrupalWebTestCase does not reset to a fresh container. That's a job for #1550866: Remove obsolete drupal_language_initialize().

This patch combines #19 with #1511482-19: Bootstrap for the Dependency Injection Container and fixes the above. That issue should be repurposed for more advanced stuff, like compilation, file-based definitions, event dispatching for extensibility, etc.

effulgentsia’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1552744-21.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
5.03 KB

Stupid typo.

Status: Needs review » Needs work

The last submitted patch, 1552744-24.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
5.03 KB

Same stupid typo. Copy/pasted to a different place. Ok, this should pass.

Anonymous’s picture

Status: Needs review » Needs work

overall, this looks really good, but i think we need more explanation here:

+
+    // Force languages to be initialized.
+    drupal_language_initialize();

reading that, i have NFI why the call is necessary.

sun’s picture

Status: Needs work » Reviewed & tested by the community

That call is what triggered and exposed this critical bug in the first place. See #5. It was removed as a stop-gap fix in the original issue, so testbots don't bump into fatal errors anymore. The test failures can only be reproduced with that call. The point of this issue is to fix the critical bug, not PathMonolingualTestCase.

Also, friends, it's pretty much pointless to aim for a picture perfect solution and beautiful code here. drupal_container() will be massively refactored over the next weeks anyway. And as already mentioned in #12, it might even go away entirely.

Review log doesn't contain a fatal with #26, so RTBC it is.

Anonymous’s picture

meh.

RobLoach’s picture

Title: DI blows up tests » Bootstrap for the Dependency Injection Container and make sure SimpleTest abides to it

Since we're merging the two patches, let's also merge the titles. Meaningful titles are meaningful.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Good fix. Committed.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.