#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.
Comment | File | Size | Author |
---|---|---|---|
#26 | 1552744-26.patch | 5.03 KB | effulgentsia |
#24 | 1552744-24.patch | 5.03 KB | effulgentsia |
#22 | 1552744-21.patch | 5.04 KB | effulgentsia |
#19 | 1552744-19.patch | 3.7 KB | RobLoach |
#17 | 1552744-17.patch | 3.21 KB | RobLoach |
Comments
Comment #1
neclimdulSome additional context, this sounds terrible but its possibly less so because of the next comment by RobLoach:
Comment #2
RobLoachI 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.
Comment #3
RobLoachMaybe this?
Comment #5
sunPlease 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.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedThe problem is in DrupalWebTestCase::tearDown(), we have this:
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.
Comment #7
sunEasiest and consistent way to do that would be to backup the DI in setUp() like the other backups:
and in tearDown(), find (some) way to "re-inject the entire dependency injection container into drupal_container()." ;)
Comment #8
sunAlright. Attached patch works for me.
Comment #9
RobLoachObjects in PHP5 are always passed by reference. We probably don't need the explicit & on the return.
Comment #10
sunYes, 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.
Comment #11
effulgentsia CreditAttribution: effulgentsia commentedIt's needed for this to work:
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?
Comment #12
sunI 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.
Comment #13
neclimdulI 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.
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedBTW, I'm good with #8, but waiting on feedback from Rob before RTBC'ing.
Comment #16
sunThis patch might resolve some weird testbot issues we've had over the past days, so it would be good to get this done.
Comment #17
RobLoachMaybe 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();
?Comment #19
RobLoachI'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()...
Comment #20
sun1) 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.
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedEventually, 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.
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.
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedComment #24
effulgentsia CreditAttribution: effulgentsia commentedStupid typo.
Comment #26
effulgentsia CreditAttribution: effulgentsia commentedSame stupid typo. Copy/pasted to a different place. Ok, this should pass.
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedoverall, this looks really good, but i think we need more explanation here:
reading that, i have NFI why the call is necessary.
Comment #28
sunThat 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.
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedmeh.
Comment #30
RobLoachSince we're merging the two patches, let's also merge the titles. Meaningful titles are meaningful.
Comment #31
Dries CreditAttribution: Dries commentedGood fix. Committed.
Comment #32.0
(not verified) CreditAttribution: commentedUpdated issue summary.