Updated: Comment #N

Problem/Motivation

Drupal still uses a copy of the HttpKernel class which once was part of the Symfony Framework Bundle. However this class is long gone (deprecated in 2.2 and removed in 2.3) and therefore we also should get rid of our copy.

There are two candidates we could go for as a replacement for the custom http kernel:

  1. Symfony\Component\HttpKernel\HttpKernel
  2. Symfony\Component\HttpKernel\DependencyInjection\ContainerAwareHttpKernel

In fact the latter is the successor of the HttpKernel we copied over from the framework bundle and only differs from the former that it still manages the request scope.

Regrettably just swapping out our custom kernel with ContainerAwareHttpKernel causes problems all over the place. Unlike our implementation it sets the request to NULL before leaving the request scope. In order to make our services compatible with the ContainerAwareHttpKernel we'd need to adapt them such they accept NULL for the $request parameter.

The other alternative (switching to Symfony\Component\HttpKernel\HttpKernel) requires us to convert all our services to the recently introduced request_stack. Given that Symfony moves away from request service as well as from container scopes, and that we have to touch all our request-dependent services anyway, this seems the better alternative.

Proposed resolution

  1. Convert all request-dependent services to request_stack
  2. Get rid of the request-scope.
  3. Remove Drupal\Core\HttpKernel and replace it with Symfony\Component\HttpKernel\HttpKernel

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

znerol’s picture

Let's see what explodes.

znerol’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: 2223189-remove-http-kernel.patch, failed testing.

znerol’s picture

FileSize
56.28 KB

We probably will need to go a looong way and convert everything to the new RequestStack before being able to get rid of this legacy. Let's hope that this time we don't hit a fatal error...

znerol’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: 2223189-remove-http-kernel-3.patch, failed testing.

znerol’s picture

Ok, the request-service (and worse \Drupal::request is spread all over the place. Let's take little steps: #2223593: Decouple the router.request_context service from the request service.

znerol’s picture

znerol’s picture

znerol’s picture

dawehner’s picture

I wonder whether we should make our code more request passing along like.

znerol’s picture

Just discovered that there is already an issue underway which helps us to decouple the current_user service from request: #2180109: Change the current_user service to a proxy. Great!

dawehner’s picture

Title: Remove HttpKernel » Remove the custom Drupal http kernel

Renaming title to not get confused again.

znerol’s picture

znerol’s picture

znerol’s picture

Issue tags: +WSCCI
znerol’s picture

Issue summary: View changes
znerol’s picture

Issue summary: View changes
znerol’s picture

znerol’s picture

Status: Needs work » Needs review
FileSize
24.44 KB

Let's see which tests are still blowing up.

Status: Needs review » Needs work

The last submitted patch, 20: 2223189-remove-http-kernel-20.patch, failed testing.

sun’s picture

Title: Remove the custom Drupal http kernel » Use regular upstream HttpKernel instead of Drupal's custom
cordoval’s picture

Status: Needs work » Needs review
FileSize
399.89 KB

I redid the changes on 8.x bleeding

there were some wrong things i corrected, from now on I talked with @crell and he told me to create a child issue and work on the propagation of the stuff

Status: Needs review » Needs work

The last submitted patch, 23: 2223189-23.patch, failed testing.

cordoval’s picture

FileSize
23.37 KB

reuploading patch rebased

cordoval’s picture

Status: Needs work » Needs review
cordoval’s picture

wait a sec, this is messed up,

will this be Kernel from symfony rather than HttpKernel?

ping @crell, @effulgentia

cordoval’s picture

oh nevermind I was confused :), thsi is still good!

Status: Needs review » Needs work

The last submitted patch, 25: 2223189-25.patch, failed testing.

znerol’s picture

Crell’s picture

Status: Needs work » Fixed

This was completed by other issues.

Status: Fixed » Closed (fixed)

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