Allows DrupalKernel::handle to handle more than one requests (rough support for PHP-PM)

I think we should class this as a task, or failing that - a bug

Will work on a test in the morning

Thanks Marc, for php-pm and for contributing!

To be honest this issue should be more like a meta issue, given that there will be many potential subissues.

One problem which you will probably run into is #2613044: Requests are pushed onto the request stack twice, popped once which will polute your memory.

There can be a meta issue, sure, but this is one targeted fix. Just targeted fix it.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -624,7 +631,9 @@ public function handle(Request $request, $type = self::MASTER_REQUEST, $catch =
   -      $this->initializeSettings($request);
   +      if ($this->settingsInitialized){
   +          $this->initializeSettings($request);
   +      }
   

   Drupal coding standards say space before the {, and 2-space indent for the method body.

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -954,6 +963,11 @@ public static function bootEnvironment() {
   +    if ($this->settingsInitialized) {
   +        throw new \LogicException('Can not initialize the kernel twice.');
   +    }
   

   2 space indent, please.

   Do we want to throw an exception here? It seems that the failure case results in the same post-condition as the success case: The kernel is now initialized and usable. Why not just return-early instead of throwing an exception, as the end result is still an acceptable state?

   Alternatively... maybe this is a good use case for an assertion?

Here is the meta issue #2218651: [meta] Make Drupal compatible with persistent app servers like ReactPHP, PHP-PM, PHPFastCGI, FrankenPHP, Swoole

I'm interested in this and I saw a small bug in the initial patch, so instead of a review, here's another patch.

-      $this->initializeSettings($request);
+      if ($this->settingsInitialized){
+          $this->initializeSettings($request);
+      }

This bit should actually check if the settings are NOT initialized, then initialize.

I also agree with #5, so removed the exception throwing as well and fixed the CS nits.

Could you please explain, why do we need this check

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -643,7 +650,9 @@ public function handle(Request $request, $type = self::MASTER_REQUEST, $catch =
+      if (!$this->settingsInitialized) {
+        $this->initializeSettings($request);
+      }

if we have this one?

protected function initializeSettings(Request $request) {
+    if ($this->settingsInitialized) {
+      return;
+    }

I mean, wouldn't it be enough to only check if settings were initialized in the initializeSettings method?

Could you please explain, why do we need this check

Good question, it's just to avoid the extra function call.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -643,7 +650,9 @@ public function handle(Request $request, $type = self::MASTER_REQUEST, $catch =
+      if (!$this->settingsInitialized) {
+        $this->initializeSettings($request);
+      }

If this is checked by initializeSettings already, why do we need to check it upfront as well?

The patch in #8 does not apply to 8.5.x-dev neither to 8.6.x-dev. Fortunately I could apply that patch manually on 8.6.x-dev and now I would like to publish the new patch. Since the patch cannot apply to 8.6.x-dev I cannot create an interdiff.

I'm new at publishing patches. Can you tell me if I can publish my new patch without an interdiff or else what do I do?

@alauzon You did well! When you don't do any logic changes you also don't need to post an interdiff. But yeah patches and interdiffs are super weird 2018.

@dawehner How about retro, retro cool or even retro chic? 😜

Issue still makes sense, moreover reactPHP tagged LTS https://github.com/reactphp/react/releases/tag/v1.0.0

The numbers of running SF with reactPHP beats php-pm and sometimes php-fpm

Re-roll and address #14 (interdiff is bigger then patch) - we use this pattern already in \Drupal\Core\DrupalKernel::boot()

OTOH instead of new property we can check that Settings already initialized (probably try Settings::getInstance() catch ... or add helper method

re-roll for 9.2

Re-roll for 9.3

LGTM.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -244,6 +244,13 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface {
    
   +  /**
   +   * The state whether initializeSettings() has been called or not.
   +   *
   

   Can probably drop 'The state' here.

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1067,6 +1074,11 @@ public static function bootEnvironment($app_root = NULL) {
   +      // Early exit to prevent repeating resource expensive operations.
   

   Is this because it's expensive or because it will fatal? Should also say early return rather than early exit.

It's expensive but may cause a fatal iirc

I think we need to consider what should occur if Kernel::shutdown() is called. I think in this instance we should not reset it. Looking at the behaviour of ::boot() and how it interacts with $this->sitePath.

Therefore I think we need to document that this will still only occur once regardless of calling ::shutdown. Hmmm actually looking at things what happens if you have code calling \Drupal\Core\DrupalKernel::setSitePath() after a shutdown()? I think this change will break expectations.

Patch for 11.x.