Problem

  1. Only the Request is persisted across DrupalKernel rebuilds currently, the RequestStack is lost entirely.
CommentFileSizeAuthor
#8 drupal8.request-stack.8.patch3.51 KBsun
#4 drupal8.request-stack.3.patch3.13 KBsun
#2 drupal8.request-stack.2.patch2.54 KBsun
drupal8.request-stack.0.patch2.98 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

znerol’s picture

znerol CreditAttribution: znerol commented 
index 8172b48..de1fbf7 100644
--- a/core/lib/Drupal/Core/DrupalKernel.php
+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -378,6 +378,9 @@ protected function initializeContainer() {

@@ -378,6 +378,9 @@ protected function initializeContainer() {
       if ($this->container->initialized('request')) {
         $request = $this->container->get('request');
       }
+      if ($this->container->initialized('request_stack')) {
+        $request_stack = $this->container->get('request_stack');
+      }
     }
     $this->container = NULL;
     $class = $this->getClassName();
@@ -456,6 +459,9 @@ protected function initializeContainer() {

@@ -456,6 +459,9 @@ protected function initializeContainer() {
     if (isset($request)) {
       $this->container->set('request', $request);
     }
+    if (isset($request_stack)) {
+      $this->container->set('request_stack', $request_stack);
+    }

There is no need for custom persistence logic for the request stack, we simple can tag the service with the persist tag.

The reason behind special-casing the request service is that we need to cater for the request scope. We do not have this problem with the request stack.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
drupal8.request-stack.2.patch2.54 KB

Replaced custom persistence code in DrupalKernel with 'persist' tag on request_stack service.

The other adjustments are still required, since the 'persist' logic only applies to DrupalKernel::updateModules(), but those instances are replacing the DrupalKernel altogether with a new instance.

dawehner’s picture

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

This is good for now!

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
drupal8.request-stack.3.patch3.13 KB

Grepping for 'new DrupalKernel' yielded another instance of a custom kernel in update.php, which also doesn't use HttpKernel yet.

For completeness, including the grep results:

$ grep -r 'new DrupalKernel' core/
core/includes/bootstrap.inc:  $kernel = new DrupalKernel('prod', drupal_classloader(), !$test_only);
core/includes/bootstrap.inc:    $kernel = new DrupalKernel('prod', drupal_classloader());
core/includes/install.core.inc:  $kernel = new DrupalKernel($environment, drupal_classloader(), FALSE);
core/includes/install.inc:  $kernel = new DrupalKernel('prod', drupal_classloader(), FALSE);
core/scripts/run-tests.sh:  $kernel = new DrupalKernel('testing', drupal_classloader(), FALSE);
core/update.php:$kernel = new DrupalKernel('update', drupal_classloader(), FALSE);
#
core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php:    $this->kernel = new DrupalKernel('unit_testing', drupal_classloader
(), FALSE);
core/modules/simpletest/lib/Drupal/simpletest/TestBase.php:    $this->kernel = new DrupalKernel($environment, drupal_classloader(), FALSE);
#
core/modules/system/lib/Drupal/system/Tests/DrupalKernel/DrupalKernelTest.php:    $kernel = new DrupalKernel('testing', $classloader);
core/modules/system/lib/Drupal/system/Tests/DrupalKernel/DrupalKernelTest.php:    $kernel = new DrupalKernel('testing', $classloader);
core/modules/system/lib/Drupal/system/Tests/DrupalKernel/DrupalKernelTest.php:    $kernel = new DrupalKernel('testing', $classloader);
core/modules/system/lib/Drupal/system/Tests/DrupalKernel/DrupalKernelTest.php:    $kernel = new DrupalKernel('testing', $classloader);
core/modules/system/lib/Drupal/system/Tests/DrupalKernel/DrupalKernelTest.php:    $kernel = new DrupalKernel('testing', $classloader);
core/modules/system/lib/Drupal/system/Tests/System/IgnoreSlaveSubscriberTest.php:    $kernel = new DrupalKernel('testing', drupal_classloade
r(), FALSE);

The last submitted patch, drupal8.request-stack.0.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: drupal8.request-stack.3.patch, failed testing.

The last submitted patch, 2: drupal8.request-stack.2.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal8.request-stack.8.patch3.51 KB

Tests for the interactive installer (InstallerTestBase) are calling into rebuildContainer() to boot into the newly installed site environment after installation, but at that point, $this->container still contains the super-minimal container from TestBase::prepareEnvironment(), which does not have request stack.

Attached patch adjusts TestBase::rebuildContainer() for that, including comment to explain the situation.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC, I guess. Only difference in latest patch is the compatibility fix for InstallerTestBase.

znerol’s picture

znerol CreditAttribution: znerol commented

Ha, just was about the RTBC that. The fix for the installer test makes sense.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Just to clarify:

@znerol and I briefly discussed whether we could add any sensible tests here.

However, the manual DrupalKernel (re)builds are not worth to test, because pretty much all of them are caused by legacy code that is slated for removal.

If at all, I was thinking about the persistence in DrupalKernel::updateModules(), but that should be covered by 'persist' tag tests already.

Thus, what we're fixing here is essentially just a malformed service definition. There's an upper limit of sensible tests, and an incomplete/incompatible/malformed service definition exceeds that limit.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

  • Commit 7b392ea on 8.x by catch: 
    Issue #2229223 by sun: RequestStack is not persisted across kernel...

Status: Fixed » Closed (fixed)

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