Posted by joachim on June 11, 2012 at 7:11pm
4 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | documentation |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | needs backport to D7, Novice |
Issue Summary
API page: http://api.drupal.org/api/drupal/includes%21form.inc/function/drupal_red...
Enter a descriptive title (above) relating to drupal_redirect_form, then describe the problem you have found:
It would appear that a destination in the current request (ie, being at a path like foo/bar?destination=biz) overrides anything set in $form_state['redirect'].
Would be useful to state that the request destination always wins.
Comments
#1
It seems that this is already documented on that page?
#2
Not quite.
The above states that the combination of: no redirect + a destination -> go to destination.
My point is that it doesn't say that: redirect + a destination -> go to destination, not redirect.
#3
Ah. Patch then...
#4
tags
#5
Here's patch.
#6
The last submitted patch, core-drupal_validate_form doc fix-1627654.patch, failed testing.
#7
File name fixed.
#8
Issue status update
#9
Thanks for the patch! But check
http://drupal.org/node/1354#functions
for documentation standards (needs to have a one-line one-sentence summary).
Also I'm not sure the added paragraph is correct -- see comment #3 above?
#10
How about:
However, in case it was not defined and the current request contains a 'destination' query string, drupal_goto() will redirect to that given destination instead. If both are defined, the 'destination' query string is used. To prevent redirection completely even if a 'destination' query string is present, set $form_state['redirect'] to FALSE.
#11
RE #10 - looks good to me! It should be added to the other docs about redirection.
#12
Patch with #10.
#13
The text from 10 should replace the existing text that starts ' However, in case it was not defined and the current request contains a 'destination' query string ...'
This patch doesn't seem to be adding to a paragraph, rather, it's adding a whole new one.
#14
Fixed.
#15
Status update
#16
Thanks!
One of the lines there has an extra space on the end. If you plan to continue contributing patches to Drupal (core or contrib), check your editor -- most code editors have a settings that will either highlight or remove trailing whitespace.
Other than that, I think the patch is OK, but it's a bit redundant (it seems to say the stuff about the FALSE value for 'redirect' twice). Here's the patched text:
* - If $form_state['redirect'] is FALSE, a form builder function or form
* validation/submit handler does not want a user to be redirected, which
* means that drupal_goto() is not invoked. For most forms, the redirection
* logic will be the same regardless of whether $form_state['redirect']
* is undefined or FALSE. However, in case it was not defined and the current
* request contains a 'destination' query string, drupal_goto() will redirect
* to that given destination instead. If both are defined, the 'destination'
* query string is used. To prevent redirection completely even if a
* 'destination' query string is present, set $form_state['redirect']
* to FALSE.
Maybe this could be compacted somewhat to be less redundant?
#17
Thanks for the tip about trailing spaces:)
I've removed last sentence form description.
#18
I kind of think that this:
> To prevent redirection completely even if a
* 'destination' query string is present, set $form_state['redirect']
* to FALSE.
is clearer than the first sentence.
> does not want a user to be redirected, which
* means that drupal_goto() is not invoked
requires the reader to know that drupal_goto() is the mechanism for the query string destination too.
#19
I've tried make 1 sentence more clear. Patch
#20
Hmmm... This now says:
* - If $form_state['redirect'] is FALSE, a form builder function or form+ * validation/submit handler does not want a user to be redirected, and
+ * no rediration will be invoked. For most forms, the redirection logic will
+ * be the same regardless of whether $form_state['redirect'] is undefined
+ * or FALSE. However, in case it was not defined and the current request
+ * contains a 'destination' query string, drupal_goto() will redirect to that
+ * given destination instead. If both are defined, the 'destination' query
+ * string is used.
There's a typo in the 3rd line there (rediration -> redirection).
And this is confusing to me. It seems to first say that if $form_state['redirect'] is FALSE, there is no redirection, but then it says "If both are defined, the 'destination' query string is used". FALSE is a "defined" value...
I think this whole thing needs to be rewritten, and probably some of the documentation above too. I might take a stab at it...
#21
How's this? (How fun! I haven't made a docs patch in a while, too busy reviewing and committing others' patches. :) ).
#22
It looks good for me.
I'm afraid scope of this rewrite is to big for me, so I will come back for porting to d7.
#23
joachim: want to give this a review too?
#24
As joachim didn't respond and it's been a few weeks, and this has gone through several iterations and reviews, I went ahead and committed the above patch to D7 and D8. If changes need to be made, please add a comment and change this back to "needs work". Thanks all who participated in patching and reviewing!
#25
Automatically closed -- issue fixed for 2 weeks with no activity.
#26
Sorry, this one fell off my radar!