Problem/Motivation

Since #2613044: Requests are pushed onto the request stack twice, popped once, the request stack is managed with the following logic (copied from core.services.yml):

    # Drupal pushes to the request stack in DrupalKernel::preHandle(), called
    # from KernelPreHandle::handle(). Drupal pops main requests in
    # DrupalKernel::terminate() and sub requests in KernelPreHandle::handle().

This does not work for the requests generated by KernelTestBase::drupalGet(), however. They are pushed in DrupalKernel::preHandle(), but DrupalKernel::terminate() never runs as part of the test.

Steps to reproduce

You would expect

      $this->assertEquals('/', \Drupal::request()->getRequestUri());
      $this->drupalGet('/banana');
      $this->assertEquals('/', \Drupal::request()->getRequestUri());

to pass in a kernel test. It will fail, however, on the second assertion because the "current" request URI will still be /banana.

Proposed resolution

?

Remaining tasks

User interface changes

-

Introduced terminology

API changes

-

Data model changes

Release notes snippet

Issue fork drupal-3619950

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

tstoeckler created an issue.

tstoeckler opened merge request !16917

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Added a quick MR with the basic test coverage from the issue summary.

Not sure how to solve this. We could add a pop() to drupalGet() directly, but that would still leave this issue if people do $this->getSession()->visit() "manually". I guess we probably want to discourage that generally, but still maybe not great to pollute the request stack in that case? So alternatively we could add an overridden HttpKernelBrowser which pops the request at the end of doRequest()?

nitinkumar_7’s picture

nitinkumar_7 commented

The regression test looks good and correctly covers the reported request-stack pollution after drupalGet().

nitinkumar_7’s picture

nitinkumar_7 commented

One minor suggestion: instead of asserting the hard-coded / URI, could we capture the original request and assert that it is still the current request after drupalGet()?

$original_request = \Drupal::request();

$this->drupalGet('/system-test/main-content-handling');

$this->assertSame($original_request, \Drupal::request());

This would make the test more directly verify that the original request is restored, rather than only checking its URI.