Allow form redirects to ignore ?destination query parameter

Ah, I created #2950885: Add an API to disable to automatic destination-based redirect override, didn't see this.

I don't think it is or should be restricted to the form system, if anything, it could provide an accessor that would make it easier to use but that will also result in more dependencies in FormState.

There's another issue which using different way to override destination #2640672: Make redirect.destination service update request

Workaround:

       \Drupal::request()->query->remove('destination');
       return new TrustedRedirectResponse('https://example.org', 302);

Re: @mlncn's workaround:

What if you want the page you're redirecting to to have the destination intact in the query?

I came at this issue from #2932050: The "Save & Edit" button redirects to admin/content using destination query. There, you want the destination to be temporarily overridden, but for the form submission to take you back to a form where the old destination is intact.

Thanks for the patch. Although there was discussion about the patch, it didn't look like the code was actually reviewed, so I did that below. This still needs testing.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -50,17 +51,19 @@ public function checkRedirectUrl(FilterResponseEvent $event) {
   +          }
   +          catch (\InvalidArgumentException $e) {
   +          }
   

   Nitpick: I know this is the same as the original code but I find empty catch clauses confusing. Consider adding a comment here.

2. +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
   @@ -158,6 +158,26 @@ public function setRedirectUrl(Url $url);
   +   * Determines whether the redirect respects the destination query parameter.
   +   *
   +   * @param bool $status
   

   Nitpick: "Determines" makes me think of a getter, not a setter. Consider changing the wording. Sets? Defines?

3. +++ b/core/lib/Drupal/Core/Routing/IgnoreDestinationRedirectResponse.php
   @@ -0,0 +1,11 @@
   + * Provides a redirect response which will override the destination query parameter.
   

   Nitpick: Comment is over 80 chars.

4. +++ b/core/lib/Drupal/Core/Routing/IgnoreDestinationRedirectResponse.php
   @@ -0,0 +1,11 @@
   +class IgnoreDestinationRedirectResponse extends RedirectResponse {
   +}
   

   Nitpick: Might be good to explain that the class is empty intentionally and why.

5. +++ b/core/modules/image/src/Form/ImageStyleEditForm.php
   @@ -220,6 +220,7 @@ public function effectSave($form, FormStateInterface $form_state) {
   +      $form_state->ignoreDestination();
   

   It would be good to explain why this is being set for this form.

I'm coming from contrib issue #3046178: Temporarily override the original 'destination' query parameter and am wondering whether the IgnoreDestinationRedirectReponse would help with our case (and similar ones, such as Core's node entity) or if that's a separate issue.

In the node case, the user coming from admin/content goes to node/1/edit (by hitting the edit button). Now on node/1/edit, the user clicks 'Delete' but then decides to 'Cancel'.

Now the user is sent all the way back to admin/content instead of just going one step back to node/1/edit.

This is because the overview page appended itself as ?destination, while the node edit page preserved the ?destination rather than overriding it.

While using a IgnoreDestinationRedirectReponse in the confirm form to always send the user to the edit form would cover this particular case, it doesn't seem the right thing to do. Finally, we do rely on ?destination being properly set, as otherwise we don't know where the user comes from. They might have used the 'Delete' button on the overview form, so shouldn't always be sent to the edit form.

Instead I would expect node/1/edit:

• either to override the ?destination it inherited from admin/content
• or: to prepend its own path (if ?destination could be an array).

Just did an update with 2950883-2.patch file to able to apply 8.8.x branch (also tested on 8.9.x too), attached new patch file

Just wanted to add this case where the patch didn't work...

I have a views bulk action that includes what is essentially a confirm form on steroids. The action has evolved into a rather complicated process, so it is now a 3-page multi-step form. At various stages, it needs to take the user either back to the original page from where they initiated the action, or redirect them to a node created in the submit for form step 1 (of 3). The views bulk action process automatically puts the "destination" arg in the URL, which I want to ignore in favor of what I am returning in the response.

It's failing to redirect the user to the newly created node, returning them, instead, to the original page (which is specified in the "destination=/path/to/original/page").

Via debug, I've tried to narrow it down to where it's failing. In the dispatch() method of ContainerAwareEventDispatcher, it goes through a foreach loop several times. The value of $event->response->response is correct (/sales/releases/release-12345) for several iterations through the loop. But at some point, that gets changed to the original page (/shipping/shipment-3456). I believe it is happening here:

call_user_func($definition['callable'], $event, $event_name, $this);

...which is at line 111 in my Drupal 8.8.5 version, with the #19 patch above applied.

The value of the $definition array is as follows (this may be a partial rendering, and I may not have gotten the indentation right):

$definition = {array} [2]
service = {array} [2]
0 = "redirect_response_subscriber"
1 = "checkRedirectUrl"
callable = {array} [2]
0 = {Drupal\Core\EventSubscriber\RedirectResponseSubscriber} [3]
unroutedUrlAssembler = {Drupal\Core\Utility\UnroutedUrlAssembler} [3]
requestContext = {Drupal\Core\Routing\RequestContext} [11]
completeBaseUrl = "http://localhost"
*Symfony\Component\Routing\RequestContext*baseUrl = ""
*Symfony\Component\Routing\RequestContext*pathInfo = "/actions/release-containers"
*Symfony\Component\Routing\RequestContext*method = "POST"
*Symfony\Component\Routing\RequestContext*host = "localhost"
*Symfony\Component\Routing\RequestContext*scheme = "http"
*Symfony\Component\Routing\RequestContext*httpPort = {int} 80
*Symfony\Component\Routing\RequestContext*httpsPort = {int} 443
*Symfony\Component\Routing\RequestContext*queryString = "destination=/shipping/shipment-9194"
*Symfony\Component\Routing\RequestContext*parameters = {array} [0]
_serviceId = "router.request_context"

The important part of that is the queryString entry. That's the wrong destination.

I wasn't able to copy/paste the stack from my debugger, so I've attached a couple of screenshots instead.

I'm going to try @mincn's work-around (post #10), but hopefully this will help point someone toward a fix.

Adding #3033960: After adding media, return the user to the media listing they started from as a related issue, since it is also affected by this bug.

Regarding additional test coverage, we can easily adapt some of the coverage from #3033960-52: After adding media, return the user to the media listing they started from. It proves that EntityController::addPage() is also broken by the destination parameter when there is only one bundle of the entity being added.

Fixing a typo in the issue title.

Fixing the title again to clarify the actual scope of this patch.

Here's a very basic test of this functionality, with a fail patch to prove the test is effective.

I'm wondering if instead of implementing a class to do this we implement a switch on redirect_response_subscriber to disable destination overriding. And then inject this service in the form_submitter service.

IMHO it makes sense to do this, but I think we should expose the switch on the redirect.destination service. My reasoning is that RedirectDestinationTrait is already widely used, so consuming classes wouldn't need to adjust their dependencies to inject an obscure event subscriber.

I wonder if something like this might make sense: $this->getRedirectDestination()->set(FALSE), as an explicit way of saying "completely disable the redirect destination". Internally, the RedirectDestination service would then call a ignoreQueryParameter() method on RedirectResponseSubscriber. This approach has two main benefits I can see -- very little code needs to be adjusted to benefit from this, and backwards compatibility is maintained.

reroll of #26

I've taken #26 and implemented #27 - no interdiff since it's a different approach and needed a re-roll.

I kind of agree with #29 although if RedirectDestination is calling something in RedirectResponseSubscriber anyway then we're really just putting an intermediate step inbetween for semantics because it feels weird to inject an event subscriber (fwiw I agree it feels kinda weird). That would then mean consumers could inject either service, making another "more than 1 ways to do the same thing" XD

Hiding patch #32 as it includes .orig files.

Fixed phpcs issues.

Fixed FormSubmitterTest and FormBuilderTest

Also adding a BC layer for the constructor changes, will wait for confirmation on this approach before creating the CR and updating the link.

Reroll after #3229012: Fix copy-paste mistake in code comment was committed.

Another reroll after #3130606: MockBuilder::setMethods is deprecated in PHPUnit8 and removed from PHPUnit10 was committed.

This needs documentation.

This creates a 3-level hierarchy, AFAICT, from low to high priority

- the form's redirect
- the URL destination query parameter
- the form's redirect if the form really means it

At the very least, FormSubmitterInterface::redirectForm() needs a rewrite to explain this. It would be nice to see some overview docs about this too, say on RedirectDestination?

Just a re-roll against the latest 9.4.x. Please review.

Ignore destination will remove the destination completely, in some cases it would be better if it is passed on to the next page.

This could be useful in flows, like password reset. The user would complete all steps of the password reset flow and then be redirect to the destination he/she had on the login page.

+1 for #11 / #44

Created a new MR to reroll the patch #43 to the 9.5 branch

Added a new commit trying to move the redirection to the target url, as suggested on #11 and #44.

#10 works well. But we can use this in EntityForm while waiting for a real solution accepted

$this->requestStack->getCurrentRequest()->query->remove('destination');

#3325167: Revisit the redirect to 'add block' form in the 'add block content' form appears to be blocked by this as well.

Tried starting a 11.x branch but no luck so patch it is

I started from MR 2678

Updated the RedirectTest to check the path not destination (since it was ignored)
Updated typehinting for D10
Updated trigger_error message, still needs a change record

I undid the change for core/modules/image/src/ImageStyleListBuilder.php as I wasn't sure it was needed? If it is then the solution needs to be looked at because currently when adding an image style
1. The url is messed up with the destination parameter now appearing.
2. When saving an image style instead of going back to the individual style you get redirected to the whole styles page.

Had issues with the interdiff so just made a diff.

Or this approach where I add the changes back to ImageStyleListBuilder but in RedirectResponseSubscriber I don't add the destination paramter for item 55.1

Which also solved 55.2

1. +++ b/core/core.services.yml
   --- a/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   
   +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -59,12 +66,21 @@ public function checkRedirectUrl(ResponseEvent $event) {
   +        else {
   +          $target_url = $response->getTargetUrl();
   +          $query_string = parse_url($target_url, PHP_URL_QUERY) ?: '';
   +          parse_str($query_string, $query);
   +          $target_url = substr($target_url, 0, strpos($target_url, '?')) . '?' . http_build_query($query);
   +          $response->setTargetUrl($target_url);
   

   Why do we need this 🤔

2. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -25,6 +25,13 @@ class RedirectResponseSubscriber implements EventSubscriberInterface {
   +  protected $ignoreDestination = FALSE;
   

   We can use property type declaration 😇

3. +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -136,6 +136,13 @@ class FormState implements FormStateInterface {
   +  protected $ignoreDestination = FALSE;
   

   We could use property type declaration here too 😇

4. +++ b/core/lib/Drupal/Core/Form/FormStateDecoratorBase.php
   --- a/core/lib/Drupal/Core/Form/FormStateInterface.php
   +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
   
   +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
   @@ -158,6 +158,26 @@ public function setRedirectUrl(Url $url);
   +  public function ignoreDestination(bool $status = TRUE);
   

   Wondering if this should be setIgnoreDestination given that this can be used for disabling this behavior too?

#57.1 = I'm trying to remember but lets see what happens without it.
#57.2 = added type
#57.3 = added type
#57.4 = changed all function calls to setIgnoreDestination

Added back that snippet from 57.1

Seems to grab the URL minus the destination.

Also started the CR https://www.drupal.org/node/3375113

Updated issue credits

1. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -59,12 +66,21 @@ public function checkRedirectUrl(ResponseEvent $event) {
   +          $target_url = $response->getTargetUrl();
   +          $query_string = parse_url($target_url, PHP_URL_QUERY) ?: '';
   +          parse_str($query_string, $query);
   +          $target_url = substr($target_url, 0, strpos($target_url, '?')) . '?' . http_build_query($query);
   +          $response->setTargetUrl($target_url);
   

   Can we get a comment explaining what this is doing? It seems to parse the query, and then un-parse it and re-set it on the same url?

2. +++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
   @@ -34,10 +42,17 @@ class FormSubmitter implements FormSubmitterInterface {
   +  public function __construct(RequestStack $request_stack, UrlGeneratorInterface $url_generator, RedirectResponseSubscriber $redirect_response_subscriber = NULL) {
   

   shouldn't this be ?RedirectResponseSubscriber

Will work on this tomorrow. If I don’t get to in 2 days anyone can have at it.

Assigning to myself as a submaintainer

Lets try this.

Address #60

Updated our patch for this issue during a 10.1 upgrade and something has broken with a mix of $form_state->setRedirect() and setIgnoreDestination() - now the form doesn't redirect at all, instead the page refreshes with an empty query string.

Debugging through it, somehow $target_url is becoming literally a ? after this line:

$target_url = substr($target_url, 0, strpos($target_url, '?')) . '?' . http_build_query($query);

Where before the line, target_url is http://127.0.0.1:8080/admin/content/service-status-messages/560/scheduled-transitions

and $query is empty.

I guess we need test coverage for this and only do this if $target_url contains a ?

Weird cause this test case should be testing exactly that

    // Test basic redirection, ignoring the 'destination' query parameter.
    $options['query']['destination'] = $this->randomMachineName();
    $edit = [
      'redirection' => TRUE,
      'destination' => $this->randomMachineName(),
      'ignore_destination' => TRUE,
    ];
    $this->drupalGet($path, $options);
    $this->submitForm($edit, 'Submit');
    $this->assertSession()->addressEquals($path);

Actually no that's incorrect, it should be $this->assertSession()->addressEquals($edit['destination']);

EDIT 2: Just so it's clear (because calling something "destination" when it's actually the redirect URL in the context of this test is extremely confusing), we do this in the test form:

$form_state->setRedirectUrl(Url::fromUserInput('/' . $form_state->getValue('destination')));

Addressing this bug, looks like it was first introduced in #55

The snippet was added in the MR to retain the destination in the url

Fixed, also added coverage for when the redirect url itself contains a query param.

@smustgrave no, the MR still had the correct check in the test https://git.drupalcode.org/project/drupal/-/merge_requests/2678/diffs#62...

Was referring to $target_url = substr($target_url, 0, strpos($target_url, '?')) . '?' . http_build_query($query);

But #69 is all green and still working.

Would be nice to get this in as it's a blocker for a block_content redirect.

+++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
@@ -59,12 +66,25 @@ public function checkRedirectUrl(ResponseEvent $event) {
+          // The 'destination' parameter should remain in the query but be
+          // unused by the target_url.
+          $target_url = $response->getTargetUrl();
+          if (str_contains($target_url, '?')) {
+            $query_string = parse_url($target_url, PHP_URL_QUERY) ?: '';
+            parse_str($query_string, $query);
+            $target_url = substr($target_url, 0, strpos($target_url, '?')) . '?' . http_build_query($query);
+          }
+          $response->setTargetUrl($target_url);

Sorry if I'm being daft here, but I don't understand what this code is doing.

It seems to
a) get the query
b) if it contains a ? parse the query
c) recreate the same query

https://3v4l.org/XYAX5 - the body of queryManip is copy pasted from here

Confirming #57, #60 and #74 - this code is redundant.

What it looks to be trying to achieve is what's described in #11 and #44 which while it might be nice, is IMO out of scope of this issue. If people want this functionality (retaining the destination query param on the redirected URL) then please file a follow up. That way we can discuss specific use cases and have a more targeted discussion.

Awesome!

Follow-up issue created: #3376832: Pass on desgination redirect when ignoring form redirects' ?destination query parameter

1. +++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
   @@ -27,6 +28,13 @@ class FormSubmitter implements FormSubmitterInterface {
   +  protected $redirectResponseSubscriber;
   

   We can typehint this

2. +++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
   @@ -34,10 +42,17 @@ class FormSubmitter implements FormSubmitterInterface {
   +      @trigger_error('Calling ' . __CLASS__ . '::__construct() without the $redirect_response_subscriber argument is deprecated in drupal:10.2.0 and is required in drupal:11.0.0. See https://www.drupal.org/node/XXXX', E_USER_DEPRECATED);
   

   This needs an actual change record URL

CR added https://www.drupal.org/node/3377297

Type and comment updated.

Feedback addressed

Committed 92abbb5 and pushed to 11.x. Thanks!

Thanks all, we got there in the end.

Publishing both change records.

Opened a minor follow-up for this: #3377882: \Drupal\Core\Form\FormSubmitter::__construct $redirect_response_subscriber missing default value.