Move DrupalKernel::initializeCookieGlobals() into page cache kernel decorator

For now we can at least extract the initialization of cookies.

Adapting the title due to #2331909: Move DrupalKernel::initializeCookieGlobals() into page cache kernel decorator and #2331913: Move DrupalKernel::preHandle code into kernel decorator

So, this time it is green.

1. +++ b/core/core.services.yml
   @@ -391,7 +391,7 @@ services:
        class: Drupal\Core\StackMiddleware\PageCache
   -    arguments: ['@kernel']
   +    arguments: ['@config.factory']
   

   <3

2. +++ b/core/lib/Drupal/Core/StackMiddleware/PageCache.php
   @@ -24,31 +25,56 @@ class PageCache implements HttpKernelInterface {
   +    if (Settings::get('page_cache_without_database')) {
   

   Can we at least inject Settings for now?

3. +++ b/core/lib/Drupal/Core/StackMiddleware/PageCache.php
   @@ -24,31 +25,56 @@ class PageCache implements HttpKernelInterface {
   +    if ($cache_enabled && !$request->cookies->has(session_name()) && drupal_page_is_cacheable()) {
   ...
   +      $response = drupal_page_get_cache($request);
   ...
   +        drupal_serve_page_from_cache($response, $request);
   

   Are these old methods used in multiple places?

4. +++ b/core/lib/Drupal/Core/StackMiddleware/PageCache.php
   @@ -24,31 +25,56 @@ class PageCache implements HttpKernelInterface {
   +        // We are done.
   +        $response->prepare($request);
   +        $response->send();
   +        exit;
   

   Did you considered to just do a return $response ? This should go back to index.php, call the same methods and so be safe.

+++ b/core/lib/Drupal/Core/StackMiddleware/InitCookies.php
@@ -0,0 +1,110 @@
+  /**
+   * Initialize cookie settings.
+   *
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   The current request.
+   *
+   * @todo D8: Eliminate this entirely in favor of a session object.
+   */
+  protected function initializeCookieGlobals(Request $request) {

We cannot eliminate this in Drupal 8, the symfony Session object has no substitute for generating a site-specific session_name.

That said I propose to move the logic into a separate service where we also can add useful methods like hasSessionCookie($request).

Related: #1934730: Alternative session handler implementation is not able to override session_name()

the symfony Session object has no substitute for generating a site-specific session_name.

@znerol: Did you considered to create an issue upstream?

Let's just also drop support for page caching in the legacy bootstrap.

Meh, this seems to be a problem with run-tests.sh

the symfony Session object has no substitute for generating a site-specific session_name.

@znerol: Did you considered to create an issue upstream?

I do not think that this is something which belongs into a framework. It seems to me that the session-cookie name falls into the app-domain, or even the site-configuration. In addition I suspect that Drupal does too much work here, for example I do not see any reason why the session-name is recomputed on every request.

From what I gather from the git-history and the issue queue the case for automatically generating the session-cookie name is mere convenience with a moderate security/privacy touch (mixed mode SSL was later added to the mix). The first version of this code was introduced with #56357: Login issues with multiple sites in the same domain (session cookie collision).

However, it seems to me that the session cookie name also could be just generated at install-time and then written to settings.php (or perhaps into a container parameter which is passed directly to SessionManager/NativeSessionStorage $options.

For the record, I've checked some Symfony sites/blogs and many simply use the default PHPSESSID cookie name.

Let's try.

Reroll after #2263981: Introduce a robust and extensible page cache-policy framework. Also remove handlePageCache() from http.php and https.php.

Thank you for your reroll. Do you think the interdiff in #18 looks sane?

This patch accidentally removed every invocation of DrupalKernel::getContainer() and therefore the container was not dumped anymore.

The Drupal\views_ui\Tests\DisplayPathTest fails to call WebTestBase::resetAll() after installing the menu_ui. Therefore maybe that module is not active in the simpletest child-site? That would mean that the assertions there are not asserting the right thing though.

Re #22: I do not have an opinion yet, I'm trying to figure out the test failures first.

Filed #2342807: DisplayPathTest methods enable menu_ui module when it is already enabled on views.

I really like it! Anyone else want to RTBC it?

+++ b/core/includes/bootstrap.inc
@@ -1009,9 +1009,6 @@ function drupal_bootstrap($phase = NULL) {
         case DRUPAL_BOOTSTRAP_PAGE_CACHE:
-          $kernel->handlePageCache($request);
-          break;

Just curious: do you remove that bootstrap phase somewhere else already? Maybe in the page cache => HTTP Cache issue itself? It feels like dropping it here would be the right place.

Re #18/#22: This looks like a perfect opportunity to actually decouple the session_name of the simpletest child site from the one the parent site uses.

The test failures in the installer and in authorize.php pinpoint the actual problem: Requests will not go through stack middleware when something is using prepareLegacyRequest() instead of handle().

This is great if the page cache is a stack middleware, it is not so great for the session cookie initialization. In order to resolve that it would be possible to introduce a new event (e.g. DrupalKernel\KernelEvents::LEGACY_REQUEST) and add request subscribers also there if necessary. Still this would result in very odd parallelism for cookie initialization because we'd end up with a stack middleware (for real requests) and a legacy request subscriber (for the installer / authorize.php).

Another possibility would be to introduce an event which replaces DrupalKernel::preHandle. That event would be fired from within DrupalKernel::handle() but also from within DrupalKernel::prepareLegacyRequest. Being executed before the page cache, this would roughly correspond to D7 hook_boot.

I'm in favor of option 2. Opinions?

Investigated Option 2, this would have the following consequences:

• KernelEvents::BOOT would be fired from within handle() and prepareLegacyRequest()
• Implement session cookie initialization as a BOOT event subscriber
• As a consequence implement the reverse proxy middleware as a BOOT event subscriber, because that needs to be run before cookie session initialization
• Implement kernel PreHandle as a BOOT event subscriber but only for legacy requests because on normal requests it needs to be called after the page cache and therefore in a stack middleware or a in a request subscriber.

Regrettably this does not resolve the "odd parallelism" problem described in #29.

+++ b/core/lib/Drupal/Core/StackMiddleware/PageCache.php
@@ -24,31 +26,71 @@ class PageCache implements HttpKernelInterface {
+    $request_policy = \Drupal::service('page_cache_request_policy');

Middlewares are in the Container, so inject this. Don't ever use \Drupal inside a class that's registered in the container.

+++ b/core/lib/Drupal/Core/StackMiddleware/InitCookies.php
@@ -0,0 +1,111 @@
+    // If we do this more then once per page request we are likely to cause
+    // errors.
+    if (static::$isRequestInitialized) {
+      return;
+    }

This doesn't need to be static. There's only a single instance of this class created anyway, so an object property is sufficient.

znerol, I'm a bit unclear on the problem you describe. But an answer of "add moar events" sounds very much like we're asking the wrong question. What exactly is "legacy request", and why does it even exist?

Is this issue title a typo? AFAIK there has never been a method called initializeCookiesIntoGlobal.

yes

Filed #2347877: Move DrupalKernel::initializeCookieGlobals() into a SessionConfiguration service as an alternative approach.

@znerol or @damiankloip is this needed considering the other approach was committed #2347877: Move DrupalKernel::initializeCookieGlobals() into a SessionConfiguration service? Can we close this?

Yeah we can.