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.

CommentFileSizeAuthor
#7 708318-c.patch11.33 KBezra-g
#5 708318-b.patch9.19 KBezra-g
#4 708318-a.patch8.91 KBezra-g
#2 708318.patch7.62 KBezra-g

Comments

anarcat’s picture

I 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.

ezra-g’s picture

Status: Active » Needs review
StatusFileSize
new7.62 KB

I tested selection and ranking with this refactoring and everything seems to behave as expected.

anarcat’s picture

Status: Needs review » Needs work

Okay, 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:

- * implementation of the submit() hook
+ 

... that shouldn't be in that patch. Otherwise, it seems like a proper way of doing this.

ezra-g’s picture

Status: Needs work » Needs review
StatusFileSize
new8.91 KB

Indeed, 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!

ezra-g’s picture

StatusFileSize
new9.19 KB

Here's one more version that restores the drupal_set_message(t('Your vote was registered.'));.

ezra-g’s picture

Status: Needs review » Needs work

Just noticed that we can also remove the "return $OK" as validation functions in D6 do not need to return anything.

ezra-g’s picture

Status: Needs work » Needs review
StatusFileSize
new11.33 KB

Here's a re-roll with the above change.

ezra-g’s picture

Status: Needs review » Fixed

This is now committed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.