Enable use of Mollom for any form

IMHO the CAPTCHA module does take the Drupal Way in allowing every form to be CAPTCHA-scanned, not just the preset ones. This feature request seems like a good one to me as it allows Mollom to provide more complete security.

I think that currently this is the #1 reason preventing a more widespread Mollom use (along the 'allow another fallback strategy' issue). Any development on this?

Subscribing

+1.

The mollom module also suggests its as easy as adding a mollom_$form_id variable in the comments for mollom_validate. However even that functionality doesn't work. This is a fairly big bug if you use any other sort of form other than core drupal forms for content entry.

Check this patch for mollom + guestbook.
http://drupal.org/node/302941

@lismail I guess that just solidifies my point. We shouldn't have to patch mollom for every contrib module :-D

I like the way the CATPCHA module allows you to install the challenge on any form you have.

I would not move to Mollon unless this is also possible as it would leave many of my forms exposed to spam.

Ideally, we'd be able to add Mollom to all forms in Drupal. For this to happen it would be ideal, yet not strictly necessary, if:

1. Drupal had a form registry so that the Mollom module can retrieve a list of all the forms,
2. each form had a human-readable name that could be used in the UI (rather than the form_id) and
3. we associate each output format/filter with each form element so that the Mollom module can translate the input to XHMTL.

All that said, we can probably get away with a "less than ideal" solution like the CAPTCHA module uses.

Three days ago, we committed a reasonably big change to the Mollom module which should make it easier to add Mollom to more forms. I'm hopeful that we can add support for more, if not all form, to a future release of Mollom. If anyone wants to help with that, please do. I'm happy to provide direction, assistance and testing.

Just a suggestion: would the form store module take care of Point #1?

Updating the version. Let's try to fix this in CVS HEAD of 6.x first, then consider backporting it to Drupal 5.

I was thinking of just making mollom more configurable by using a hook. In _mollom_protectable_forms use:

$forms = module_invoke_all('mollom')

then implement "on-behalf" hooks for Drupal core modules. And then each module that wanted to expose their form to mollom could implement a hook_mollom.

Here's the patch...

slick
+1

This looks like a good step forward.

This would also require the module to prepare the data ... right now we use:

    $function = 'mollom_data_'. $form_id;
    if (function_exists($function)) {
      $data = $function($form_state['values']);
    }

... in the module but I wonder if that needs to be made more solid / integrated in the API. It seems like there should be a field 'data' that points to a data preparation function?

'data' => 'mymodule_mollom_data',

Thoughts?

I appreciate that D6 is the start right now, but I have a paying user who needs 5.x support now.

I've added the suggestion from Dries to the patch found in #13 as well as an addition of a drupal_alter so that modules can change what other modules defines.

drupal_alter is not available n 5.x; how necessary is it to this patch?

Well - it makes it easier to do modification of added form specifications which is good practice. As I understand a patch is first made against Drupal 6 and then backported to Drupal 5 where either drupal_alter could be removed or replaced with other code.

FWIW, this works for me in D6. I don't know about D5.

In settings.php

<?php
// Set a variable to tell Mollom that my form needs protection.
// The key is "mollom_" . $form_id
$conf['mollom_panels_comment_form'] = 2;
?>

Doing that enabled Mollom on a form, but how does the data in the fields of the form get mapped to the fields needed for Mollom to submit the data for analysis?

You need a function mollom_data_form_id($form_state) implemented to handle the data mapping.

Here's the code that I've been working on as a part of #412760: User interface brainstorming to enable Mollom for any form. Since this is going to require at least a few major underlying changes, I'll be making my own little local Mollom fork to post as diffs to the latest version.

What if we could integrate this with the validation api module? IT has the nice feature of being able to validate any form item in drupal.

http://drupal.org/project/validation_api

We could probably supply a validator hook? the users could set on the form items of their choosing.

Anyone want to weigh in on the validation api route?

My preference is to not require other modules if at all possible. Plus keep in mind that there are 5.x sites that use Mollom as well.

yes that's true, I was just looking closer at the two module's code. I don't THINK that validation API has the ability to add form elements, just works through the $form['validate'] hooks. It also seems focused around fields, while mollom deals with the form as a whole. People might want to check the validation api code though anyway.

I'd also prefer not to rely on other modules at this point. Let's bring the discussion back on topic. We can discuss the validation API module in another issue if desired.

Bringing the conversation back on topic, what's wrong with the original patch or the updated patch? I'm using this on several Drupal 5.x sites right now.

The hook is the way to go here.

I am working on integrating webform and mollom. Populist wrote a patch (#259488: Webform support) to do this. However it works by making mollom webform aware. The work done is great, but this is a slippery slop and isn't scalable for contrib modules.

Another issue I ran into, is that webform is can create multistep forms. Right now, _mollom_get_mode($form_id) works solely on the $form_id. It should also be passed the $form_state so the mollom protection can be conditional (i.e. show only on the last page). Perhaps as we declare a protectable form, we can declare a 'check' function in the protectable forms to run before simply passing back the $form[$form_id]['mode'].

so webform would declare:

$form['webform_57'] = array(
'name' => 'webform_57',
'mode' => MOLLOM_MODE_CAPTCHA,
'check callback' => 'webform_mollom_check'
'check arguments' => array(57, $form_state);
);

This would call webform_mollom_check and for example return MOLLOM_MODE_DISABLED unless we are on the last page.

I am proposing:

• Adding hook_mollom (#17) so contrib modules can declare to be protected.
• Adding $form_state to _mollom_get_mode
• Adding some kind of check capability to make sure that we can conditionally declare the form to be protected. In d6 we can use the drupal_alter I believe, but not in d5.
• Adding a default menu task for admin/settings/mollom so contrib modules with a more complicated UI can declare a tab. This is the case in the webform patch (there is mapping of fields)

I am working on a d5 site right now, so I don't want to rollout any patches. We should develop on d6 and then backport. But I wanted to start discussing some ideas.

And for convenience here is the hook patch for drupal5 (there was some formatting that prevented it from applying). I also removed the drupal_alter line.

Hi all,

I'm the maintainer of the CAPTCHA module and I made a proof of concept module to integrate the Mollom CAPTCHA with the CAPTCHA module and its admin UI workflow, so the Mollom CAPTCHA can be added to virtually any Drupal form.

Some notes:

• It's proof of concept in a separate module, with the sucky name "captcha_mollom_integration", but I think it should be part of mollom.module eventually.
• it only provides the Mollom CAPTCHA, not Mollom analysis, but again, it's only proof of concept
• it requires a custom branch of the CAPTCHA module, available at http://github.com/soxofaan/drupal-captcha-dev/tree/473002_custom_captcha... , but the difference with the cvs.drupal.org HEAD version is actually extremely small: only one line
  ;). If there is interest in captcha_mollom_integration, it's a no-brainer to merge it with cvs.drupal.org.
• there is still a lot to do (e.g. Mollom analysis, avoiding that page caching is unnecessarily disabled, etc), but I first wanted to release this proof of concept and collect some feedback before working on it further.

Subscribe.

Subscribing.

Subscribing

A huge +1 on Mollom addressing the "apply to any form" by integration with CAPTCHA. It would be nice for Mollom to provide integration the other way as well, by allowing different CAPTCHAs to be used when analysis fails. And CAPTCHA module should be able to implement support for Mollom generically enough that other services could potentially be substituted (or even fall back on, say, Akismet if Mollom is down).

I had no idea none of this worked, I kind of just assumed it did, until I found out the opposite about 8 minutes ago. Subscribing!

subscribing

subscribing

Subscribe.

I think this deserves to be critical.

@mcrittenden: I completely agree, but let's leave it at normal for now. I'm devoting the rest of today and tomorrow to actually getting this any-form integration code working. I'll be posting patches for review and screenshot of the new interface.

Excellent! I'll be glad to review when you have something ready.

Initial patch with most of the interface work done. You can select forms to be protected, select fields for analysis. Still bugs to work out, but it also shows up the CAPTCHA correctly on initial quick tests. There is also some extra stuff in the patch that's just me playing with how to work the admin interface for this.

Looking good so far. I like the simplicity of the UI. Haven't had a chance to do a functionality review so far. Hopefully soon.

Thanks for the work on this. I know a lot of people are pretty excited about this getting in there.

Subscribe.

Subscribing.

Subscribe.

Subscribe.

Yep - good stuff! Subscribed.

Subscribing

Will be making the second half push tomorrow through this weekend to get this accomplished. Stay tuned.

subscribe

Subscribed.

I absolutely love Mollom but we need to use Webform for our site-wide contact form! The CAPTCHA module provides a couple of unobtrusive possibilities, but I'd rather have NO captcha using Mollom's text analysis.

Hi Dave, any updates as regards the comment:
http://drupal.org/node/245682#comment-2020772

I have a couple of production sites being attacked by spam. Is the patch attached with the above referenced comment stable enough to be used on a production site??

Also, are there any special steps to be kept in mind while applying this path on a Windows machine??

@r_honey: No it's not ready yet. There's a few minor bugs and I need to update all the tests in the module to be sure I don't find any more.

Subscribing....

I would be waiting eagerly for the stable release.

subscribe

I would also be very interested in this.

Subscribing.

subscribing... I'll have a look at it later on.

Subscribing...

Going by the number of subscribe comments I see on this issue, and on various others, I would seriously like the Drupal team to enable "Post/Comment reply notifications" feature for some parts of Drupal.org (specifically forums/issues).

It might require capability for sending significant amount of mails daily (or even per hour), but it will atleast keep the nodes clean, and focused on the topic at hand.

subscribing

You are basically implementing a custom version of Form controller here.

The particular problem applies to all modules that want to attach something to forms, potentially to all forms, but in a user-configurable manner. The idea of Form controller module is to introduce a unique entry point for all modules to build a UI for doing so, and optionally configuring further options for the particular form alteration of the implementing module. In other words, "Form controller" could have also be named "form_alter_ui" module.

Advancing on that paradigm and abstracting it a bit more, then you can say that Field API in D7 does the same in green: Given a certain entity id, we attach either fields to the object, or form widgets to a form. Form controller does the same, but just not targeting the object behind the form, but the form itself. The "entity" is the form, and the "id" is the $form_id. Contrary to Field API, however, it is an extra conditional controller on top of a form alteration process that may attach itself to _all_ forms by default, depending on the use-case and default configuration values of the implementing module. As mentioned on its project page, Journal module is such a use-case that, by default, attaches itself to all forms on a site, but can be configured per form, whether it will be attached and whether it will be a required field or not. Extra benefit: In case Form controller is not installed, the implementing module just uses its default configuration (just like Mollom does now), so it is a totally optional dependency.

To attach its own UI to configure form attachments, Form controller basically follows the pattern we will introduce with #473268: D7UX: Put edit links on everything in D7, which you already know from Views' admin links. Here, the to-be-configured thing simply isn't a block, node, or view, but a form. Configuring the form means to enable/disable/configure all optional form attachments provided by implementing modules.

Yes, it is still D5 and didn't see much traction so far due to my other obligations. However, instead of implementing yet another custom approach here, I'd really like to see us joining forces to implement a solution that benefits a full range of other contributed modules. It definitely needs a rewrite with less nifty jQuery stuff (which was a bad idea anyway), but overall it shouldn't be hard, and I'd be very happy to grant CVS access to you, Dave, or others.

Please take this into consideration, thank you. You know where to find me. :)

tha_sun: Yeah, this entire approach is very hacky, having to build forms ourself and get fields, etc. Having a default simple interface with the option to be able to control any forms with a generalized solution is a very interesting and attractive approach instead of trying to re-invent stuff. Definitely want to look into this some more.

subscribe

subscribe

Hi Dave, can you specify a timeline in which the updated Mollom project would be made available??

If it would take time, can I request you to give me an idea for how to patch the Guestbook module for integration with Mollom. I have site guestbooks open for Anonymous commenting (that's why they are for, isn't it).

But they are being flooded by Spam heavily!!!

subscribe

Subscribing (interested in using Mollom with the feedback module)

subscribe

#65: nice explanation.

subscribing

subscribe

Hi Dave, it has been quite some time since any update for this issue. Is there any chance of getting this fixed in the near future??

Subscribing. Need to protect webforms.

Subscribing with all my heart. I badly need it for the Guestbook.

Subscribing...

Subscribing

I just tested this with a webform and it doesn't work. I get the error "An illegal choice has been detected. Please contact the site administrator." after selecting the webform field I want to base textual analysis on. I think this is caused by the fact that the checkbox values are like submitted[contact_us][last_name] and is treated as array? When I add debug print statements to form.inc, I see that the $elements['#value'] is structured like Array ( [submitted[contact_us] => Array ( [name] => submitted[contact_us][name] ) ) .

Other than that the patch looks like a good step in the right direction. Though perhaps some help text should be added to the "Fields to use for textual analysis" config option to explain what types of fields are good candidates.

Cheers,
Stella

I'm working on a clients site, who is using and more importantly paying for two mollom subscriptions. I've just found a email out form, which is custom written, and we believe is sending out spam. I was thinking, no problem, we've paid for mollom, it will fix this problem. Should be as easy as the captcha module. Right?...

Turns out, there's no easy way to do this. People have been talking for 20 months about adding feature support which is similar to captcha, but still as far as I can see, it still doesn't exist. It really shouldn't take more than a couple days to get it done. Perhaps the profit motive of allowing all the free subscriptions to increase the amount of captcha which are processed, is not allowing this to go through. I can understand that, but what about your paying customers?

This combined with the other issues I've run into on my first project with mollum have been very disappointing and it will be hard to me to recommend it to other paying clients. I've never had a problem with the Captcha module and it seems to get rid of spam pretty well.

So far the only benefit I've seen from mollom has been prettier and easier to read captchas. Hardly worth the couple hundred dollars paid to mollom.

</feet-to-fire-rant>

For those interested in adding mollom to any form programatically, it's pretty easy to do.

function yourmodule_form_alter(&$form, $form_state, $form_id) {
 switch($form_id) {
   case 'spammy_form':
     $form['my_spam_gollum'] = array(
     '#type' => 'mollom',
     '#mode' => MOLLUM_MODE_CAPTCHA, // or MOLLOM_MODE_DISABLED, MOLLOM_MODE_ANALYSIS
     '#weight' => 9999, // sink it to the bottom
     );
   break;
 }
}

Also the solution in #20 should work from the tracing of the code I did.

Or you could simply:
variable_set("mollom_FORMID", 2); // 0 is disabled, 1 is captcha and 2 is mode analysis.

It would be nice is these kind of developer snippets and information was made easily available in the Drupal Tutorials: http://mollom.com/tutorials/drupal

Tip on #43: if you apply it to a site with Mollom installed, you need to uninstall Mollom first, as the module needs to create an extra database table.

I tested it against Mollom 6.x-1.10, patch works, ignore the 1 reject.

Can't add any fields for textual analysis as I get "illegal choice detected". But it displays a captcha.

Any updates from the official Mollom maintainers?? People are having trouble out there man..

Subscribing

Yep we're still actively working on this. Keep posted and I'll try to post a revised patch around this weekend.

+++ mollom.admin.inc	9 Sep 2009 04:27:56 -0000
@@ -6,6 +6,215 @@
+function mollom_get_available_forms() {

Right now we have functions that are a bit confusing: mollom_get_available_forms(), mollom_get_forms() and mollom_get_protected_forms(). It was not immediately clear how mollom_get_forms() is different from the two other functions.

+++ mollom.admin.inc	9 Sep 2009 04:27:56 -0000
@@ -6,6 +6,215 @@
+    $forms = mollom_get_forms();
+    $query = db_query("SELECT fid FROM {mollom_form}");

- I had to dug around a bit to understand why mollom_get_forms() and the query returned different results. One is all the forms that could be protected with Mollom, the other one is all the forms that are actually protected by Mollom. We might be able to write that a bit more cleanly. Anyway, at first glance, this function was a bit funny.

- This could probably be simplified. There is currently no code inserting rows in the {mollom_form} table. We can probably leave that out for now, and revisit this in the context of form_controller module as a follow-up step?

+++ mollom.admin.inc	9 Sep 2009 04:27:56 -0000
@@ -6,6 +6,215 @@
+      l(t('Edit'), 'admin/build/mollom/protect/' . $form->fid),

I'd rename 'Edit' to 'Configure'. It is closer to configuration than to editing, IMO.

+++ mollom.admin.inc	9 Sep 2009 04:27:56 -0000
@@ -6,6 +6,215 @@
+  $rows[] = array(array('colspan' => 4, 'data' => l(t('Add form protection'), 'admin/build/mollom/protect')));

Add a @todo to use local tasks in Drupal 7.

+++ mollom.admin.inc	9 Sep 2009 04:27:56 -0000
@@ -6,6 +6,215 @@
+function mollom_autocomplete_form_ids($string = '') {
+  $forms = mollom_get_available_forms();

I'm not sure why we need the auto-complete. Doesn't mollom_get_available_forms() returns all forms that are explicitly made available through the new hook_form_info() using human-readable names? We could consider removing this for now, and revisiting this when we discuss form_controller support in a next step.

+++ mollom.admin.inc	9 Sep 2009 04:27:56 -0000
@@ -6,6 +6,215 @@
+function mollom_protect_form(&$form_state, $mollom_form = NULL) {
+  if (!isset($form_state['storage']['step'])) {
+    $form_state['storage']['step'] = $mollom_form ? 2 : 1;
+    $form_state['storage']['mollom_form'] = $mollom_form;
+  }
+  else {
+    $mollom_form = $form_state['storage']['mollom_form'];
+  }

Instead of using a multi-page form, it would be nicer if we dynamically refreshed the page using Javascript when selecting a form. The multi-step form experience always feels awkward to me.

+++ mollom.form.inc	9 Sep 2009 04:27:56 -0000
@@ -0,0 +1,98 @@
+function node_form_info() {
+  global $user;
+  $account = $user; //drupal_anonymous_user();
+
+  $node = array(
+    'uid' => $account->uid,
+    'name' => isset($account->name) ? $user->name : '',
+    'language' => '',
+    'vid' => 0,
+  );
+

The fact that you have to specify those parameters (e.g. $node) is somewhat unfortunate. I know it is required for drupal_retrieve_form() but maybe we can solve this differently so hook_form_info() can be a bit more simple/elegant. Making this API simple is valuable because it makes it easier for people to add support for their module(s).

+++ mollom.form.inc	9 Sep 2009 04:27:56 -0000
@@ -0,0 +1,98 @@
+      'parameters' => array(
+        node_load($webform->nid, NULL, TRUE),
+        array(),
+        TRUE,
+        FALSE,
+      ),

See above. Here as well, it would be nice if we could get rid of 'parameters'.

+++ mollom.form.inc	9 Sep 2009 04:27:56 -0000
@@ -0,0 +1,98 @@
+  $forms['comment_form'] = array(
+    'title' => t('Comment'),
+    'parameters' => array(
+      array('nid' => db_result(db_query_range(db_rewrite_sql("SELECT n.nid FROM {node} n WHERE n.status = 1"), 0, 1))),
+    ),

I'm not sure I understand the purpose of the SQL query.

+++ mollom.module	9 Sep 2009 04:27:56 -0000
@@ -94,6 +94,49 @@ function mollom_menu() {
+    'title' => 'Edit form protection',

Configure form protection?

+++ mollom.module	9 Sep 2009 04:27:56 -0000
@@ -123,6 +176,71 @@ function mollom_menu() {
+    $field = str_replace(']', '', $field);
+    $field = explode('[', $field);

This code looks unnecessary because we implode() the stuff away later. Can probably be simplified.

+++ mollom.module	9 Sep 2009 04:27:56 -0000
@@ -123,6 +176,71 @@ function mollom_menu() {
+      $value = isset($value[$key]) ? $value[$key] : NULL;

We need to convert the value to its final HMTL output. That is, if the value comes from a textarea with an input format, we want to run the filter before sending the data to Mollom. Mollom does not understand all possible input formats, and expect the final HTML output. Should be a lot easier in Drupal 7. Might be doable in Drupal 6. At a minimum, there should be a @todo for Drupal 7.

+++ mollom.module	9 Sep 2009 04:27:56 -0000
@@ -123,6 +176,71 @@ function mollom_menu() {
+  $data['post_title'] = NULL; // @todo Will this still be ok?

We should try to set 'post_title'.

+++ mollom.module	9 Sep 2009 04:27:56 -0000
@@ -123,6 +176,71 @@ function mollom_menu() {
+  $data['post_body'] = $fields_data ? implode("\n", $fields_data) : NULL;

The problem with this code is that it duplicates information in 'post_body'. If an 'author_mail' is provided, it is also set in 'post_body'. Not the end of the world, but it could negatively affect the classifier. It is better to exclude known fields from 'post_body'.

+++ mollom.module	9 Sep 2009 04:27:56 -0000
@@ -123,6 +176,71 @@ function mollom_menu() {
+  // Add common data.
+  $data['author_name'] = isset($values['name']) ? $values['name'] : (isset($user->name) ? $user->name : NULL);
+  $data['author_mail'] = isset($values['mail']) ? $values['mail'] : (isset($user->mail) ? $user->mail : NULL);
+  $data['author_url'] = isset($values['homepage']) ? $values['homepage'] : NULL;
+  $data['author_openid'] = $user->uid ? _mollom_get_openid($user) : NULL;
+  $data['author_id'] = $user->uid ? $user->uid : NULL;
+  $data['author_ip'] = ip_address();

This could be incorrect. $values['name'] is not guaranteed to be the 'author_name'. Not sure how to fix that. One possible solution is to manually maintain a mapping in hook_form_info().

Working on this.

First of all: Straight re-roll of Dave's patch (without some unrelated change to the admin settings form).

subscribing.

Hrm. This approach and the overall requirements are more complex than I thought. I spent the last couple of hours to entirely revamp this patch.

First of all, there are only two valid approaches to accomplish the goal:

1. Define available forms + available fields upfront
2. Don't define anything and try to automatically figure everything out. (which admittedly is a lot harder, especially after finding out about all the requirements for Mollom)

A mixture of both approaches will #fail. Just to make this clear.

Since we want to go with 1) and leave the generic approach for later, we have to properly define forms + fields in info hooks.

By providing a proper hook architecture (like D7 does in many areas now), all modules are able to hook into anything and enhance any field information provided by whatever else.

Attached patch implements this.

• Existing tests are almost (?) working. Didn't run all, but looks promising.
• New tests for the new functionality are required.
• Existing data retrieval functions for various supported forms still need to be removed.

aww. That's quite a patch. :)

Will continue tomorrow.

Thanks sun! Did a quick/initial review:

+++ mollom.admin.inc	20 Nov 2009 00:18:01 -0000
@@ -7,7 +7,226 @@
+      $form['mollom']['form_id_custom'] = array(
+        '#type' => 'textfield',
+        '#title' => t('Custom form ID'),
+        '#size' => 25,
+      );

I'm still not sure I fully grok the custom form ID support. It seems to be at odds with the _mollom_form_info() hook.

+++ mollom.api.php	20 Nov 2009 00:16:15 -0000
@@ -0,0 +1,58 @@
+ *   An associative array containing information about forms that can be
+ *   protected. Each key is a $form_id whose value is an associative array:
+ *   - title: The human-readable name of the form.
+ *   - mode: (optional) The default protection mode for the form. If omitted,
+ *     the form will not be configured upon installation of Mollom module.
+ *   - parents: (optional) An associative array of elements in the form that are
+ *     available and can be configured for Mollom's text analysis. Each key
+ *     forms the form API parents of the element in the form and the
+ *     corresponding value contains the form element label. For nested elements
+ *     a key of "parent][child" can be used. If omitted, Mollom can only provide
+ *     a CAPTCHA protection for the form.

- Yes, please. Not all forms can be protected with text analysis, so giving module authors the option to force a CAPTCHA makes total sense. For example, you can't protect the user registration form with text analysis. There just isn't enough data to analyze.

- I think it also worth noting that it allows you to exclude fields. Imagine a site that asked for a social security number or credit card number. They probably don't want to send that to Mollom.

+++ mollom.api.php	20 Nov 2009 00:16:15 -0000
@@ -0,0 +1,58 @@
+    'parents' => array(

I was wondering about the name 'parents'? I was expecting 'fields' or something. I haven't reviewed the entire patch yet, but the term 'parents' doesn't make sense yet. Maybe it will become clear further down.

+++ mollom.form.inc	20 Nov 2009 00:19:33 -0000
@@ -0,0 +1,118 @@
+      'message' => t('Message'),

Shouldn't 'message' be 'body'? Or do we map 'message' to mollom.postBody as well?

+++ mollom.form.inc	20 Nov 2009 00:19:33 -0000
@@ -0,0 +1,118 @@
+      'message' => t('Message'),

Same as above. Shouldn't 'message' be 'body'?

+++ mollom.form.inc	20 Nov 2009 00:19:33 -0000
@@ -0,0 +1,118 @@
+    if (module_exists('content')) {
+      $content_info = content_types($type->type);
+      foreach ($content_info['fields'] as $field_name => $field) {
+        $parents[$field_name] = check_plain(t($field['widget']['label']));
+      }

Not sure if we want all fields. We should probably limit it to text fields (i.e. textfields and textareas). We don't necessarily want dates, chexkboxes, radio buttons, etc.

This review is powered by Dreditor.

Alrighty.

- Removed support for custom form ids, because that indeed makes not really sense. (carried over from original patch)

- Reworked hook_mollom_form_info() documentation.

- Limited CCK fields to fields of text.module.

Clarification:

- For Contact module's forms, the posted message lives in a form element 'message' and that is the form element we want to process. All selected fields from the available form elements are processed and concatenated into 'post_body'. However, I now realize that a potential 'title' field will be merged into 'post_body' too, if selected. Hmmm. So we need to extend the form element mapping a bit. :-/

- The key 'parents' maps to form API's key '#parents'. I chose this key name, because we are using the same approach as form_set_error() to handle nested form elements. We cannot use 'fields', because we use that key to store the enabled fields already. I actually like to re-use existing concepts that developers should already know. If you find it totally awkward, then we could rename it to 'elements'.

Forgot to remove the JS for custom form ids as well.

- Re-rolled against HEAD.

- Fixed/removed some first @todos.

I installed the module on a Drupal 6 site from scratch (no upgrade) and I got: Fatal error: Call to undefined function mollom_get_form_info() in mollom.install on line 98. Haven't been able to debug that yet, so just reporting my error for now.

Yup, fixed now.

This one passes all existing tests.

This time for real. Had to borrow myself a testing server to resolve the test failures iteratively...

Desperately trying to find time to test this properly. Thanks SO MUCH for your work on this sun.

Making the installation defaults work properly.

- Renamed 'parents' to 'elements'.

- Introduced new 'mapping' to declare optional 'title', 'name', 'mail', and 'url' (homepage) elements in registered forms.

- Removed old data processing implementations.

- Fixed the entire implementation by adding a test to ensure data processing. Took some hours to figure out the proper shifting around of submitted form values in combination with configured fields, in combination with mapping certain fields to certain data properties and omitting them, and in combination with mapping all remaining form values to eventually further data properties. ;)

The only remaining @todo is:

// @todo How do we get invisible & dependent 'format' support in here?

+++ mollom.api.php	20 Nov 2009 22:11:53 -0000
@@ -0,0 +1,68 @@
+function hook_mollom_form_info() {

'mapping' is not documented.

+++ mollom.form.inc	21 Nov 2009 03:13:47 -0000
@@ -0,0 +1,148 @@
+    'mapping' => array(
+      'title' => 'subject',
+      'name' => 'name',
+      'mail' => 'mail',
+    ),

(and elsewhere) Keys need to be updated, now using Mollom's data structure keys.

+++ mollom.form.inc	21 Nov 2009 03:13:47 -0000
@@ -0,0 +1,148 @@
+      // @todo How do we get invisible & dependent 'format' support in here?
+      // $parents['format'] = t('Input format'),

Tinkering about this some more, I think this is obsolete. We do not want to _alter_ the user input in any way before spam detection, because that would potentially break Mollom's parser by either removing spam content (f.e. links) or adding content that could be considered as spam (f.e. inline macro tags that are processed into images containing 3rd party text contents and/or even links).

+++ mollom.install	20 Nov 2009 22:40:03 -0000
@@ -53,6 +53,36 @@ function mollom_schema() {
+      // @todo 'module' required for maintenance.

This would be nice, but technically not strictly required - until D7. Because, without hook_modules_uninstalled(), we have no trigger to delete configured forms. So all we could do here is preparation for D7...

+++ tests/mollom.test	21 Nov 2009 03:03:29 -0000
@@ -810,3 +899,93 @@ class MollomResellerTestCase extends Mol
+    $this->assertEqual($data['post_title'], $values['subject'], t("@key: '@data' is equal to '@value'.", array(
+      '@key' => 'post_title',
+      '@data' => $data['post_title'],
+      '@value' => $values['subject'],
+    )));

Add a @todo here to remove those ugly, lengthy $message strings when latest D7 code has been backported to D6 SimpleTest.

This review is powered by Dreditor.

Also, this:

Took some hours to figure out the proper shifting around of submitted form values in combination with configured fields, in combination with mapping certain fields to certain data properties and omitting them, and in combination with mapping all remaining form values to eventually further data properties.

...should be explained in PHPDoc of mollom_form_get_values().

When I upgraded an existing Drupal 6 site with Mollom module, I got some warnings:

    * notice: Undefined variable: forms in /Users/dries/Sites/mollom-6/mollom.form.inc on line 146.
    * warning: Invalid argument supplied for foreach() in /Users/dries/Sites/mollom-6/mollom.module on line 467.
    * warning: array_merge_recursive() [function.array-merge-recursive]: Argument #2 is not an array in /Users/dries/Sites/mollom-6/mollom.module on line 476.
    * warning: array_merge_recursive() [function.array-merge-recursive]: Argument #1 is not an array in /Users/dries/Sites/mollom-6/mollom.module on line 476.
    * warning: array_merge_recursive() [function.array-merge-recursive]: Argument #1 is not an array in /Users/dries/Sites/mollom-6/mollom.module on line 476.
    * warning: array_merge_recursive() [function.array-merge-recursive]: Argument #1 is not an array in /Users/dries/Sites/mollom-6/mollom.module on line 476.

Here is a screenshot of the errors.

With a clean install, I got the same errors. After installation, I can't configure any forms so I can't proceed with testing.

+++ mollom.admin.inc	20 Nov 2009 22:37:20 -0000
@@ -7,10 +7,227 @@
+  // Add a row to add a form.
+  if (empty($rows)) {
+    $rows[] = array(array('data' => l(t('Add form'), 'admin/settings/mollom/add'), 'colspan' => 4));
+  }

In Drupal 7 we should use a local task. If we do Drupal 7 todo's, this should be one.

+++ mollom.admin.inc	20 Nov 2009 22:37:20 -0000
@@ -7,10 +7,227 @@
+ * Helper function to return registered forms suitable for #options.

It is not clear what 'suitable for #options' means. I'm sure it will become clear as I read more code, but reading from top to bottom, it isn't. Could probably be clarified a bit better.

+++ mollom.admin.inc	20 Nov 2009 22:37:20 -0000
@@ -7,10 +7,227 @@
+function mollom_admin_unprotect_form(&$form_state, $mollom_form) {
+  $form['#tree'] = TRUE;
+  $form['form'] = array(
+    '#type' => 'item',
+    '#title' => t('Form'),
+    '#value' => $mollom_form['title'],
+  );
+  $form['mollom']['form_id'] = array(
+    '#type' => 'value',
+    '#value' => $mollom_form['form_id'],
+  );

It is unclear why we want to store/use anything else but the form_id here. Might become clear as I read more of the code. It is also unclear why we store it in $form['mollom']['form_id'] instead of $form['form_id'].

+++ mollom.admin.inc	20 Nov 2009 22:37:20 -0000
@@ -99,7 +274,7 @@ function mollom_admin_settings() {
-    '#collapsible' => TRUE,
+    '#collapsible' => $keys,

Now that the keys are on their own settings page, I'd remove '#collapsible' and '#collapsed'.

+++ mollom.admin.inc	20 Nov 2009 22:37:20 -0000
@@ -128,12 +303,15 @@ function _mollom_verify_key() {
+    return FALSE;

If we remove the '#collapsible' this might be unnecessary?

+++ mollom.api.php	20 Nov 2009 22:11:53 -0000
@@ -0,0 +1,68 @@
+/**
+ * @file
+ * API documentation for Mollom module.
+ */

- 'mapping' does not seem to be documented yet in mollom.api.php. What if something is not in elements but it is in mapping? And vice versa, what if something is in elements but not in mapping? When is something concatenated to the postBody? Some fields like author_ip don't seem to be specified at all. That is, they are outside of mapping and elements.

+++ mollom.form.inc	21 Nov 2009 03:13:47 -0000
@@ -0,0 +1,148 @@
+ * @todo Move this into Webform module.

We can probably leave it in mollom.module for now.

+++ mollom.form.inc	21 Nov 2009 03:13:47 -0000
@@ -0,0 +1,148 @@
+  // @todo

Stale @todo.

+++ mollom.module	21 Nov 2009 03:13:29 -0000
@@ -332,7 +392,9 @@ function mollom_form_alter(&$form, $form
+        '#mode' => $mollom_form['mode'],
+        '#fields' => $mollom_form['fields'],
+        '#mapping' => $mollom_form['mapping'],

It seems like sometimes we call this 'elements' and sometimes we call this 'fields'. I guess 'fields' might be a processed 'elements'?

+++ mollom.module	21 Nov 2009 03:13:29 -0000
@@ -373,156 +435,184 @@ function mollom_form_node_admin_content_
+  // Allow modules to react on saved form configurations.
+  if (isset($status) && $status) {
+    if ($status === SAVED_NEW) {
+      module_invoke_all('mollom_form_insert', $mollom_form);
+    }
+    elseif ($status === SAVED_UPDATED) {
+      module_invoke_all('mollom_form_update', $mollom_form);
+    }
+  }

I'm trying to think of a use case for this ... feature creep?

+++ mollom.module	21 Nov 2009 03:13:29 -0000
@@ -373,156 +435,184 @@ function mollom_form_node_admin_content_
+  // All optionally specified fields specified in $mapping must be excluded from
+  // $fields, because they are used in other data properties.

What is meant with 'used in other data properties'? Can you clarify in the comments?

+++ mollom.module	21 Nov 2009 03:13:29 -0000
@@ -373,156 +435,184 @@ function mollom_form_node_admin_content_
+  // Add user data.
+  $data += array(
+    'author_name' => isset($user->name) ? $user->name : NULL,
+    'author_mail' => isset($user->mail) ? $user->mail : NULL,
+    'author_url' => !empty($user->homepage) ? $user->homepage : NULL,
+    'author_openid' => $user->uid ? _mollom_get_openid($user) : NULL,
+    'author_id' => $user->uid ? $user->uid : NULL,
+    'author_ip' => ip_address(),
+  );

This looks like a problematic change. If a site editor edits a spam comment, we'd send the site editor's user name, user ID, IP address along with the spam comment (instead of the original spammer's information). This would harm the site editor's reputation. We can't just send global user information along with someone else's content.

+++ tests/mollom.test	21 Nov 2009 03:03:29 -0000
@@ -127,6 +127,78 @@ class MollomWebTestCase extends DrupalWe
+  protected function setProtection($form_id, $fields = NULL) {

- It looks like setProtection assumes that you are logged with the proper access rights.

- I was expecting setProtection() to give me control over the Mollom protection mode but that doesn't seem to be the case. That might be OK, just recording my expectation mismatch.

+++ tests/mollom.test	21 Nov 2009 03:03:29 -0000
@@ -127,6 +127,78 @@ class MollomWebTestCase extends DrupalWe
+      $this->clickLink(t('Add form'));

Maybe that should be called 'Protect new form'.

+++ tests/mollom.test	21 Nov 2009 03:03:29 -0000
@@ -127,6 +127,78 @@ class MollomWebTestCase extends DrupalWe
+  protected function delProtection($form_id) {

- Let's write deleteProtection() instead of delProtection().

- It looks like delProtection assumes that you are logged with the proper access rights.

+++ tests/mollom.test	21 Nov 2009 03:03:29 -0000
@@ -127,6 +127,78 @@ class MollomWebTestCase extends DrupalWe
+    if ($exists) {
+      $this->drupalGet('admin/settings/mollom/unprotect/' . $form_id);
+      $this->assertText(t('Mollom will no longer protect this form from spam.'), t('Unprotect confirmation form found.'));
+      $this->drupalPost(NULL, array(), t('Confirm'));
+    }

We don't actually test that the protection is remove. We only test the confirmation page.

+++ tests/mollom.test	21 Nov 2009 03:03:29 -0000
@@ -397,9 +469,10 @@ class MollomFallbackTestCase extends Mol
   function testFailoverMechanism() {
+    $this->drupalLogout();
     // Set the fallback strategy to 'blocking mode', so that if the failover
     // mechanism does not work, we would expect to get a warning.

Odd change. If that is intentional, please add a line of documentation.

+++ tests/mollom.test	21 Nov 2009 03:03:29 -0000
@@ -477,11 +549,13 @@ class MollomUserFormsTestCase extends Mo
+    $this->drupalLogin($this->admin_user);
+    $this->setProtection('user_pass');
+    $this->drupalLogout();

Given that this is a bit of a pattern, have you considered moving the login and logout functionality inside of setProtection()?

+++ tests/mollom.test	21 Nov 2009 03:03:29 -0000
@@ -810,3 +899,93 @@ class MollomResellerTestCase extends Mol
+    $this->assertEqual($data['author_id'], $user->uid, t("@key: '@data' is equal to '@value'.", array(
+      '@key' => 'author_id',
+      '@data' => $data['author_id'],
+      '@value' => $user->uid,
+    )));

See above. This assumption is only valid for new submissions, but not for editing existing submissions.

Fixed the installation/update errors. (Only happens with Webform module being installed, which I still did not try yet.)

Also fixed all other issues, which I won't comment on in the following...

In Drupal 7 we should use a local task.

It is one already. :) In D7, we actually do both: Local actions (just a 'type' change in hook_menu()), but in D7 we also output a table row like that, containing a link to add a form, in case there are no forms configured yet, so the table does not contain a table header only.

It is unclear why we want to store/use anything else but the form_id here. Might become clear as I read more of the code. It is also unclear why we store it in $form['mollom']['form_id'] instead of $form['form_id'].

1) We only store the 'form_id'. The other form element is of #type 'item', only to display the form's human-readable title.

2) We cannot use $form['form_id'], because that element is already used by form API internals. For this unprotect confirmation form, $form['form_id'] is 'mollom_admin_unprotect_form'.

Now that the keys are on their own settings page, I'd remove '#collapsible' and '#collapsed'.

I already took some time to optimize that page in terms of UX. The form still contains the fallback method selection, too. Since both the keys and the fallback method selection have lengthy descriptions, it makes sense to use fieldsets here. Initially, only the key configuration is visible, in an uncollapsible fieldset. When the keys are verified and valid, then also the fallback method fieldset is displayed, while the key configuration collapses itself.

That's similar to the previous behavior on that page. As of now, the only difference is that the key configuration fieldset is not collapsible initially. Not sure whether uncollapsing/displaying both of them all of the time is better.

If we remove the '#collapsible' this might be unnecessary?

I think that a function like _mollom_verify_key() should return the status either way.

+ * @todo Move this into Webform module.

We can probably leave it in mollom.module for now.

I know that you stated it multiple times already, but ideally, Webform should implement support for Mollom on its own, and Mollom should only implement support on behalf of Drupal core modules. That is, because http://drupal.org/project/webform exists in multiple versions, and contributed modules in general are free to alter their APIs, data structures, and form structures at any time.

Since Drupal's hook system only understands modules, but not module versions, Mollom would never know whether Webform's APIs may have changed, and when they change... fatal errors will be the result. For one, Mollom couldn't provide the same hook implemenation for different versions of Webform. Second, moving the hook to Webform module later on would be a pain of fatal errors for all users, because, unless users update both modules to new releases that contain the hook movement at the same time, the same function will be declared twice.

I have a lot of experience in this area, mainly because I did this mistake in a few of my modules already.

I'm pretty sure that quicksketch will be fine + happy to include this hook implementation.

It seems like sometimes we call this 'elements' and sometimes we call this 'fields'. I guess 'fields' might be a processed 'elements'?

'elements' was 'parents' before. We need different properties here, because one of them ('elements') holds the generally available form elements in a certain form, as registered via hook_mollom_form_info(). The other one ('fields') is our own property and holds the keys of the 'elements' that have been selected for textual analysis (if any). That is different information, so we need and want to use different properties.

Note that only 'fields' are stored in the database, because 'elements' are supplied by the hook implementation, and can certainly change over time (f.e. by adding a new CCK field).

I'm trying to think of a use case for this ... feature creep?

It's hard to think of something, but let's design this API properly. Saves all of us from pain and work in the future.

This looks like a problematic change. If a site editor edits a spam comment, we'd send the site editor's user name, user ID, IP address along with the spam comment (instead of the original spammer's information). This would harm the site editor's reputation. We can't just send global user information along with someone else's content.

Yes, I agree. But to be fair - that's not invented here:

-function mollom_data_comment_form($form_state) {
-  global $user;

-  $data = array(
-    'post_title'    => isset($form_state['subject']) ? $form_state['subject'] : NULL,
-    'post_body'     => isset($form_state['comment']) ? $form_state['comment'] : NULL,
-    'author_name'   => isset($form_state['name']) ? $form_state['name'] : (isset($user->name) ? $user->name : NULL),
-    'author_mail'   => isset($form_state['mail']) ? $form_state['mail'] : (isset($user->mail) ? $user->mail : NULL),
-    'author_url'    => isset($form_state['homepage']) ? $form_state['homepage'] : NULL,
-    'author_openid' => $user->uid ? _mollom_get_openid($user) : NULL,
-    'author_id'     => $user->uid ? $user->uid : NULL,
-    'author_ip'     => isset($form_state['cid']) ? NULL : ip_address(),
-  );

I would have ideas to resolve this, but that definitely belongs into a separate issue.

- It looks like setProtection assumes that you are logged with the proper access rights.
- It looks like delProtection assumes that you are logged with the proper access rights.
Odd change. If that is intentional, please add a line of documentation.
Given that this is a bit of a pattern, have you considered moving the login and logout functionality inside of setProtection()?

Yes, I considered to put drupalLogin() + drupalLogout() directly into the setProtection() and delProtection() first, and actually had the code like that before. However, after diving into mollom.test a bit more, I quickly figured that we could and should definitely add more centralized helper methods to perform administrative tasks that are common to more than one test. If login + logout would be contained in the helper methods, then we would needlessly login + logout + login + logout all over the time... which is why I figured that we can simply turn it around and let the caller ensure that we have administrative permissions before trying to perform any administrative actions.

That single "odd change" was caused by the structure of the testcase, but I streamlined that now.

All tests pass.

Not sure what happened to my brain, but the @todo I added in the previous patch was nonsense. Sorry.

meh, and I forgot to do the changes to the tests you mentioned, sorry.

The tests I had written initially had a similar setProtection() but doesn't use any interface drupalGet, etc. Just messes with variables/db directly. If we want to test the interface, we can set up a specific test for that, but doing things directly helps keep the test quick and from unnecessary assertions.

I need to dig into this a bit more but the old code tries to use the form values (i.e. $form_state['name']) when available. So when editing a form from another user, we wouldn't use the global $user object:

-    'author_name'   => isset($form_state['name']) ? $form_state['name'] : (isset($user->name) ? $user->name : NULL),
-    'author_mail'   => isset($form_state['mail']) ? $form_state['mail'] : (isset($user->mail) ? $user->mail : NULL),

At first glance, the new code always uses the global $user object and discards existing form values when editing something:

+++ mollom.module	21 Nov 2009 19:23:08 -0000
@@ -373,156 +437,224 @@ function mollom_form_node_admin_content_
+  $data += array(
+    'author_name' => isset($user->name) ? $user->name : NULL,
+    'author_mail' => isset($user->mail) ? $user->mail : NULL,

Ditto with ip_address(). In the old code we pass NULL in case we're editing an existing form:

-    'author_ip'     => isset($form_state['nid']) ? NULL : ip_address(),

In the new code we always pass ip_address():

+    'author_ip' => ip_address(),

In other words, it looks like a regression introduced by this patch. As I said, I need to dig a bit deeper into the code to make sure I'm not overlooking something, but at first glance it looks like a regression that should be tackled in this patch.

As far as 'name', 'mail', etc. are concerned, the new code does the same as the old, due to:

  $data = array();
  // Add the post body.
  $data += array(
  // Add data of mapped fields.
  $data += $mapping;
  // Add user data.
  $data += array(

...as long as there is a field mapping. Otherwise, it falls back to $user, just like the old code did.

I think the entire fallback on $user might be wrong. Instead, we probably want to ensure that the form contains at least #type => value elements for data mapping purposes.

But then there is also the special case of author_ip... that's a bit hard. I currently can't think of any logic that could be applied to all forms. If some piece of content was posted by an anonymous user, then the only place where the IP is stored is in {sessions}, but {sessions} is cleaned over time, and well, we wouldn't have any way to get from uid 0 to the visitor's session id at a later time. (note, that's basically what http://drupal.org/project/visitor is about)

As far as 'name', 'mail', etc. are concerned, the new code does the same as the old, as long as there is a field mapping.

Ah, in that case the mappings are still incomplete compared to what we have in DRUPAL-6 HEAD now. User IDs, OpenIDs and IPs are not properly mapped yet. User IDs, Open IDs and IPs provide critical information for Mollom -- it is more important than some other fields.

The problem only occurs when editing content. We could make the Mollom API and the backend such that when editing existing content, Mollom does not need to be passed all the fields. When a field is omitted, Mollom could maintain the old value of the field in the backend (i.e. without overwriting what was previously submitted). For example, unless you specify a new author IP when editing a comment, Mollom would maintain the old author IP on the backend. Of course, we need to send those updates with the proper Mollom session ID -- which we used to do, and hopefully still do. We'll want to double check -- I believe we have tests for that.

That would still require some changes to the patch because currently we're sending the wrong IP address instead of sending no IP address (or an empty IP address). However, it simplifies the problem. We'd need to figure out if we're editing a previously submitted entity (existing comment/node ID + existing Mollom ID), or whether creating a new entity (no comment/node ID yet + no Mollom ID yet). In case of the latter, we can probably rely on the global $user object. In case of the former, we should simply omit the fields that can't be modified through the UI.

It would be good if we had an assertXmlrpc() test function similar to the assertMail() test function. It is important to continuously verify that this all works correctly. Being able to inspect the actual XML-RPC messages would be invaluable to achieve this. I've created an issue for that at #639608: Provide assertXmlrpc() function to inspect and verify XML-RPC messages.

Ben correctly pointed out that sometimes, you do want the behavior in this patch; e.g. on a wiki page. sun, I'll want to thinker about this some more with Ben. Hold on. :)

It would be an option for the Mollom-backend to preserve data in an existing session, and it would be technically feasible. This would however create the idea that Mollom always stores all session information for an indefinite time. This is both due to technical and privacy reasons not possible.

When giving some though on the possible edit-scenarios, these are the possibilities:

1. spammer posts ham content, later edits it to spam
2. spammer posts mild spam content (unsure) by filling in captcha, later edits it to hard-core spam
3. good user posts ham content, spammy 'editor' turns it into spam (eg. a wiki page)
4. spammer posts unsure content (thanks to filling in the captcha), a legitimate editor tries to edit the content and does not expect to be asked again to fill in a captcha.

There as some things Mollom should do to take care of these scenarios:

• Mollom has to recheck content when it is edited (for scenario 1)
• Mollom can only temporarily (or not at all) remember that a captcha was solved to accept unsure content (for scenario 2)
• The credentials of the 'editor' need to be taken into account (for scenario 3)

The last scenario is toughest. Ideally the legitimate editor would, or, have an role which does not need Mollom checking, or, edit the content so it is not spammy anymore. Preserving the author information from the original post kind of conflicts with the other scenarios.

Food for thought...

Just quickly posting my gut reaction. This means that we probably want

a) each form to specify a pointer to another potentially existing submitted form value (f.e. 'nid') to determine whether we're dealing with new or existing content.

b) based on a), we want to store the session data (as currently being done for nodes and comments) in {mollom}, and send this session data in case it exists.

c) for the (rather unusual) editing case of a wiki page, we want the form definition to be able to specify that each submission must be validated ('always validate' => TRUE).

d) to copy/move the user permission to post without validation into a user account setting, so it can be applied to individual, trustworthy users.

subscribing

Looking forward to it. Subscribing!

The currently released version of the module does resubmit content to Mollom once it is edited. If the current patch does the same (it should or we have a regression), I think behavior of resubmitting the content using the global $user object is OK. It would work for people editing their own content, and it would work when people are making edits to a wiki page.

The big question is what happens when an administrator upublishes some spam -- if a status change submits the content to Mollom, then using the global $user object is problematic. Mollom would think that user has just submitted some spam.

One way to work around that is to extend _mollom_form_info() to return a 'access' attribute. The 'access' attribute would be an array of permissions. If the users has one or more of the permissions set, we'd skip Mollom checking. Thus, we could instruct the module to by-pass permission checking for users with the 'administer comments' permission, or for users with the 'administer nodes' permissions. Like that, we'd never send something to Mollom when edited by an administrator.

Either way, when a non-administrator edits a post, we should resubmit the content with the matching Mollom session ID. The current module does this, and this patch can't break that functionality.

Thoughts?

- Removed 'mapping' column from {mollom_form}. Re-install required, if previous patches were tested.

- Added XML-RPC server communication tests through a fake Mollom server implementation.

- Added more tests.

And 3 assertions of the new tests are failing. That is, because Drupal core's Comment module sucks. I need to revamp Mollom's form validation process within this patch to properly support the dynamically defined form elements in comment_form().

Hence, this patch is just for reference.

Created #642702: Form validation handlers cannot alter $form structure

Posting something in between. I really wonder why Mollom doesn't use $form_state['storage'] to trigger automated caching + rebuilds of forms until validation passed?

Aside from that, I can confirm that the simple fix in #642702: Form validation handlers cannot alter $form structure eliminates a lot of trouble, so we should at least get that into D7.

Getting closer.

Don't try this at home.

Next one: #644150: $form_state is stateless, even with enabled caching

Wow, that is a big rewrite. I thought we were relatively close so I'm curious what made you rewrite large parts of the form handling. Here are my initial comments after a first skim.

+++ mollom.api.php	21 Nov 2009 17:55:44 -0000
@@ -0,0 +1,103 @@
+  // Mymodule's user registration form.
+  $forms['mymodule_user_register'] = array(
+    'title' => t('User registration form'),
+    'mode' => MOLLOM_MODE_CAPTCHA,
+    'mapping' => array(
+      'author_name' => 'name',
+      'author_mail' => 'mail',
+    ),
+  );

When you use a CAPTCHA, you don't need to send Mollom any information. That said, it is OK to send information.

It might be worth mentioning that some data like author IP, user ID are also sent regardless of the presence of a mapping (unless I'm mistaken).

+++ mollom.form.inc	25 Nov 2009 13:53:11 -0000
@@ -0,0 +1,153 @@
+      // @todo comment_form() dynamically uses different form elements for
+      //   anonymous and authenticated users. This makes it impossible for
+      //   Mollom to define a valid mapping. Drupal core sucks again?

Wow, major WTF. I didn't know this was the case. Mmm, I wonder if that triggers a bug in the current version of the Mollom module. Good catch.

+++ mollom.module	26 Nov 2009 17:53:58 -0000
@@ -332,12 +399,60 @@ function mollom_form_alter(&$form, $form
+  echo "<pre>"; var_dump('form_alter'); echo "</pre>\n";
+  krumo($form_state);

Keekaboo! :)

+++ mollom.module	26 Nov 2009 17:53:58 -0000
@@ -332,12 +399,60 @@ function mollom_form_alter(&$form, $form
+          '#session_id' => NULL,
+          '#form_id' => $form_id,
+          '#require_analysis' => $mollom_form['mode'] == MOLLOM_MODE_ANALYSIS,
+          '#require_captcha' => $mollom_form['mode'] == MOLLOM_MODE_CAPTCHA,
+          '#passed_captcha' => FALSE,
+          '#user_session_id' => session_id(),

#session_id is #mollom_session_id?

+++ mollom.module	26 Nov 2009 17:53:58 -0000
@@ -567,47 +744,99 @@ function mollom_elements() {
+ * Format the Mollom form element.
+ */
+function theme_mollom($element) {
+  return isset($element['#children']) ? $element['#children'] : '';
+}

Eh? Odd.

+++ mollom.module	26 Nov 2009 17:53:58 -0000
@@ -567,47 +744,99 @@ function mollom_elements() {
+  echo "<pre>"; var_dump(__FUNCTION__); echo "</pre>\n";
+  krumo($form_state['storage']['mollom']);

keekaboo!

+++ tests/mollom_test.info	25 Nov 2009 11:16:09 -0000
@@ -0,0 +1,5 @@
+name = Mollom Test

'Mollom test'?

Next one: #644222: Some forms are incompatible with form caching *cough*...Mollom invalidates Form API...

Since all of those core bugs won't be fixed anytime soon in D6, I now need to post another fallback patch, before revamping it once more to work around the bugs.

However, when porting Mollom to D7, this patch should give some clues about how it should work, i.e. leveraging proper form caching, proper form storage, proper form validation, etc. :P

Alrighty, after working a couple of days on this, I finally made it work (and finally crossed the 100K mark :P). Contains big phat block of @defgroup that (hopefully) explains all the nightmare.

Ten minutes ago, all tests were passing. However, I suddenly get 3 fails in fallback mechanism tests, which I can't explain (perhaps my host got blocked due to massive test requests? ;-)

Also note that some minor and one major review issues raised above still need to be incorporated.

The major one is to add 'entity_id' => 'parent][child' to the form information returned by hook_mollom_form_info(). Given that additional mapping, we can trigger the sending of an IP address. ...although I'm no longer sure whether it is still wanted to not send an IP address when editing stuff...?

Next one: #644648: comment_form() structure is inconsistent / $form['#token'] WTF

Alright, my testing server indeed seems to be blocked or may have exceeded the limit of submissions or whatever...

To work around this, I've additionally implemented mollom.getImageCaptcha and mollom.checkCaptcha XML-RPC methods into the fake testing server, which makes the previously failing fallback mechanism test pass.

One of the remaining issues is the question whether we want/need to fully restore the previous behavior regarding 'author_ip', which was only populated with the current user's IP address for _new_ posts, i.e. not having a nid, cid, etc. If it is absolutely required, then I would suggest to add a new 'post_id' key to the existing mapping fields. Thus, for example, the form definition for the node form would specify 'nid' here. And depending on whether this mapping key exists, and the value for the specified form element is non-empty, we would not send the current user's IP address.

Since hook_mollom_form_info() is invoked for all protected forms and all users, the form definition would even be able to conditionally define the additional mapping. E.g., only return it in the definition, if the current user does not have the 'administer comments' permission.

That, however, also triggers the question whether any data should be sent and validated for content administrators at all...?

...

Partially related to that is: Currently, the module implements hook_nodeapi() and hook_comment() to store the (successful) validation result received from Mollom, which is later on used for administrative "Report to Mollom" links. It needs to do that, because form submit handlers do not have access to the inserted or updated content object from the database. However, this means that for each form and module that should be protected, there also needs to be a corresponding mollom_$object_insert() implementation... Without the stored information, "Report to Mollom" is not possible.
I'm entirely not sure how this could be solved.

Summary of Drupal core issues, which badly need support ASAP, because some of them are not back-portable:

#642702: Form validation handlers cannot alter $form structure
#644150: $form_state is stateless, even with enabled caching
#644222: Some forms are incompatible with form caching
#644648: comment_form() structure is inconsistent / $form['#token'] WTF

I've already submitted first or even already complete patches to most of them. Please help to get them in.

But also, though less critical:

#639608: Provide assertXmlrpc() function to inspect and verify XML-RPC messages

EDIT: Completing the list, from this issue, but also others:

#648170: Form constructors cannot enable form caching or form rebuilding
#645374: Make entity ids available to form submit handlers
#652394: Aggressive watchdog message assertion
#673974: PHP notice when mass-unpublishing or deleting comments, and wrong form validation redirect
#347959: modules_installed is broken during testing

One of the remaining issues is the question whether we want/need to fully restore the previous behavior regarding 'author_ip', which was only populated with the current user's IP address for _new_ posts, i.e. not having a nid, cid, etc.

I think we want to implement it as discussed in #121: the big question is what happens when an administrator upublishes some spam -- if a status change submits the content to Mollom, then using the global $user object is problematic. To work around that is to extend _mollom_form_info() to return a 'access' attribute. The 'access' attribute would be an array of permissions. If the users has one or more of the permissions set, we'd skip Mollom checking. Thus, we could instruct the module to by-pass permission checking for users with the 'administer comments' permission though comment_mollom_form_info(), or for users with the 'administer nodes' permissions though node_mollom_form_info(). Like that, we'd never send something to Mollom when edited by an administrator, and we would send things to Mollom if a non-administrator user has edit own content rights. Sounds do-able?

Partially related to that is: Currently, the module implements hook_nodeapi() and hook_comment() to store the (successful) validation result received from Mollom, which is later on used for administrative "Report to Mollom" links. It needs to do that, because form submit handlers do not have access to the inserted or updated content object from the database. However, this means that for each form and module that should be protected, there also needs to be a corresponding mollom_$object_insert() implementation... Without the stored information, "Report to Mollom" is not possible.

We store the Mollom session ID for two reasons: (i) for the administrative "Report to Mollom" links but also (ii) for when the content is edited. When content is edited, we need to send the session ID to Mollom so Mollom understands the old session was edited. Can you confirm that (ii) still works?

I don't think we can avoid the mollom_$object_insert() implementation unless Form APIs _submit handlers would be required to capture that data. I think it might be worth adding a sentence to mollom.api.php with regard to the need for a mollom_$object_insert() function.

Next one: #645374: Make entity ids available to form submit handlers

I'm reviewing all of the patches mentioned in #245682-133: Enable use of Mollom for any form to help push things forward. It is obvious that the Form API needs some tough love in Drupal 8.

Because #645374: Make entity ids available to form submit handlers (mentioned above) was committed, we can simplify how mollom_set_data() gets called in the Drupal 7 version of the module. Because it is somewhat off-topic in this issue, I'm breaking that out to a new issue: #647860: Simplify how we call mollom_set_data().

@sun, wondering if you had any follow-up to #134 or #135? Thanks!

Attached patch additionally implements a "bypass access" property for registered forms, as discussed above. Including tests.

Re: #135

Can you confirm that (ii) still works?

Negative. mollom_get_data() isn't called anywhere during form altering/processing, also not without this pach. It's only called by functions in mollom.pages.inc, which are only about "report to Mollom" functionality.

Of course, the new bypass access handling invalidated a few assertions I added for data processing for administrative editing of posts. Only commented out for now, because I believe we likely want to uncomment this code at some point (and perhaps just test with the same user or something like that - separate issue).

Remaining issues:

- hook_mollom_form_info() needs to allow to specify 'post_id' form value mapping, i.e. 'post_id' => 'nid'. If defined, Mollom module takes over storage and retrieval of session data.

- mollom_set_data() should store session info by $form_id. Upgrade path required.

- Helper function for mollom_set_data() for other modules required, so they do not have to fiddle with Mollom module's internals. Something like mollom_data_save($form_id, $id); Likewise, mollom_data_load($form_id, $id) will be required so that other modules ..... hrrrrm. At least the loading sounds like it has to be automated, resp. inlined into mollom_form_load(). Note that the latter considerations are rather off-topic for this issue/patch, rather related to session storage/retrieval when editing posts without bypass access.

- "Report to Mollom" functionality needs to be centralized; unique page callback taking $form_id and $id, leverages mollom_load_data(), displays form, invokes callback to delete on submit. Hence, hook_mollom_form_info() also needs to specify a 'delete callback' taking an $id (nid, cid, etc).

- mollom.form.inc makes no sense. Although we're not talking about hook implementations, but rather hook implementations on behalf of other (core) modules, we should follow D7's paradigm, and put the support code for Node module (required module) into mollom.module, while the support code for Comment and Contact (both optional) live in mollom.comment.inc and mollom.contact.inc. Those includes we conditionally load in mollom_init().

- setProtection() and delProtection() still need minor tweaks.

- Lastly, the current organization of code doesn't make sense for me. I'll move around a couple of functions and add corresponding @defgroups.

Implemented the previous points, except "report to Mollom" centralization.

Fixed installation and tests.

errr. Something went totally wrong in my patch numbering, so I need a stub comment now. :P Sorry.

New patch with new "Report to Mollom" functionality. Not including tests, but a first one will follow. (further ones should really live in an own issue, because there already were no tests before)

Completed. However, still 2 failing tests in MollomAccessChecking

Holy cow! I had to invent another awesomeness to figure out the cause for those 2 fails. Core worthy.

All tests pass, and I'm quite happy with this patch now.

Turned that into a core patch: #652394: Aggressive watchdog message assertion

Excellent, thanks a ton sun. Committed to DRUPAL-6 HEAD.

Upgrade had an error with db_change_field expecting the old and new column names.

Adding protection to a form results in no checkboxes to select and the Form: label with no form name.
warning: Invalid argument supplied for foreach() in /home/dave/Projects/www/drupal6dev/includes/form.inc on line 1206.

For a new form and adding protection mollom_form_load() returns FALSE.

Committed #151 as it fixed the upgrade for me, but still problems with adding forms and how to change mollom_form_load so it works properly with defaults.

sun, very awesome work on this.

@Dave, did the upgrade path work flawlessly? How carefully did you test it?

Yes. I restored the previous db and ran the same upgrade with the patch and it worked. db_change_field() is defined with &$ret, $table, $field, $field_new, $spec, $new_keys = array(). It was missing the $field_new parameter.

Also get a bunch of these watchdog messages when testing functionality:

# All Mollom servers were unavailable: Array ( [0] => http://174.37.205.152 [1] => http://88.151.243.81 [2] => http://88.151.243.145 ) , last error: 1000 - XMLRPC parse problem. The SAX parser returned: The specified field 'title' is not valid.
# Error 1000 from http://174.37.205.152: XMLRPC parse problem. The SAX parser returned: The specified field 'title' is not valid. for method mollom.checkContent:

Array
(
    [post_body] => <p>unsure</p>
    [author_name] => 
    [title] => test
    [author_mail] => 
    [author_url] => 
    [author_id] => 
    [author_ip] => 127.0.0.1
)

This should fix all the bugs. Sorry.

errr, plus that data mapping fix.

And fixing the tests, omg. :)

1.

+++ tests/mollom.test	6 Dec 2009 22:25:46 -0000
@@ -460,8 +460,7 @@ class MollomAccessTestCase extends Mollo
-    $this->drupalGet('admin/settings/mollom/settings');
+    $this->assertMollomWatchdogMessages(FALSE);
     $this->assertRaw(t('"messages error"'), t('The Mollom settings page reports that the Mollom keys are invalid.'));
   }

This looks like a strange change. Maybe we just need to move the assertRaw() up?

2.

+++ tests/mollom.test	6 Dec 2009 22:25:46 -0000
@@ -675,6 +674,102 @@ class MollomServerListRecoveryTestCase e
+    // @todo During Drupal installation, modules are installed in an arbitrary
+    //   order, so module_list() only returns modules that were installed before
+    //   this module. They seem to be ordered alphabetically, so registered
+    //   forms for Contact and Node modules are not contained yet. We'd either
+    //   have to install Mollom module manually, or we simply comment out these
+    //   assertions until D7. :P

If they are ordered alphabetically, you'd expect contact module to be available, but not node module. Is your sort broken, or am I confused?

3. In #157, Dave's bug report shows that a title wasn't properly mapped to post_title. I don't see that fixed in #160.

1) The order of assertions is unimportant. What matters is the negated assertion of watchdog messages, because this test verifies the behavior of a invalid server keys. However, you are right in that this other assertion (not invented here) looks a bit strange and needs to be improved.

2) Neither is available, only Comment and User module's forms are contained. For whatever reason. Perhaps the order is really totally arbitrary.

3) I'm not sure whether this is fixed with this patch, but I hope so. At least the data processing unit tests are returning the proper data. To ensure that even more, I've added another set of values.

Thanks sun. Committed to DRUPAL-6 HEAD. Would be great if Dave could try to reproduce his problem?

Adding/unprotecting forms works very well now. Didn't experience any errors. When I went to create a node type that had audio and PDF file fields, I got a couple of errors (only title and node were selected for analysis) in addition to a veeeeeeeery long CAPTCHA field.

The Mollom watchdog message for this was:

Unsure: Unsure Array
Session: 0912070ba76c26655c

So a major problem now when I was writing an implementation for webform to test this out. Using the doc for hook_mollom_form_info(), webform provides it's fields in $form_state['values']['submitted']['field_name']. So I did:

$elements = array(
'submitted][field1' => 'Field 1',
'submitted][feidl2' => 'Field 2',
);

However, when trying to add/save protection on the webform I get the error 'An illegal choice was detected' because the form creates form checkboxes like <input name="mollom[fields][submitted][name]" id="edit-mollom-fields-submitted-name" value="submitted][name" class="form-checkbox" type="checkbox"> which messes with what Drupal expects as valid options. Maybe we need to reconsider changing the parent/child delimiter.

Argh. :) I'll think about this.

One important question is whether this only affects Mollom's configuration form. I guess the actual protection of a webform should work once Mollom's configuration was saved? Can you try with mollom_form_save() ?

mollom_form_save(array(
  'form_id' => '...',
  'mode' => MOLLOM_MODE_ANALYSIS,
  'fields' => array(
    'submitted][field1',
    'submitted][field2',
  ),
  'module' => 'webform',
));

Yes it does appear to work if I enter it manually. I did get a couple of errors:

# notice: Undefined variable: account in /home/dave/Projects/www/drupal6dev/sites/all/modules/mollom/mollom.module on line 665.
# notice: Undefined variable: account in /home/dave/Projects/www/drupal6dev/sites/all/modules/mollom/mollom.module on line 683.

And looking into mollom_form_get_values() has me worried about this chunk:

  // User name.
  if (!empty($mapping['author_name'])) {
    $data['author_name'] = $mapping['author_name'];
    // Try to inherit user from author name.
    $account = user_load(array('name' => $data['author_name']));
  }
  elseif (!empty($user->name)) {
    $data['author_name'] = $user->name;
  }

  // User e-mail.
  if (!empty($mapping['author_mail'])) {
    $data['author_mail'] = $mapping['author_mail'];
  }
  elseif (!empty($data['author_name'])) {
    if ($account && !empty($account->mail)) {
      $data['author_mail'] = $account->mail;
    }
  }
  elseif (!empty($user->mail)) {
    $data['author_mail'] = $user->mail;
  }

  // User homepage.
  if (!empty($mapping['author_url'])) {
    $data['author_url'] = $mapping['author_url'];
  }

  // User ID.
  if (!empty($mapping['author_id'])) {
    $data['author_id'] = $mapping['author_id'];
  }
  elseif (!empty($data['author_name'])) {
    if ($account && !empty($account->uid)) {
      $data['author_id'] = $account->uid;
    }
  }
  elseif (!empty($user->uid)) {
    $data['author_id'] = $user->uid;
  }

If I submit an anonymous e-mail with the name 'Dries Buytaert' it's going to lookup Dries' account and put his mail and UID with something he didn't send. I don't think this was how it was looked up previously.

I started with writing tests for all of this to ensure we don't break it again. Not completed yet, but wanted to let you know that I'm on it.

Thanks sun. I was going to suggest adding our own form with sub-elements in mollom_form_test.module but you beat me to that. It'll be the best if we can test all these fringe cases ourselves like you're doing now. Awesome work.

This patch fixes the nested value handling.

I think I need more info about what exact author values we want to send to Mollom. Given that we now skip form validation/protection for users that are allowed to bypass it, the question boils down to anonymous and "regular" users.

I need to amend that modules exposing forms with author information to anonymous users should implement a logic like for example Comment module has: If an anonymous user tries to submit a comment using an author name of a registered user, the anonymous user will get a validation error.

The current logic to determine the author information that is sent to Mollom more or less follows that logic.

Alrighty, this one should really cover everything.

Also turns the fake server into an almost real server in testing mode, respecting the submitted data for the returned response.

Does this work for you? :)

Been busy with Do It With Drupal, but will review it shortly -- on the plane home.

Yeah I can review it this afternoon now as well.

A review would be great! :)

Confirmed that with sun's latest patch my Webform integration patch. Was unsure about how to handling things like mollom_data_save() and mollom_data_delete() as well as the post_id mapping for contrib modules. All those were issues that could be addressed post-patch.

+++ mollom.admin.inc	8 Dec 2009 16:32:07 -0000
@@ -130,11 +130,31 @@ function mollom_admin_configure_form(&$f
+      foreach ($mollom_form['elements'] as $key => $value) {
+        $elements[rawurlencode($key)] = $value;
+      }

A bit ugly, but required. I don't think we can get rid of the ugly 'parent][child' syntax.

+++ tests/mollom.test	8 Dec 2009 18:47:00 -0000
@@ -680,21 +681,130 @@ class MollomServerListRecoveryTestCase e
+    $edit = array(
+      'title' => 'ham',
+      'parent[child]' => 'spam',
+    );

Why not set one field to 'unsure'? The code that we use in the Java backend is this:

      if (body.contains("spam")) {
        spamResult = new SpamClassification(SpamClassification.Type.SPAM);
        quality = 0;
      }
      else if (body.contains("ham")) {
        spamResult = new SpamClassification(SpamClassification.Type.HAM);
        quality = 1;
      }
      else if (body.contains("unsure")) {
        spamResult = new SpamClassification(SpamClassification.Type.UNSURE);
        quality = 0.5;
      }
      else {
        throw new MollomException("Your key is in test mode. You have sent an unknown testing string '" + body +"', it should be 'ham', 'spam', 'unsure', 'redirect' or 'refresh'");
      }

and the Java-code for CAPTCHA requests when in test mode:

      if (solution.contains("incorrect")) {
        result = new CaptchaClassification(CaptchaClassification.INCORRECT);
      }
      else if (solution.contains("correct")) {
        result = new CaptchaClassification(CaptchaClassification.CORRECT);
      }
      else {
        throw new MollomException("Your key is in test mode. You have sent an unknown testing string '" + solution +"', it should be 'correct', 'incorrect', 'redirect' or 'refresh'");
      }

+++ tests/mollom.test	8 Dec 2009 18:47:00 -0000
@@ -680,21 +681,130 @@ class MollomServerListRecoveryTestCase e
+    // Try to submit values for nested field and multiple value field.
+    $edit = array(
+      'title' => 'ham',
+      'parent[child]' => 'ham',
+      'field[new]' => 'ham',
+    );
+    $this->drupalPost('mollom-test/form', $edit, 'Add');
+    $this->assertNoText('Successful form submission.');
+    $this->assertNoText($this->unsure_message);
+    $this->assertNoText($this->spam_message);
+    $edit = array(
+      'field[new]' => 'ham',
+    );
+    $this->drupalPost(NULL, $edit, 'Add');
+    $this->assertNoText('Successful form submission.');
+    $this->assertNoText($this->unsure_message);
+    $this->assertNoText($this->spam_message);
+    $edit = array(
+      'field[new]' => 'spam',
+    );
+    $this->drupalPost(NULL, $edit, 'Submit');
+    $this->assertNoText('Successful form submission.');
+    $this->assertText($this->spam_message);

It is not clear what you're trying to do here. There are only assertNoText()s, and they make it look like you want this to be neither submitted, nor blocked? I guess you can't check for a CAPTCHA because you're not using the actual Mollom server? If so, it might be nice to clarify that.

+++ tests/mollom_test.module	8 Dec 2009 18:59:25 -0000
@@ -27,9 +27,41 @@ function mollom_test_check_content($data
+    // Lastly, take a forced 'unsure' into account.
+    elseif (strpos($data[$key], 'unsure') !== FALSE) {
+      $spam = TRUE;
+      $ham = TRUE;
+    }
+  }
+  if ($spam && $ham) {
+    $response = MOLLOM_ANALYSIS_UNSURE;
+  }

Why not have a variable called $unsure like we do in the Mollom backend and API documentation?

I'm on crack. Are you, too?

I have clarified the test documentation, but extending the fake server to behave even more like a real server should really be tackled in a separate issue. I'm especially not sure how to properly tackle exceptions as well as the special 'redirect' and 'refresh' values. For the current tests, this functionality is not required.

Additionally, the unsure message was changed in the commit http://drupal.org/cvs?commit=301374 without updating the test. Fixed in this patch.

Great, thanks a lot sun. Committed the patch in #178 to CVS HEAD. There are a number of TODO list items to be extracted from this issue, so I'm marking it 'needs review', I guess.

Since the @todos are in code, I think we can mark this fixed. (I just wondered whether this issue still contained a patch I had forgotten...)

I fail to apply the patch (mollom-DRUPAL-6--1.any_.178.patch) to the files in the latest mollom version (mollom-6.x-1.10.tar.gz)

I type this in putty
patch < mollom-DRUPAL-6--1.any_.178.patch

and get:

patching file mollom.admin.inc
Hunk #1 FAILED at 130.
Hunk #2 FAILED at 205.
2 out of 2 hunks FAILED -- saving rejects to file mollom.admin.inc.rej
patching file mollom.module
Hunk #1 FAILED at 609.
Hunk #2 FAILED at 665.
Hunk #3 FAILED at 683.
Hunk #4 FAILED at 696.
4 out of 4 hunks FAILED -- saving rejects to file mollom.module.rej
can't find file to patch at input line 113
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|Index: tests/mollom.test
|===================================================================
|RCS file: /cvs/drupal-contrib/contributions/modules/mollom/tests/Attic/mollom.test,v
|retrieving revision 1.1.2.22
|diff -u -p -r1.1.2.22 mollom.test
|--- tests/mollom.test  7 Dec 2009 00:09:59 -0000       1.1.2.22
|+++ tests/mollom.test  14 Dec 2009 11:33:39 -0000

Could someone plz help?

Kind regards

@Eszee: It needs to be applied to the latest DRUPAL-6--1 CVS code. See http://drupal.org/node/240806/cvs-instructions/DRUPAL-6--1.

@Dave: Thanks & Happy New Year!

Is the patch already applied to latest version for d6 in CVS? I noticed you can protect webforms in the admin interface, but all spam go through.

Any idea when this is going to be included in a release? I have a client site that could really use this, but I'm hesitant to mess around with development versions for such a site.

Yup, what lisefranc said. Within a few hours of installing my first form, in flows the spam. Just presumed Mollom CAPTCHA dealt with this. Silly me! ;")

Fingers crossed for a webform/mollom marriage very soon! :"D

We're still busy fixing a couple of bugs in the development snapshot of the module. Hopefully we can do a release in 2 weeks or so.

So should we mark this back a active or needs work? I'm a little confused but it sounds like its not fixed yet and well... its confusing to see so much activity on a fixed bug.

I moved the bug reports to separate issues, because this issue is very long already; primarily #674230: Broken mass-reporting, session storage, node title mapping, watchdog messages, administrative user creation form

Subscribing. Need the patch or new release for this badly.

FYI: Completed the list of all Drupal core issues in #133

Just thought I'd post this:

http://mollom.com/blog/improved-mollom-module-with-webform-support

After some incredible work that I'm sure was more wide ranging than anyone originally thought, the new v3 beta release of webforms and the new release of mollom work together!

Thanks to all who worked so hard on this!

Great news! Support for webforms is very important to us.

Marc.

I've been using Mollom module about 3 months and It's working pretty good. I wonder Is there any way to put the mollom to login form ? or any form that I want.

Yes, that has been the sole purpose of this issue, which is resolved already. In the current stable release, you can technically protect any form with Mollom.

However, I'm going to revert the status and close comments on this issue now. If anything remains, please search the existing issues for your request or create a new one. Thanks!