Download & Extend

Wrong implementations of forms _submit() functions.

Project:MySite
Version:5.x-3.3
Component:Code
Category:bug report
Priority:normal
Assigned:Unassigned
Status:closed (won't fix)

Issue Summary

As per the forms api quick start guide, a _submit function should return the url to be redirected to and not use drupal_goto directly.

http://api.drupal.org/api/file/developer/topics/forms_api.html/5
Look under the section labeled "Submitting Forms", bullet number 4:

The return value of the _submit function will be the target of a drupal_goto; every form is redirected after a submit. If you return nothing, the form will simply be redirected to itself after a submit.

Wrong:

<?php
function myform_submit($form, $form_values) {
 
// do your thing
 
drupal_goto('somwhere');
}
?>

Good:

<?php
function myform_submit($form, $form_values) {
 
// do your thing
 
return 'somwhere';
}
?>

There are various forms _submit() function in the mysite module which are calling drupal_goto() instead of returning.
This means that I cannot use hook_form_alter() and override the destination by altering $form['#redirect'] like I am supposed to.
http://api.drupal.org/api/file/developer/topics/forms_api_reference.html...

Comments

#1

Status:active» needs review

Here is proposed patch

AttachmentSize
521246.patch 2.6 KB

#2

Looks good. Does it break anything?

#3

I did not take the time to do full suite of tests.
Does it even have a suite of unit test for d5?

#4

Good old fashioned form clicking is the only way to go. The only thing I could see breaking is the fake multi-step form stuff in edit and settings, but I don't see that affected here.

Those gotos(), btw, are a legacy of 4.7.x, I think.

#5

Status:needs review» reviewed & tested by the community

Looks good. I will roll a new release.

#6

Status:reviewed & tested by the community» needs work

Actually, there are instances of these all over the Type includes as well.

#7

Status:needs work» needs review

Here's an extended patch. One good review and I'll roll a new realease.

AttachmentSize
521246-extended.patch 6.98 KB

#8

Status:needs review» closed (won't fix)
nobody click here