Posted by chx on August 28, 2007 at 7:16pm
22 followers
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | forms system |
| Category: | bug report |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | needs backport to D6, needs backport to D7, Needs manual testing |
Issue Summary
That's not good. I will have a patch made for D6 as well.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| redirect_1.patch | 794 bytes | Idle | FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch redirect_1.patch. Unable to apply patch. See the log in the details link for more information. | View details | Re-test |
Comments
#1
Explanation: If your
form actionis something likefoo?bar=baz, the query parameters will be stripped when submitting that form.#2
You must either specify a description or change something about this issue.
#3
How does a base URL prevent encoding? Because of the handling in url()?
#4
Yes. It becomes an absolute URL and external URLs are returned early, before
<?php$path = drupal_get_path_alias($path);
$path = drupal_urlencode($path);
?>
#5
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?
#6
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.
#7
My problem with Install (linked above) turned out to be completely unrelated, so you can ignore my commnets here.
#8
Site in subdirectory, clean urls off, redirects add duplicate copies of the subdirectory.
#9
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.
<?php
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;
}
?>
#10
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.
#11
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...
#12
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
#13
Tagging.
#14
Here are the patches for D7 and D8.
#16
Merging in the tests from #1261842: Create test for form redirect.
#20
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.
#21
Let's see some more info.
#23
Debug code didn't take into account the absolute => TRUE in assertURL.
#25
Ah, so it's the subdirectories? Later I'll figure out how to install Drupal locally with subdirectories, for now here's this.
#27
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'#28
Hm, wonder how this might work.
#29
The last submitted patch, drupal-171267-28.patch, failed testing.
#30
#28: drupal-171267-28.patch queued for re-testing.
#31
Here's a more straightforward patch.
For D7, we'll have to use a copy of $_GET, and unset 'q' first.
#32
I can't RTBC this -- although I would normally. I would like to see what Crell says about the usage of request.
#33
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.
#34
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.
#35
I opened #1668866: Replace drupal_goto() with RedirectResponse.
#36
OK then temporarily let's get this major fixed :)
#37
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.
#38
Thanks! Rerolled.
#39
Yay! Didn't even take five years to get this superb complicated issue fixed.
#40
+1 to commit
#41
+ 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-informationwill redirect you to a URL that looks likeadmin/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.
#42
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.
#43
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.
#44
Going to kick back to David since he seems to understand this code better than I do. :)
#45
Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/50fce51
Moving to Drupal 6 for backport.
#46
Woohoo! Happy day. : )
#47
This is a reroll of #2 for D6.
#48
The last submitted patch, drupal-171267-47.patch, failed testing.
#49
Should be request_uri() not request_url(), but shouldn't it also backport your tests?
#50
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.
#51
#52
Thanks @dcam! You're correct about the tests. Since this is now a D6 patch, it needs to be tested by hand.
#53
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)