Posted by sun on September 16, 2009 at 6:20pm
11 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | API clean-up |
Issue Summary
While working on #578520: Make $query in url() only accept an array, I discovered:
$form['#redirect']
$form_state['redirect']
$form['destination']
$form_state['no_redirect']
Comments
#1
you forgot hook_drupal_goto_alter. wth is $form['destination'] thats new to me and what are the rules for form_state[no_redirect]? Where this got added? Someone should dig around in cvs annotate and ask the original writer.
#2
sub
#3
Killing $form['#redirect'].
Is almost entirely replaced by $form_state['redirect'] already. Patch makes additionally use of $form_state['no_redirect'] in ajax.inc.
Which leaves us with $form['destination'], which is potentially just cruft we carried over from old days. I'll investigate.
#4
Indeed. $form['destination'] is crap we carried over from D5 without notice. Since those had no effect since D6, let's just remove 'em.
#5
OH JOY. $form gone from drupal_redirect_form and there was much much rejoicing. For those who do not remember that's an ancient remnant in FAPI.
#6
Sorry. I just realized what is form['destination'] -- it sets REQUEST['destination'] for drupal_goto and that REQUEST really should be GET and wherever we used the form destination so that if an action is set (do we have ANY place in core where that is set??) then after the form submission Drupal finds its way back to the page but then we can put the destination in the query string.
REQUEST is broken however because it's contents is PHP.INI dependent. So. Change please REQUEST to GET in drupal_goto and let's see what breaks.
#7
hah, that was fun... :) Both chx and me had no clue about WTF $form['destination'] could be... puzzled in CVS history, and, yes, we should just try to break that. :)
btw, that would go hand in hand with #582456: Make confirm_form() respect query string destination 8)
But for now, I realized that we can just replace $form['destination'] with $form_state['redirect'] ;)
Next one will try to break.
#8
Let's break $_POST['destination']. :)
#9
The last submitted patch failed testing.
#10
#11
This is with patch from #8, but notice small adition inside drupal_access_denied
#12
The last submitted patch failed testing.
#13
And here the patch that i forgot to attach.
#14
#15
Fixing that weird test after a lengthy, awesome debugging session with litwol in IRC. :)
Lessons learned: $_REQUEST['foo'] = 'bar'; is NOT like $_GET['foo'] = 'bar'; -- Which of both ends up in $_GET?
#16
One hack less?
#17
This is very promising, there is a "user is redirect to" which comes from the original comments but since we are touching it anyways, let's fix it.
#18
*redirected
#19
I truly love this. REQUEST bites the dust, that's exquisite.
#20
+++ includes/ajax.inc 20 Sep 2009 06:08:53 -0000@@ -184,7 +184,7 @@ function ajax_get_form() {
// Since some of the submit handlers are run, redirects need to be disabled.
- $form['#redirect'] = FALSE;
+ $form_state['no_redirect'] = TRUE;
Can't
$form_state['no_redirect'] = TRUE;be replaced by$form_state['redirect'] = FALSE;? Would that allow us to get rid of$form_state['no_redirect']too? The reason I'm asking is because the snippet below is somewhat counter-intuitive and confusing:+++ includes/form.inc 20 Sep 2009 08:23:15 -0000@@ -581,17 +581,17 @@ function drupal_process_form($form_id, &
+ // - $form_state['no_redirect'] is set to TRUE.
+ // - $form_state['redirect'] is set to FALSE.
#21
$form_state['redirect'] can be set by the form builder itself and can be altered by any form submit handler during processing.
$form_state['no_redirect'] is supposed to be defined in the initial $form_state only (like in ajax.inc) to explicitly prevent/disallow redirection of the form, regardless of what's in $form_state['redirect']. Form builders/handlers are not supposed to alter $form_state['no_redirect'], only $form_state['redirect'] - but there is nothing that would prevent them from doing it.
I agree that this construct is a bit fragile, but it's not invented here. Instead, we just clean up other stuff in this API that was not removed when these two were introduced.
I'll try to incorporate this info into the code comments.
#22
Incorporating that information in the code comments would be great, then we can revisit it later. Thanks for the clarification, and thanks for rerolling!
#23
Thanks! That additional clarification allowed for another re-thinking and further clean-up:
We now just pass $form_state to drupal_redirect_form() and keep all the redirection logic along with the documentation about the behavior in one place.
Less magic, less grepping for invocations of drupal_redirect_form(), less mess to understand it.
I've tried to be very verbose in the PHPDoc about the relevant options. And I also didn't squeeze all conditions into one giant if statement, but instead made the code as readable and understandable as possible, so developers can read the PHPDoc, but can also quickly scan the actual code to grok it.
The order of conditions in PHPDoc doesn't match the code, since I wanted to keep the original order in the code, but move the most important properties for developers to the beginning in the PHPDoc.
I love this one. :)
#24
Updated two comments in batch.inc and batch_process().
#25
The last submitted patch failed testing.
#26
Sorry, silly mistake. Tests should pass now.
#27
Together with the phpDoc, I can actually understand the form APIs redirection handling now, instead of having to study the code for 2 hours straight. That is a good success metric. ;-)
Truly great code clean-up, sun and chx. Committed to CVS HEAD. Great job.
#28
So, I have D6 code that changes the redirect destination after a comment. Thats done with by setting $form['#redirect'] in hook_form_alter(). It looks like in D7, one has to add a submit handler after comment modules handler since that is what sets $form_state['redirect']. IMO, thats not an improvement. Not worht re-opening this issue I guess, but #redirect was useful.
#29
Automatically closed -- issue fixed for 2 weeks with no activity.
#30
This was not quite completed.
a) There is no documentation on http://drupal.org/update/modules/6/7 about #redirect being gone that I can see. What should we tell D6 updaters to do?
b) There are still two places in modules/locale/locale.admin.inc that are trying to use $form['#redirect']. What should they be doing?
#31
and
c) It needs to be updated in the Form API Reference.
I'll take these on.
@jhodgdon, for b) they use $form_state['redirect']. We'll probably want a separate issue for that.
#32
also
c) #redirect needs to be removed from the FAPI reference guide for Drupal 7.
#33
#34
Followup on b) (locale.admin.inc) is at #766200: Obsolete locale usage of $form['#redirect'] needs to be changed to $form_state['redirect'].
#35
a) Replace $form['#redirect'] with $form_state['redirect']. The value is identical. That said, (but different issue) $form_state['redirect'] supports the same arguments as drupal_goto() in D7.
b) most probably see a), but I'll review that patch separately.
c) Right, Form API reference needs to be updated. We badly need to put that into code. :-/
d) This patch also removed support for $_REQUEST['destination'] in favor of $_GET['destination']. All code needs to be replaced like that, too.
@moshe: Sorry for recognizing your follow-up earlier. Especially when considering multiple form actions/buttons, I think it would be wrong to force a certain, global redirect target via hook_form_alter() for all form actions. Requiring developers to override the form redirection target in a dedicated form submit handler for the intended form action therefore sounds like an improvement to me.
#36
RE #35 c - If you'd like to comment on how to move the FAPI into code, we have an issue on that:
#100680: Automatically generate Forms API & other big array documentation
Constructive suggestions on how to manage it would be welcome there.
#37
#38
Looks reasonable to me. Thanks rfay!
#39
Automatically closed -- issue fixed for 2 weeks with no activity.
#40
After reading #21 and #28, I'm hoping for some clarification. Is it intended that a redirect can no longer be set by a form builder function or a hook_form_alter()? With $form['#redirect'], people were able to do this. With $form_state['redirect'], it's not clear. If the intention is that it can only be set during drupal_process_form(), we should probably document that in the module upgrade guide. If the intention is that it ought to be settable from a form builder function or hook_form_alter(), then we have a bug, because that doesn't work when the form is cached, because 'redirect' is one of the keys in form_state_keys_no_cache(), and we can't simply remove it from there, because if two submit handlers run and one sets 'redirect' and one sets 'rebuild', then we need to remove the 'redirect'.
#41
The question in #40 is specifically about $form_state['redirect'] and whether it should remain in form_state_keys_no_cache(), but regardless of that, please see #800460: WTF coupling between $form_state['cache'] and form processing.
#42
#43
Note: Documenting $form_state (including $form_state['redirect']) in #802746: Document $form_state and form builder function in form_api group. Comments and participation welcome.
#44
Not sure what's left here? #40 re-opened, but moved to a new issue, which seems to fix #40 and #41. #43 referenced an already fixed issue only.
Therefore, reverting status. Feel free to re-revert. ;)
#45
what about #action? how does it work? it got me all confused up when trying to redirect after submitting comments...
#46
barraponto, you probably know this, but #action is for *submitting* to a different location, which is a very uncommon situation. If you want to redirect after submission, use the standard technique of $form_state['redirect'] = 'some/path';