Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.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.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | uc_addresses.patch | 3.66 KB | tic2000 |
| #8 | uc_addresses.patch | 3.66 KB | tic2000 |
| #1 | uc_addresses.patch | 2.4 KB | tic2000 |
| uc_addresses.patch | 1.57 KB | tic2000 |
Comments
Comment #1
tic2000 commentedAnd a new patch which takes care of the delete confirmation form too.
Comment #2
freixas commentedGreat 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'?
Comment #3
tic2000 commentedIf 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.
Comment #4
freixas commentedHmmm....
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.
Comment #5
tic2000 commentedMy 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....
Comment #6
tic2000 commentedMy 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....
Comment #7
freixas commentedI 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.
Comment #8
tic2000 commentedNo 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.
Comment #9
tic2000 commentedComment #10
freixas commentedYou 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.
Comment #11
tic2000 commentedYou can always namespace the variables. The chance of Drupal using #uc_address_some_name property I think it's pretty slim :)
Comment #12
tic2000 commentedPatch rerolled
Comment #13
freixas commentedFinally got to this. While I didn't use your patch, I used an equivalent construct. Seems to work and no longer uses drupal_goto() or $form_state. Thanks for finding this problem and working out the solution.
The fix should be in any dev release dated 12/11/2009 or later.
Comment #14
dww@freixas: I don't see any commits to CVS that reference this issue at http://drupal.org/project/cvs/262984 -- in fact, I only see one commit anytime in the last few months. Can you please confirm and commit your changes, preferably with a commit message that complies with the suggestions at http://drupal.org/node/52287 ? ;)
Thanks,
-Derek
Comment #15
freixas commentedSorry, I am still making updates and haven't checked anything in.
My time is limited and I'm trying to batch things up. Unfortunately, responding to one of the comments you made have slowed me down. :-)
Comment #16
freixas commentedComment #17
dwwRe: "My time is limited and I'm trying to batch things up."
Eeeek! Please don't do that. ;) That defeats much of the purpose of version control. Each commit should correspond to a distinct change. However, look at this commit:
http://drupal.org/cvs?commit=301000
A) It's a very large diff from 3 different issues -- makes it very difficult to review the diff and understand what changed and why.
B) If we wanted to undo one of those issues, we'd have to manually do it -- even harder since you regularly make changes without posting patches to issues. ;) If each change was its own commit, we could just revert a revision via CVS automatically, even if you never posted a patch specifically for a change.
C) When debugging something, if you hit one of these lines and looked at cvs annotate, all you'd know is that line changed because of one of those 3 issues. You'd then have to read all 3 issues and try to figure out why that line was changed to the way it is. A large part of the beauty of working with revision control and the issue queue is that when you're looking at any line in the source code, CVS annotate tells you what issue number was responsible for the last change to that line, and you can read the issue to get the whole history about why that line was changed to the way it was...
Please, please don't "batch" your changes. It's vastly better in many ways if you apply a patch for an issue, test it and commit it with a message that just references that one issue. Even if it takes you 45 more seconds per commit, the time savings in the long run, especially if you have co-maintainers or other people trying to use your code and contribute to it, are immense.
Thanks,
-Derek
Comment #18
freixas commentedYou make some good points. I'll keep them in mind.
Comment #19
dwwThanks. ;)