The next release of Mollom has a way for contrib modules to allow Mollom protection to be added to the modules' forms. One of the biggest requests we've gotten is to allow protection of Webform submissions. I've been working on a patch for Webform 2.x to enable this to happen and will be posting it here for users to test and hopefully get into the next stable release of Webform.

Comments

Status:Active» Needs review
StatusFileSize
new3.04 KB

Here's the latest code I had. WIll probably need some testing with latest versions of Mollom as well. We'll need to see about adding other things like a 'Report to Mollom' link on submissions on-site and in e-mails.

+++ webform_submissions.inc 15 Jan 2010 21:08:18 -0000
@@ -59,6 +59,10 @@ function webform_submission_insert($node
+  if (module_exists('mollom')) {
+    mollom_data_save('webform', $sid);
+  }

It's possible that we can skip this part, if we can make the automated saving work.

Powered by Dreditor.

I'm fairly sure this line of code is not correct and would cause PHP errors when Mollom is not available:

+  if (module_exists('webform')) {
+    mollom_data_delete('webform', $submission->sid);
+  }

The 2.x version of Webform is feature-frozen, but this might make an exception considering the demand and usefulness of the functionality. However we'll definitely need a 3.x version of this patch.

Assigned:Unassigned» sun
Status:Needs review» Reviewed & tested by the community
StatusFileSize
new3.02 KB

Simplified due to #647860: Simplify how we call mollom_set_data()

Works like a charm!

What is and where is 3.x? ;) HEAD?

Not that anyone tries out this patch... it depends on #647860: Simplify how we call mollom_set_data() - so if you want to test, you need to apply this patch to Webform module and aforementioned to Mollom module.

HEAD is 3.x (for Drupal 6) currently.

#647860: Simplify how we call mollom_set_data() has been committed, so this patch would and should work flawlessly now. A new official release of Mollom module is planned within the next 1-2 weeks or so.

Do you need or want or require a patch for HEAD first or could this follow in a follow-up? :)

I'd really like a patch to HEAD, since I've "officially" said that no new features are being added to 2.x, this is already a bit of an exception. I'm pretty sure we won't be including this change to the Drupal 5 branch though. ;-)

Status:Reviewed & tested by the community» Needs work

Everywhere in this patch that uses "form_key" needs to be improved so that it will work within fieldsets.

For example:

+        $forms[$form_id]['mapping'][$mapping] = 'submitted][' . $node->webform['components'][$component]['form_key'];

This won't work if the field is located within a fieldset. In the 3.x version we have the utility function webform_component_parent_keys(), but we'll have to do it manually in the 2.x branch.

Regarding this todo:

+    // @todo Is 'author_id' (uid) available for authenticated users, too?

I don't think uid is available from just the values in $form_state, though it would be a good candidate to include in the $form['details'] array, along with the NID and the SID. This would make mollom more lenient towards trusted user accounts right?

Version:6.x-2.x-dev»
Status:Needs work» Needs review
StatusFileSize
new6.37 KB

As it seems that 3.x is relatively stable already, I'd even consider to add this to 3.x only. People can upgrade, I think.

I had to move that utility function into webform.module, since we would have to load components.inc on any page containing a form (not just webforms).

Also added that 'uid' value to the submission details.

Feel free to ping me in Skype/IRC when porting Webform to D7. I've already prepared the changes in this patch, mainly according to #645374: Make entity ids available to form submit handlers

Status:Needs review» Needs work

Webform can be attached to any node type in 3.x; this query needs to be updated to select all webform-enabled types:

+  $result = db_query(db_rewrite_sql("SELECT n.nid FROM {node} n WHERE n.type = 'webform'"));

The component parent keys still aren't being taken into account when passing in the from, email, and subject values into mollom:

+          $forms[$form_id]['mapping']['post_title'] = 'submitted][' . $node->webform['components'][$component]['form_key'];

This set of changes is not necessary:

-  $form = array();
+  // Always provide the entity id in the same key as in the entity edit form.
+  $form['#tree'] = TRUE;
+  $form['details']['sid'] = array('#type' => 'value', '#value' => $submission->sid);
+
   $form['node'] = array('#type' => 'value', '#value' => $node);
   $form['submission'] = array('#type' => 'value', '#value' => $submission);

There's no need to pass through $submission->sid as a value when you already have the entire $submission object.

Status:Needs work» Needs review
StatusFileSize
new6.76 KB

Thanks for your review!

Fixed the first two issues in this patch.

I should have explained that last change a bit more instead of just linking to the Drupal core issue (#645374: Make entity ids available to form submit handlers)...

The same changes have been applied to Drupal core entity delete confirmation forms. That is, because it allows modules like Mollom to seamlessly integrate with all forms of an entity.

For example, in "add" or "edit" forms, we have:

<?php
  $form
['nid'] = array('#type' => 'value', '#value' => $node->nid);
?>

Therefore, modules like Mollom can integrate with the form and safely rely on $form_state['values']['nid'] being the resulting entity id - to store their data that is bound to the entity, but only affecting form submission.

Aforementioned patch for Drupal core also changed all entity delete confirmation forms to provide at least this entity id in the same form values location:

<?php
  $form
['nid'] = array('#type' => 'value', '#value' => $node->nid);
?>

This means that modules like Mollom can identically integrate with those forms to react on the deletion of an entity. In Mollom's case, the delete confirmation form will additionally be enhanced with so called "Feedback options" - because it is essential for Mollom that users are reporting all submissions that were able to slip through the validation.

As of now, this Webform integration with Mollom is actually incomplete for that reason, because there is no way to report a submission to Mollom (instead of just deleting it). However, I refrain to add this additional code burden to other modules and instead, trying to find a generic solution that works "once for all". ;)

I'm currently working on this in #717212: Remove "report to Mollom" links and integrate with entity delete confirmation forms instead

When this functionality will be available, then it just means to do a one-line change to webform_mollom_form_info():

<?php
   
'delete form' => '$form_id of webform submission delete confirmation form',
?>

Hm, okay that makes some sense, however why do we have 'sid' inside of 'details' and set #tree to true?

+  // Always provide the entity id in the same key as in the entity edit form.
+  $form['#tree'] = TRUE;
+  $form['details']['sid'] = array('#type' => 'value', '#value' => $submission->sid);

+++ webform.module 20 Feb 2010 21:48:41 -0000
@@ -2728,3 +2745,73 @@ function webform_views_api() {
+function webform_mollom_form_info() {
...
+    $form_id = 'webform_client_form_' . $node->nid;
+    $forms[$form_id] = array(
...
+      'mapping' => array(
+        'post_id' => 'details][sid',

That's because we expect it in the very same $form_state['values'] location as in the add/edit form, as defined in the mapping.

Of course, if 'sid' wasn't contained in 'details', then we wouldn't need the #tree. Not sure why those properties live in 'details' at all (historical reason?), but as long as that is the case, I've just resembled the location to match the mapping.

Powered by Dreditor.

Tried this just now -- seems to work fine on one of my dev sites for a webform with no fieldsets: Webform works as expected, Mollom seems to be performing the intercept as expected. (Also, at least on my system, can update from 2.x to 3.x using the patched module.)

Other than the usual warnings (maintainer is held harmless, etc.), is there any glaring reason we shouldn't use this on a production site? I'm starting to get webform spam and would like to nip it in the bud.

CLARIFYING: Test was against Sun's revised patch @ #12

Status:Needs review» Needs work

uh oh. quicksketch just tells me that there are sites with tens of thousands of webforms. oh boy. ;)

I gave this patch a review and I think it looks pretty good overall. If I were going to commit it I'd clean up the delete form so that we're not passing both 'nid' and the $node at the same time. It's fine that we'll make $form['details']['nid'] to hold it, but I think it's just inexplicable that we'd pass in the NID in two places like that.

However that's a minor issue compared to the potential performance impact of this patch. This patch does a node_load() for EVERY webform on the entire site, on EVERY page load. That's going to be a crushing performance penalty. Most sites have at least double-digit forms, sites with more than a hundred webforms are common, and it's definitely possible to have thousands. Doing 100+ node_load() calls on every page (or any page at all really) is not going to be acceptable. We'll need some way to provide a list of elements only when necessary, rather than blindly loading every webform on every page load.

Status:Needs work» Postponed

I'm going to postpone this issue until we've found a solution in #728718: hook_mollom_form_info() does not scale

Version:» 6.x-3.x-dev
Status:Postponed» Reviewed & tested by the community
StatusFileSize
new6.57 KB

Revised implementation, not only due to aforementioned issue, but also due to #717212: Remove "report to Mollom" links and integrate with entity delete confirmation forms instead :)

This requires that change in the delete confirmation form for submissions.

Also note that this patch can be forward-ported as is - perhaps it even applies cleanly (depends on how much changed in HEAD).

Is there anything that needs to be improved here? :)

Righto, this new implementation works with the latest dev snapshot of Mollom only. But then again, people downloading the latest dev snapshot of Webform will most likely also have the latest dev snapshot of Mollom.

We have to port it to Drupal 7 before I can commit it. I also am still not a big fan of moving webform_component_parent_keys() into webform.module, but now that we're only using that function when necessary, I don't think we need to move it all. But the bigger thing is the Drupal 7 patch, since the 3.x branch will be maintained in-sync between D6/7.

Status:Reviewed & tested by the community» Fixed
StatusFileSize
new7.18 KB
new7.29 KB

Here's a final set of changes. Now committed to the 3.x D6/7 branches.

- Rather than hard-coding "textfield", "textarea", and "textarea" in hook_mollom_form_info(), it's now a property, "spam_analysis" of those components provided by hook_webform_component_info(). This makes it so other modules can make analysis-enabled components.
- Moved webform_component_parent_keys() back into webform.components.inc, since it's only needed by hook_mollom_form_info() and not hook_mollom_form_list() (meaning we've probably already loaded that file anyway if we're needing the whole form's information).
- Adjusted the webform_submission_delete_form() so that it makes more sense to me (and less likely to accidentally break in the future), even though it's slightly less efficient than before.
- And did a minor adjustment for Drupal 7. I think the only thing was updating the query in hook_mollom_form_list() to use DBNG.

And thank you very much sun for putting together this great feature!

yay! Thanks, Nate! Your last-minute tweaks are great!

Will try to upgrade from 2.x on my own site + run the dev snapshot, finally with spam protection :)

Status:Fixed» Closed (fixed)
Issue tags:-Mollom

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