Big pipe cannot handle (GET) form redirects (EnforcedResponseException)

For the tests in #4

Also the issue summary should be updated to include proposed solution

We have noticed this issue is reproduce-able with the following setup:

Views reference to eg let someone add a View Block to a node (BigPipe
Views ajax history in place to make Views use 'GET'
Better exposed filters enable Reset button
Attempt to click the Reset button, and failure occurs

Hoping to investigate and reproduce this well enough without a series of contrib modules, but for now hopefully this is at least somewhat helpful for others.

The issue only occurs from Views Reference > beta4 as noted here https://www.drupal.org/project/viewsreference/issues/3380691 🐛 Issue 2919092 breaks BC and causes issues for existing sites Needs review

Hi, I confirm this issue is happening when using Better exposed filters with the reset button + Views Reference Field 8.x-2.0-beta6.
I tested the patch #3 and it kind of worked, but now when we press the Reset button, it redirects first to a URL with a query string and then to the page without the query string, so it reloads twice.

Aditi Saraf → made their first commit to this issue’s fork.

@scott_euser Thank you! That's very helpful! Any chance you could write a test that reproduces this? 🙏

@efrainh Thank you! It's also very helpful to know that the patch in #3 is an incomplete fix.

scott_euser → changed the visibility of the branch 11.0.x to hidden.

Okay I need to work on reproducing this without Views Reference Field module, a bit down my queue though unfortunately.

On the plus side, have managed to get a lot more test coverage into Views Reference Field module itself on (I assume everyone here is here because of some combination of that module and/or Views Ajax History) so hopefully welcome news!

Some progress adding a test, need to wait for ajax request, but out of time for today. Hopefully at least it gets someone going further on this (or hopefully I can come back to it soon enough)

1. Added test coverage
2. Updated issue summary to standard template

Ready for review

Ran the test-only feature

1) Drupal\Tests\big_pipe\Functional\BigPipeTest::testBigPipe
Behat\Mink\Exception\ExpectationException: The string "[{"command":"redirect","url":"\/big_pipe_test"}]" was not found anywhere in the HTML response of the current page.
/builds/issue/drupal-3304746/vendor/behat/mink/src/WebAssert.php:888
/builds/issue/drupal-3304746/vendor/behat/mink/src/WebAssert.php:363
/builds/issue/drupal-3304746/core/tests/Drupal/Tests/WebAssert.php:554
/builds/issue/drupal-3304746/core/modules/big_pipe/tests/src/Functional/BigPipeTest.php:226
FAILURES!
Tests: 4, Assertions: 153, Failures:

Which shows the test coverage, yay!

Issue summary appears complete and proposal to use a catch makes sense. Personally also like the use case added to the comment which really shows what this is getting.

I believe this one should be good.

I guess you mean the ::checkRedirectUrl(), I guess we could:

1. Break the method into two, so that the $response is handled in isolation as a separate method
2. Optionally move that method into a new (or existing??) service

So some sort of ::getSafeResponse() method somewhere and then ::checkRedirectUrl() becomes something like this semi-pseudo code?

  public function checkRedirectUrl(ResponseEvent $event) {
    $response = $event->getResponse();
    if ($response instanceof RedirectResponse) {
      $request = $event->getRequest();
      $event->setResponse(::getSafeResponse); <--- TBD where that method exists.
    }
  }

In which case then bigpipe file here can also call ::getSafeResponse.

Some other refactoring in the subscriber needed as well to make that happen, particularly if we move it elsewhere.

If splitting it into a separate (existing??) service, should it be a separate child issue to this one, or okay to be in here?

Okay I left out the destination bit, was not sure whether that should be taken across too from RedirectResponseSubscriber::checkRedirectUrl() as it felt a bit like an additional feature and I could not quite think of a use case, but maybe I am wrong.

I added test coverage for the new untrusted response expectation as well.

Should I also be redirecting the untrusted response e.g. to the 403 route, or is just sending the MessageCommand okay? For now its just sending the message as I could not do a fully like for like reproduction of the RedirectResponseSubscriber::checkRedirectUrl(); specifically the 400 status.

In any case, I think it covers the feedback relatively closely (with the notes above) so hopefully we are there or at least close to there with it.

Thanks!

Believe feedback has been addressed

Committed/pushed to 11.x and cherry-picked to 11.0.x, thanks!

This doesn't cherry-pick cleanly to 10.4.x so moving there for backport.

andypost → closed merge request !8430

I did re-roll but it adding new properties to constructor so deprecation-mamba and change record are require improvement

Snapshot of latest state of MR for usage with composer-patches and D10.3.

rebased and now looks ready

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Shouldn't we get into 10.5.x too?

We got into 11.0, 11.1, current MR for 10.4.

Patches for 10.5 and 10.4 are the same but I've created new MR

This is a hard blocker for PHP 8.4

Re-roll seems good!