Currently Decisions implements decisions_voting_form_submit() and decisions_voting_form_validate() which contain some business logic and then call the relevant decisions sub-module's validation and submission functions. I agree that we should have a hook to define the function to initially define the decisions_voting_form, but I feel like the decisions_voting_form_submit and decisions_voting_form_validate functions should be eliminated and replaced with whatever submission and validation functions are defined by the form.
Motivation: As a developer, I may wish to develop a custom voting form, such as in #691782: Ajax submission, or customize the form in some way that involves changing validation and submit handlers.
Problem: I am expecting the voting form to behave like standard Drupal forms whose validation functions are the ones I would expect to see based on the form id (form_name_validate) and whichever ones are added in $form['#validate] and $form['#submit']. Instead, Decisions.module implements my forms as the contents of its own decisions_voting_form form and forces me to the accompanying code around submission and validation. The validation function provided assumes my form is structured in a particular way and the accompanying code includes a drupal_goto which interrupts the processing for an ajax submission and includes a "your vote has been registered" message that isn't appropriate in my case.
Also, rather than including the $node as part of the form values, we store only the nid and reload the node for validation, which seems inefficient.
Solution: Let's just let decisions_voting_form() call our form and let the Form API take it from there. The submodules already contain the code to validate and process submission of their forms. The refactoring would be minimal and mostly involve changing function names.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 708318-c.patch | 11.33 KB | ezra-g |
| #5 | 708318-b.patch | 9.19 KB | ezra-g |
| #4 | 708318-a.patch | 8.91 KB | ezra-g |
| #2 | 708318.patch | 7.62 KB | ezra-g |
Comments
Comment #1
anarcat commentedI agree: there's a lot of cruft in the form handling that dates back from the D5 to D6 port that wasn't cleanly done. It works, but that's it: it doesn't use a lot of the new forms API features D6 has or even the shortcuts that it allowed in D5.
Comment #2
ezra-g commentedI tested selection and ranking with this refactoring and everything seems to behave as expected.
Comment #3
anarcat commentedOkay, that's a good start. I have again a few whitespace issues with the patch, especially the first part where the functions are commented out instead of properly deleted: it makes the patch harder to read as I can't figure out if the removed code is handled anywhere else...
There are also small things like this:
... that shouldn't be in that patch. Otherwise, it seems like a proper way of doing this.
Comment #4
ezra-g commentedIndeed, also missed the commented out section looking at the diff. Here it is with that and whitespace changes removed. I took out "implementation of the submit() hook" since we're removing the decisions voting form submission and using the Form API instead, and the function name explains that this is a submit handler. Let me know if there is another comment you'd rather see there.
Thanks!
Comment #5
ezra-g commentedHere's one more version that restores the
drupal_set_message(t('Your vote was registered.'));.Comment #6
ezra-g commentedJust noticed that we can also remove the "return $OK" as validation functions in D6 do not need to return anything.
Comment #7
ezra-g commentedHere's a re-roll with the above change.
Comment #8
ezra-g commentedThis is now committed.