form redirects removes get variables like sort and order

Explanation: If your form action is something like foo?bar=baz, the query parameters will be stripped when submitting that form.

How does a base URL prevent encoding? Because of the handling in url()?

One important difference might be the fact that url() escape its parameters whereas request_uri() returns data that is potentially insecure (XSS). As you were recently investigating drupal_goto() you might be able to comment on that before we go ahead and commit this patch?

Just cross-linking a related isuue (bug report on D6 install.php) - http://drupal.org/node/172262
Not sure if that one is caused by this entirely / partially / not at all (the patch above doesn't help), but still seems related.

My problem with Install (linked above) turned out to be completely unrelated, so you can ignore my commnets here.

Site in subdirectory, clean urls off, redirects add duplicate copies of the subdirectory.

I've faced the same problem with Drupal 5 not keeping table's sort parameters upon form submit. I used the workaround below to fix this behavior; works beautifully.

function mymodule_sample_form() {  
  // Keep the original URL in order to preserve table sorting
  global $base_url;
  $form['#redirect'] = $base_url . request_uri();
  
  ...
  
  return $form;
}

According to #2 this is apparently an issue in Drupal 6.x. I don't know whether it's still a problem in D7 or D8. Changing version because Drupal 5.x is no longer supported.

This issue still exists; just found it through a bug report in Commerce. I don't think this needs to be critical, so I'm just moving it to major since we didn't have the priority when this issue was first reported. I'm guessing this needs a re-roll...

This definitive needs simpletests, but sadly there are no extra tests for form redirection.
Here is another issue for it #1261842: Create test for form redirect

Tagging.

Here are the patches for D7 and D8.

Merging in the tests from #1261842: Create test for form redirect.

This must be something to do with the base URL, I tried a couple tests locally (without a subdomain, unlike the bot) and they passed.

Something to look into tomorrow.

Let's see some more info.

Debug code didn't take into account the absolute => TRUE in assertURL.

Ah, so it's the subdirectories? Later I'll figure out how to install Drupal locally with subdirectories, for now here's this.

If you look at the debug messages for Drupal\system\Tests\Form\RedirectTest in that last patch, the base URL is 'http://drupaltestbot654-mysql/checkout', but the results of request_uri() are '/checkout/form-test/redirect?foo=bar', and checkout is duplicated.

But is that really the expected result of request_uri()? I would have thought it would be '/form-test/redirect?foo=bar'

Hm, wonder how this might work.

#28: drupal-171267-28.patch queued for re-testing.

Here's a more straightforward patch.

For D7, we'll have to use a copy of $_GET, and unset 'q' first.

While attempting a backport to D7, I realized that there was no need for any of the content type stuff in the setUp(), I must have just copy/pasted that.
This just removes those, no other changes.

In the D7 one I didn't even bother removing 'q' from $_GET first, since url() ignores it anyway.

For the D8 patch, this could only be a temporarily solution since:

1) request() is a temporary BC shiv that I hope we can remove.

2) drupal_goto() is going to have to change anyway, as you should now be using a RedirectResponse object.

Of course, we have some Drupal-specific logic in drupal_goto(), which I don't know that we can entirely eliminate. (destination= handling, etc.) That makes me wonder if we need a DrupalRedirectResponse object of some sort, (or probably a Drupal-namespaced RedirectResponse) that replicates the handling from drupal_goto(). GET query parameter persistence should be part of that, where appropriate, but I also worry about that class ending up with hard dependencies in it if we're not careful.

So, tentatively OK with this patch for now, on condition that we spin up a new issue to discuss how to eliminate drupal_goto() entirely in favor of something cleaner that would internalize the solution to this problem.

I opened #1668866: Replace drupal_goto() with RedirectResponse.

Thanks for opening the follow-up. This looks great as an interim fix and something that should be more or less straightforward to backport. Committed/pushed to 8.x.

Thanks! Rerolled.

+1 to commit

+    drupal_goto(current_path(), array('query' => $_GET));

This needs to use drupal_get_query_parameters(). Otherwise, visiting the form at, e.g., admin/config/system/site-information will redirect you to a URL that looks like admin/config/system/site-information?q=admin/config/system/site-information.

That bug won't exist in Drupal 8, but I would think it should use drupal_get_query_parameters() there too anyway, for consistency with the rest of the codebase. Leaving at D7 for now, though.

I got the same result as #41. When submitting the form at admin/config/system/site-information I was redirected to admin/config/system/site-information?q=admin/config/system/site-information. I'm attaching a patch that replaces $_GET with drupal_get_query_parameters(). It did seem to solve that particular issue.

Good catch David.

It seems that in D8, drupal_get_query_parameters() hasn't been updated to use the kernel, and I'd be just as happy to leave it as is there.

This is RTBC for D7.

Going to kick back to David since he seems to understand this code better than I do. :)

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/50fce51

Moving to Drupal 6 for backport.

Woohoo! Happy day. : )

This is a reroll of #2 for D6.

Should be request_uri() not request_url(), but shouldn't it also backport your tests?

Thanks for catching the request_uri() problem.

As for the tests, I'll admit I'm still a core newb and this is my first 6.x patch, but I thought tests don't get backported because simpletest isn't in core. If I'm mistaken then someone please correct me.

Thanks @dcam! You're correct about the tests. Since this is now a D6 patch, it needs to be tested by hand.

Be careful backporting this to Drupal 6 - it looks like it probably caused some issues: #1836082: Drupal 7.17 breaks some form redirects (including views exposed filters)