the addition of the duplicate form submission checker has created a problem with confirm forms. the duplicate checker works by checking the hash of all submitted form values against the last submitted form values, and concludes that it's a duplicate submission if the vales match. unfortunately, the confirm form almost always has the same hash, even between different confirm forms. we'll need to use the #skip_duplicate_check attribute to work around this issue.

attached patch corrects.

CommentFileSizeAuthor
#4 duplicate_check_0.patch819 byteshunmonk
duplicate_check.patch767 byteshunmonk

Comments

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community
hunmonk’s picture

hunmonk commented

note that this needs to be applied to Drupal 5 as well

dries’s picture

dries commented

Can we add a code comment for that?

hunmonk’s picture

hunmonk commented
StatusFileSize
new duplicate_check_0.patch819 bytes

attached patch adds the necessary code notation.

dries’s picture

dries commented

Patch seems to work, however, I'm left wondering if this is the best configuration/workflow. The alternative is to explicitly enable duplication checking on a handful of forms. Most forms already do duplication checking as part of their validate hook. So maybe the solution is to explicitly turn on duplication checking for comments and node forms, and leave most of the other forms untouched? Same with the search module, is it not allowed to search the same keyword twice?

I'd love to discuss this some more ...

robertdouglass’s picture

robertdouglass commented

I believe this is the right workflow and it is according to how we designed the original patch.

Dries' example with the search form is not the right concept. You'd never load a specific instance of a search form and want to submit it more than once (by clicking the Search button twice before waiting for the page to load). Loading multiple instances of the search form, each of which get used to search for the same keyword... that's allowed and the duplicate checking shouldn't prevent it.

m3avrck’s picture

m3avrck commented

I concur.

This patch seems reasonable to me and should be applied to HEAD and 5.x (since form dup is in 5 as well). Searching is not broken as Robert points out.

Explicitly enable this checking on forms seems backwards--those that do that in validate can remove code and simplify things. If they need duplicates, for whatever reason, they can turn it off.

robertdouglass’s picture

robertdouglass commented

"maybe the solution is to explicitly turn on duplication checking for comments and node forms"

I think the default of including the duplicate check on every form unless otherwise specified is correct. This is a feature that all forms want by nature (even the confirm form, really), and very few will want to go around the check. Going around it for the confirm form is a hack to sidestep a limitation in the original patch. It is an acceptable hack, but the real fix would be to find the right way to do the duplicate check so that even the confirm form works correctly.

hunmonk’s picture

hunmonk commented

the only way around that hack for the confirm form is to have a truely unique form value, which we don't have. options would be:

  1. make form_token truely unique -- this seems like it could cause problems in other areas, and i think it would require a bit of rewriting.
  2. introduce another hidden form value that is truely unique -- i'd favor this approach as it would solve the problem in a simpler fashion.
robertdouglass’s picture

robertdouglass commented

@hunmonk: I agree that going back and finding a way to remove the need for this issue is warranted. Buth this patch should go in and we should open a different issue for that, I think. I still see this patch as RTBC and the right way forward from this point.

BioALIEN’s picture

BioALIEN commented

I agree with #7 and #8. This is probably frowned upon, but what about committing the patch to 5.x and then going back and eliminating the need for the patch in 6.x?

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

I've committed this to CVS HEAD and DRUPAL-5. Thanks for the additional feedback.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)