hook_mollom_form_info() does not scale

Almost :) The tricky part is that the hook implementations do not decide whether a form is protected - they just "register" + describe a form.

I didn't sleep yet, but based on your further thoughts...

<BRAINDUMP>

1. We need a list of forms that may be protected. Only for administrative purposes, to protect a (yet unprotected) form.
2. Whether a form needs to be protected or not, is the responsibility of mollom.module. After all, it's the only module that knows whether the user configured/protected a form (or not).
3. To figure out whether a $form_id is protected, we only need a list of $form_ids that are configured for/protected by Mollom. Since this information is only known to Mollom module, it could be cached separately (only statically or not) - in the end it's the result of SELECT form_id FROM {mollom_form}; - only form ids that are contained in the result are protected + vice-versa.
4. Effectively, we don't really need to invoke hook_mollom_form_info() when investigating whether we need to alter a form. We can build + prepare that info within Mollom module.
5. We only need to invoke hook_mollom_form_info() to gather the most current form definition from the implementing module (form elements can vanish + stuff).
6. We already store the 'module' a form belongs to. That may be helpful in only invoking the actually needed hook implementation - since we know the $form_id already, there's no point in invoking the hook for modules that don't own the form.
7. However, invoking the hook would still mean that we'd load ALL forms of the module. By passing an additional $form_id we could limit the loading to the requested $form_id.
8. This, however, would not work for the administrative list of forms that can be protected. For this list, we'd likely need a completely separate thing.

   TBH, the administrative select list scenario is what scares me most... Even if we are able to quickly load 10,000 webforms, this selection will be unusable. ;) Ultimately, that's probably more what http://drupal.org/project/form is about (in the long run), since it allows to configure forms "in context", i.e. without the complex list + selection problem. But that's D7 material only.

</BRAINDUMP>

Quickly answering the open questions before going to sleep:

what happens when I add a new field to an existing Webform?

If the form is configured for textual analysis (i.e. individual form elements were configured) then the new webform component will not be part of the textual analysis unless Mollom's protection for the form is updated.

If a webform component is removed from the form without updating Mollom's configuration, then Mollom will silently fail and ignore that particular component.

All enabled form elements are stored in {mollom_form}.enabled_fields for each $form_id.

Mollom doesn't store any information about a form unless it's been explicitly configured.

This is actually the case (perhaps my last follow-up might have been confusing). Mollom only stores a configuration for a form, if the form is protected in any way. Not sure whether that re-validates some of your thoughts?

--

Based on what we analyzed until now and currently know, this would mean to do the following:

function mollom_form_alter(&$form, &$form_state, $form_id) {
  static $protected_forms;
  if (!isset($protected_forms)) {
    // Mixing some D7 in here, shorter :P
    $protected_forms = db_select('mollom_form')->fields(NULL, array('form_id', 'module'))->fetchAllKeyed();
  }
  // If this form is protected at all...
  if (isset($protected_forms[$form_id])) {
    // ...ask the exposing module for its current definition.
    $function = $protected_forms[$form_id] . '_mollom_form_info';
    if (function_exists($function) && ($form_info = $function($form_id))) {
      // Merge form definition with stored form configuration.
      // @todo Perhaps, or perhaps not, mollom_form_load() could take over the merging (or not). ;)
      $mollom_form = mollom_form_load($form_id);
      // Start processing...
    }
  }
}

function webform_mollom_form_info($form_id = NULL) {
  // If no specific $form_id was requested, return all forms that can be protected.
  if (!isset($form_id)) {
    $result = db_query('SELECT nid, title FROM {node} WHERE type = :type', array(':type' => 'webform'))->fetchAllKeyed();
    $forms = array();
    foreach ($result as $nid => $title) {
      $forms['webform_client_form_' . $nid'] = t('Webform: @title', array('@title' => $title));
    }
    return $forms;
  }
  // Otherwise, return form definition for the requested $form_id only.
  ...
}

hook_mollom_form_info() implementations can decide on their own whether it makes sense to auto-protect forms by default or not - which really depends on the module. To do this, the implementation currently declares:

  'mode' => MOLLOM_MODE_*,

If the "mode" property is not set, then a form will not be protected upon installing Mollom module.

However. With respect to what we are discussing here, I'm not really sure whether we can keep on auto-enabling protection for forms upon installation of modules any longer. It requires us to do what we currently do: Load each and every thing and return a full form information array. On top of that, this happens in mollom_install() and mollom_modules_installed(), i.e. in a request that's very slow already...

ok, so we have the first agreement in removing the default protection upon installation. We can remove a lot of code then and tests need to be adjusted. Let's see what the testbot thinks about this.

heh, I guess it helps if I don't remove drupal_install_schema() :)

ok, this one passes. Now on to the other changes we discussed.

Here comes the real patch. :)

A couple of weird changes. Still some @todos to resolve, but at least all tests except Reporting functionality are passing already.

Implements what we discussed above: hook_mollom_form_info() only returns a quick list of $form_ids and their titles if no $form_id was passed. If a $form_id was passed, only the information about that form is returned.

Some feedback topics:

1) Do we want to split into 2 hooks: hook_mollom_form_list() and hook_mollom_form_info($form_id) ?

2) Any ideas what we could do with the reporting functionality? We don't even have a $form_id there - only a $entity and $session_id. The only way to solve this currently would be to have modules also return the 'entity' property in its form list. Most probably that's even the only solution.

3) I realized that we can keep the 'mode' property in the registered form information. It's still valuable to allow forms to define the intended default protection mode when protecting the form.

$op? oh noes! /me runs ;)

So, yes, let it be two separate hooks then. I basically prepared that with mollom_form_list() and mollom_form_info() already.

re: 2) Note that this only applies to the current reporting functionality... the revamped reporting through existing delete confirmation forms in #717212: Remove "report to Mollom" links and integrate with entity delete confirmation forms instead makes this question obsolete. However, that's mostly D7 material only (although I'll try to backport the enhancements, so that we can use it for Webform already)

On it.

+++ mollom.module	3 Mar 2010 21:57:39 -0000
@@ -477,53 +488,50 @@ function mollom_get_mode($form_id) {
+function mollom_form_info($form_id, $module) {
+  $form_info = module_invoke($module, 'mollom_form_info', $form_id);
...
+  $form_info += array(

$form_info can get NULL here in case a module gets disabled.

+++ mollom.module	3 Mar 2010 21:57:39 -0000
@@ -1736,31 +1763,46 @@ function comment_mollom_report_delete($e
+          'author_name' => 'name',
+          'author_mail' => 'name',

huh? I thought this was fixed already?

Powered by Dreditor.

Here we go.

hum? Testbot is unable to apply patch? Strange. Let's try again.

re: passing $module/storing lookup mappings: We need to figure out which lookup mappings we need prior to implementing a look-up cache. From what I can tell, we need

1) a $form_id => $module lookup,
2) an "entity" => $form_id + $module lookup,

and with #717212: Remove "report to Mollom" links and integrate with entity delete confirmation forms instead, we additionally need

3) a "delete $form_id" => $form_id + $module lookup.

Note that 2) and 3) are basically substitutes of each other. When integrating with delete confirmation forms, we only need the 3rd lookup, since the 2nd is only required for the custom/generic "report to Mollom" form, which will then only be used for reporting plain session ids (i.e. only used for e-mails currently).

However, I'm not sure whether the integration with delete confirmation forms will be fully backported to D6, since it removes all the "report to Mollom" links that users currently expect. But then again, it makes Mollom's UI much more logical/usable/grokable. Further discussion about this belongs into #717212: Remove "report to Mollom" links and integrate with entity delete confirmation forms instead, of course.

I'd recommend to tackle the lookup cache in a follow-up patch or even separate issue.

errr, WTF?! It's trying to checkout and apply this patch against 6.x-1.12 ?!?

#25: mollom-DRUPAL-6--1.scale_.25.patch queued for re-testing.

Same patch for HEAD.

Plus a small follow-up for D6... visual synchronization is important, again and again :)