Problem/Motivation

#2509898: Additional uncaught exception thrown while handling exception after service changes introduced some tests that relied on dumping an ErrorContainer to disk.

However #2497243: Replace Symfony container with a Drupal one, stored in cache ensures that only the container is used and such the dumped container does not matter.

Proposed resolution

- Use a 'container_base_class' setting to ensure that the test can just write that to settings.php, run its test with the container, then move back.

Remaining tasks

- Fix it

User interface changes

- None

API changes

API addition:

- Adds the possibility to specify the container (base) class via settings.php (incorporates the idea and parts of #2489060: Allow to trace the service Container).

Data model changes

- None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because it relies on internals for testing, the unwanted behavior that a Container created by a different Kernel is cached to disk.
Issue priority Major because it blocks a critical.
Unfrozen changes Unfrozen because it only changes tests or the changes support testing.
Priorized changes Follow-up to a critical to fix it properly, reduces fragility, unblocks a critical.
Disruption Not disruptive.
CommentFileSizeAuthor
#10 decouple_error_testing-2528292-7.patch5.36 KBfabianx
#10 interdiff.txt1.46 KBfabianx
#6 decouple_error_testing-2528292-6.patch6.4 KBfabianx
#6 interdiff.txt1.77 KBfabianx
#5 decouple_error_testing-2528292-5.patch6.22 KBfabianx
#5 interdiff.txt1.59 KBfabianx
#1 decouple_error_testing-2528292-1.patch5.77 KBfabianx

Comments

fabianx’s picture

fabianx commented
Status: Needs work » Needs review
StatusFileSize
new decouple_error_testing-2528292-1.patch5.77 KB

And here is the patch, I borrowed the approach from #2489060: Allow to trace the service Container.

This reduces fragility and might help solve DrupalCI failures (not sure).

It also would hugely profit from #2493665: Add centralized container invalidation method, so I will try to get that one in first.

fabianx’s picture

fabianx commented
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
commented

Well, it is critical under the assumption that the solution you have is the only working solution for the stampede issue.

+++ b/sites/default/default.settings.php
@@ -633,6 +633,11 @@
 /**
+ * Override the default service container class.
+ */
+# $settings['container_base_class'] = '\Drupal\Core\DependencyInjection\Container';

I think its honestly fine / better to not document this, given how internal it really is.

fabianx’s picture

fabianx commented
Status: Needs review » Needs work

Got feedback from alexpott, he was okay to document (with some use cases), but wanted to remove the static:: as any code using that static would be wrong.

=> CNW

fabianx’s picture

fabianx commented
Priority: Critical » Major
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.59 KB
new decouple_error_testing-2528292-5.patch6.22 KB

- Addressed the review
- Added documentation

( uhm, clone failure, this issue is major, not critical. )

fabianx’s picture

fabianx commented
Assigned: fabianx » Unassigned
StatusFileSize
new interdiff.txt1.77 KB
new decouple_error_testing-2528292-6.patch6.4 KB

It turns out I was wrong:

The invalidateContainer method cannot be used, because then we would test the container builder on next request. *sigh*

So just added new invalidate calls and docs as interdiff.

Needs reviews now.

Change record is here: https://www.drupal.org/node/2528396

fabianx’s picture

fabianx commented
Issue summary: View changes
fabianx’s picture

fabianx commented
Status: Needs review » Needs work

settings.php are not persisted across test methods, I mis-read that.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/system/src/Tests/System/UncaughtExceptionTest.php
@@ -137,16 +153,32 @@ public function testErrorContainer() {
+    // Write back the normal container, because settings are shared across test
+    // methods.
+    $settings['settings']['container_base_class'] = (object) [
+      'value' => '\Drupal\Core\DependencyInjection\Container',
+      'required' => TRUE,
+    ];
+    $this->writeSettings($settings);
+    \Drupal::service('kernel')->invalidateContainer();

As we figured out ... we don't need this, yeah!

fabianx’s picture

fabianx commented
Title: Decouple Error testing from relying on a cached on disk-container » Decouple Error testing from relying on a cached on disk-container that is created by a different Kernel
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.46 KB
new decouple_error_testing-2528292-7.patch5.36 KB

Fixed #8 and updated IS, it still relies on a compiled container on disk (else it runs the container builder), but with the base class setting that is now a side effect and not the primary test method, e.g. other implementations are possible.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Thank you fabian!

Better safe than sorry, but also better correct than sorta correct

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

rtbc+1

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Triaged D8 critical

Committed 0f4319e and pushed to 8.0.x. Thanks!

diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
index 9ad5cae..0a4e4b8 100644
--- a/sites/default/default.settings.php
+++ b/sites/default/default.settings.php
@@ -635,9 +635,9 @@
 /**
  * Override the default service container class.
  *
- * This is useful for example to trace the service container for performance
- * tracking purposes, for testing a service container with an error condition or
- * to test a service container that throws an exception.
+ * This can be used to trace the service container for performance tracking
+ * purposes, for testing a service container with an error condition or to test
+ * a service container that throws an exception.
  */
 # $settings['container_base_class'] = '\Drupal\Core\DependencyInjection\Container';

Fixed up the comment on commit.

  • alexpott committed 0f4319e on 8.0.x 
    Issue #2528292 by Fabianx, dawehner: Decouple Error testing from relying...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I forgot to add the improvements I said I did in #13. Oops

Status: Fixed » Closed (fixed)

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