Updated: Comment 0

Problem/Motivation

As written in #1998638-42: Replace almost all remaining superglobals ($_GET, $_POST, etc.) with Symfony Request object we figured out that the ExceptionListener from symfony does not really allow to use POST requests on exception pages.
This for example blocks us from using login forms on 403 pages or search forms on 404 pages, which is a quite common usecase.
As asked on https://github.com/symfony/symfony/issues/9660 we should better extend the existing one instead of "fixing" the one from symfony.

Proposed resolution

Implement our own exception listener / controller that allows to use post requests as well. Therefore the subrequests need to pass along the request type as well as request information if existent.

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
9.58 KB

This works pretty fine for me.

Status: Needs review » Needs work

The last submitted patch, 1: exception-2147153.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

1: exception-2147153.patch queued for re-testing.

larowlan’s picture

Looks good, great work.
Those unit tests are slick.

Status: Needs review » Needs work

The last submitted patch, 1: exception-2147153.patch, failed testing.

chx’s picture

-      $form_state['input'] = $form_state['method'] == 'get' ? $_GET : $_POST;
+      $form_state['input'] = $form_state['method'] == 'get' ? $this->request->query->all() : $this->request->request->all();

This is odd. I guess I can get behind $_GET becoming query, after all the standard calls it query (RFC 1738 too), it's just PHP that names it $_GET. But $_POST becoming request?? That's a bug. $_REQUEST exists and a whole different animal. If this is a Symfony thing, we need a critical bug filed over this, if this is a typo in the patch then the patch need to be fixed.

chx’s picture

chx’s picture

I can't even put words my dismal that such a hopeful issue like this leads to another issue where I am fighting Symfony. I don't want to fight :(

dawehner’s picture

Status: Needs work » Needs review

1: exception-2147153.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: exception-2147153.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

Interesting those failures work fine on my system.

dawehner’s picture

1: exception-2147153.patch queued for re-testing.

damiankloip’s picture

  1. +++ b/core/lib/Drupal/Core/Controller/ExceptionController.php
    @@ -91,7 +91,13 @@ public function on403Html(FlattenException $exception, Request $request) {
    +      if ($request->getMethod() == 'POST') {
    

    Shall we just go all out and use === here?

  2. +++ b/core/lib/Drupal/Core/Controller/ExceptionController.php
    @@ -91,7 +91,13 @@ public function on403Html(FlattenException $exception, Request $request) {
    +        $subrequest = Request::create($request->getBaseUrl() . '/' . $path, 'get', array('destination' => $system_path), $request->cookies->all(), array(), $request->server->all());
    

    Might as well make them match and use 'GET'. I know symfony does an strtoupper() on the method anyway but...

  3. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionListener.php
    @@ -0,0 +1,68 @@
    +      // re-throw the exception from within HttpKernel as this is a catch-all
    

    This isn't re throwing anything

  4. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/ExceptionListenerTest.php
    @@ -0,0 +1,110 @@
    +class ExceptionListenerTest extends UnitTestCase {
    

    Can we get some @group action on here?

Crell’s picture

  1. +++ b/core/lib/Drupal/Core/Controller/ExceptionController.php
    @@ -91,7 +91,13 @@ public function on403Html(FlattenException $exception, Request $request) {
    +      if ($request->getMethod() == 'POST') {
    +        $subrequest = Request::create($request->getBaseUrl() . '/' . $path, 'POST', $request->request->all(), $request->cookies->all(), array(), $request->server->all());
    +        $subrequest->query->set('destination', $system_path);
    +      }
    +      else {
    +        $subrequest = Request::create($request->getBaseUrl() . '/' . $path, 'get', array('destination' => $system_path), $request->cookies->all(), array(), $request->server->all());
    +      }
    

    Nitpick: GET, not get, for consistency. (I don't think it matters to the code, but we should be consistent.

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionListener.php
    @@ -0,0 +1,68 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function onKernelException(GetResponseForExceptionEvent $event) {
    

    This clearly still has the original Symfony code in it; it's not clear what part you're changing, though. Can you document that, so we know what to review more effectively?

dawehner’s picture

FileSize
9.26 KB

There we go.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

There we go.

catch’s picture

15: wscci-2147153.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: wscci-2147153.patch, failed testing.

The last submitted patch, 15: wscci-2147153.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.41 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 20: exception-2147153.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

20: exception-2147153.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 20: exception-2147153.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

20: exception-2147153.patch queued for re-testing.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Still just HEAD chasing. Still RTBC.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionListener.php
@@ -0,0 +1,71 @@
+      // keep for BC -- as $format can be an argument of the controller callable
+      // see src/Symfony/Bundle/TwigBundle/Controller/ExceptionController.php
+      // @deprecated in 2.4, to be removed in 3.0
+      'format' => $request->getRequestFormat(),
+    );
+
+    $request = $request->duplicate(NULL, NULL, $attributes);
+
+    try {
+      $response = $event->getKernel()->handle($request, HttpKernelInterface::SUB_REQUEST, true);
+    }
+    catch (\Exception $e) {
+      $this->logException($exception, sprintf('Exception thrown when handling an exception (%s: %s)', get_class($e), $e->getMessage()), false);
+
+      // set handling to false otherwise it wont be able to handle further more
+      $handling = FALSE;
+
+      // re-throw the exception from within HttpKernel as this is a catch-all
+      return;
+    }

None of the inline comments here match coding standards.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.05 KB
9.41 KB

Fixed them. Yeah I should have not just c&pasted the original code.

Status: Needs review » Needs work

The last submitted patch, 27: exception-2147153.patch, failed testing.

The last submitted patch, 27: exception-2147153.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

27: exception-2147153.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 27: exception-2147153.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
704 bytes
10.1 KB

This just fixes a single of these failures. Batch is still broken with this.

Status: Needs review » Needs work

The last submitted patch, 32: exception-2147153.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.88 KB
12.7 KB

Fixed many failures.

Status: Needs review » Needs work

The last submitted patch, 34: exception-2147153.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
18.65 KB

#2176669: Stop persisting the url_generator fixed all the remaining failures.

Status: Needs review » Needs work

The last submitted patch, 36: exception-2147153.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

36: exception-2147153.patch queued for re-testing.

damiankloip’s picture

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionListener.php
    @@ -0,0 +1,71 @@
    +      // Re-throw the exception from within HttpKernel as this is a catch-all.
    +      return;
    

    This doesn't re throw.

  2. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php
    @@ -67,7 +67,7 @@ public static function getInfo() {
    +    $this->request = Request::createFromGlobals();
    
    +++ b/core/modules/system/lib/Drupal/system/Tests/Session/SessionHttpsTest.php
    @@ -32,7 +32,7 @@ public static function getInfo() {
    +    $this->request = Request::createFromGlobals();
    
    +++ b/core/modules/system/lib/Drupal/system/Tests/System/FloodTest.php
    @@ -35,8 +35,7 @@ public function setUp() {
    +    $this->request = Request::createFromGlobals();
    

    What's the reason for these changes? (mainly out of interest).

  3. +++ b/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/Kernel.php
    @@ -774,7 +774,7 @@ public static function stripComments($source)
    +        // replace multiple new lines with a single newlinel
    

    oops

  4. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/ExceptionListenerTest.php
    @@ -0,0 +1,113 @@
    +  public function testHandleWithPostRequest() {
    

    Do you think we should also add an assertion to test that the system path destination gets set here?

I guess this will no longer apply anyway, as #2176669: Stop persisting the url_generator is in, and this has that rolled into it.

Crell’s picture

36: exception-2147153.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 36: exception-2147153.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

Thank you for your review!

This doesn't re throw.

Note: This is coming 1by1 from symfony, so I think we should not deal with it here, but maybe add an issue to upstream.

What's the reason for these changes? (mainly out of interest).

Previously it set example.com to the request object which was used in the test on 403/404 pages.

Do you think we should also add an assertion to test that the system path destination gets set here?

This would just makes sense to be used on a potential unit test of the ExceptionController.

dawehner’s picture

FileSize
12.7 KB

Forgot to upload the file.

Status: Needs review » Needs work

The last submitted patch, 43: exception-2147153.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

43: exception-2147153.patch queued for re-testing.

dawehner’s picture

Title: Create a special exception listener / exception controller which allows to use POST requests » Replace the last instance of $_GET/$_POST; Create a special exception listener / exception controller which allows to use POST requests

Adapted the title.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Let's file an upstream issue. Otherwise, I think this can move forward again.

dawehner’s picture

Crell’s picture

I was referring to this:

This doesn't re throw.

Note: This is coming 1by1 from symfony, so I think we should not deal with it here, but maybe add an issue to upstream.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

In #2161397: Update to Symfony 2.4.1 we updated Symfony which added a new protected method duplicateRequest(). I think this means we can just override the new duplicateRequest() function rather than the onKernelException() function.

alexpott’s picture

Status: Needs work » Needs review
FileSize
8.23 KB
11.48 KB

Like so.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
11.47 KB
1.1 KB

HAHAHA, busted: https://github.com/symfony/symfony/commit/6af28015d4d102145bd1a4541cdf7f...

Just fixed the 3 codestyle issues.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

exception-2147153_6.patch no longer applies.

error: patch failed: core/core.services.yml:215
error: core/core.services.yml: patch does not apply

Xano’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
11.57 KB

Re-roll for the synchronized property in core.services.yml.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

jhodgdon’s picture

I think that this commit broke the patch for #1366020-59: Overhaul SearchQuery; make search redirects use GET query params for keywords -- The explanation for what is happening is on comment #59, having to do with attempting to use $form_state['redirect_route'] from a form in a block on a 404 page.

I'm not absolutely positive yet that this commit is the cause of that patch failing now, but the form block redirect code in that patch was working on Jan 25th and it is now not working, so damiankliop and timplunkett suggested that this commit is probably what broke it.

jhodgdon’s picture

Status: Fixed » Closed (fixed)

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