Don't use drupal_goto in uc_addresses_get_address_form_submit()
| Project: | Ubercart Addresses |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Background:
What I try to do is "moving" the address form to a new page, not on user pages. All goes well, only that when an addresses is added/edited the form redirects to the user pages.
Solution:
As http://api.drupal.org/api/drupal/developer--topics--forms_api.html/6 in "Submiting Forms" point 4 states, the proper way to redirect forms on submition is the use of $form_state['redirect'] or otherwise any submit function attached to the form using hook_form_alter/hook_form_FORM_ID_alter will not trigger because the redirection will occur before the function is called.
Problems:
The form stores some inf in $form_state['storage'] although it's not a multistep form. This means that when you hit the submit button the redirection does not occur because it thinks we did not finish (maybe that's why the author used drupal_goto in the first place).
That's why I stored the values we need in $form['#VARIABLE']
Patch attached.
| Attachment | Size |
|---|---|
| uc_addresses.patch | 1.57 KB |
#1
And a new patch which takes care of the delete confirmation form too.
#2
Great stuff. Maybe I can actually use this approach in my uc_product_keys module.
Could you explain the use of '#address', '#user', etc. Couldn't these potentially conflict with future versions of Drupal? Why not just 'address' and 'user'?
#3
If this would conflict with future versions of Drupal a module update for that version would change them. But I don't think form.inc changed in that regard in Drupal 7.
$form['address'] would be treated like a regular form element so it would be outputed (or at least try to output it), but maybe an error would occure, because if no type is defined it implies a markup type and expects a '#value'.
If you look at node module for example you will see that the node object is passed around in $form['#node'] for the same reason.
#4
Hmmm....
When I store stuff in form elements, I've been using this pattern:
$form['my_storage'] = array(
'#type' => 'value',
'#value' => $my_value,
);
This seems to avoid the problem of future name collisions and the problem of having the element treated as a form element to display.
As for $form['#node'], since it's in core, the Drupal people are probably aware that they shouldn't adopt it as a form attribute. They wouldn't know to avoid #user or #address. Unless there is something I'm missing, it seems like my approach would be a safer way of doing this. As always, I'm happy to be corrected.
#5
My point was that it uses '#variable', and not that you should use exactly '#node'. You can check this link:
http://api.drupal.org/api/drupal/developer--topics--forms_api_reference....
#6
My point was that it uses '#variable', and not that you should use exactly '#node', not that that is a problem if you use it on other that node forms. You can use as name anything you want as long as it doesn't conflicts with form api (#action, #submit, etc) or another variable set in the form (#node in altering node forms or #user in user forms), unless you know what you're doing.
You can check this link:
http://api.drupal.org/api/drupal/developer--topics--forms_api_reference....
#7
I was completely puzzled until I saw this little caveat at the bottom:
This seems equivalent to saying "Drupal will never add any new internal properties to the Form API" because if does, it could potentially screw up code somewhere that people were told would be safe.
I suspect the real situation is more like: If we need to add a new internal property, we will add it and list it on the instructions for upgrading modules from one version to the next.
I still suggest using my approach as avoids potential future problems without requiring much extra work.
#8
No that seems equivalent with "for Drupal X with have this Form API properties, everything else it's safe to use, because there is a code freeze and there is no way we add new properties in a released version of Drupal".
As upgrading modules goes, I think there are far "worst" changes that need to be done than changing a $form['#foo] to a $form['foo'] as type 'value', that if that type 'value' would actually exists in the new Form API.
For Drupal 6 we have a Form API and the approche I use it's valid. You're aproach is valid. It's just a case of taste. I prefer to store complex values in $form['#foo'], and node form does that too.
Anyway, a new patch wich wraps the submit handling in an else so the redirect for the delete is not overwritten by that of the submit.
#9
#10
You said:
and I said
so I think we both understand the realities of Drupal.
Thanks for the patch. I'll get this in as soon as I have a free moment to patch the code.
#11
You can always namespace the variables. The chance of Drupal using #uc_address_some_name property I think it's pretty slim :)