Kill cache_mollom, rely on Form API

Exactly. We've taken care of most bugs in D7 already:

http://drupal.org/project/issues/search/drupal?issue_tags=D7%20Form%20AP... :)

Additionally, all forms in D7 need to be aware of form caching due to the new #ajax framework, so we should be safe to do this.

uhm, for later reference'n'investigation... note that form caching of $form_state is buggy in D6, but caching of $form is pretty straightforward (almost no changes in D7). Might be worth a try.

Core bug for reference:

#648170: Form constructors cannot enable form caching or form rebuilding

1250 passes, 0 fails, 0 exceptions

+++ mollom.module	16 Feb 2010 18:30:29 -0000
@@ -871,26 +867,29 @@ function mollom_theme() {
 function mollom_process_mollom($element, &$form_state, $complete_form) {
...
+  // The 'cache' flag does not persist upon validation errors (yet).
+  // @see http://drupal.org/node/648170
+  $form_state['cache'] = TRUE;

That's the issue I linked to in the previous follow-up. Without this override, the 'cache' flag gets lost in case of a form validation error, and therefore any updated information in $form_state (new Mollom session data) is lost, too.

+++ mollom.module	16 Feb 2010 18:30:29 -0000
@@ -871,26 +867,29 @@ function mollom_theme() {
     $form_state['mollom'] = array(
-      'session_id' => NULL,
...
-      'response' => NULL,
+      'response' => array(
+        'session_id' => '',
+      ),

This is an additional clean-up I sneaked into this patch - already wanted to do this for D6, but ran out of steam there. ;) We may want to consider to backport this, but it's not required.

In short: Whatever result is returned from Mollom servers, it's always stored in $form_state['mollom']['response'].

In the future, we want to check whether we can remove the global $mollom_response variable that we currently use for mollom_data_save(), because $form_state['mollom']['response'] is exactly what we want to store there. The usage of the global variable is caused by having to support e-mails though... a major challenge on its own.

+++ mollom.module	16 Feb 2010 18:30:29 -0000
@@ -1118,26 +1066,25 @@ function mollom_validate_captcha(&$form,
   // Bail out if no value was provided.
   if (empty($form_state['values']['mollom']['captcha'])) {
-    form_set_error('mollom][captcha', t('The CAPTCHA field is required.'));
     return;
   }

Apparently, this form error message was always displayed next to the "To submit this form, please complete the word verification." message that's already set by textual analysis.

I hope I didn't mixed this up during debugging, so I hope it is correct to remove it. Since the tests pass, it "should" be correct. But anyway, just wanted to point out that this additional form error may have had a (undocumented) reason. So let's try to keep this in mind.

+++ mollom.module	16 Feb 2010 18:30:29 -0000
@@ -1118,26 +1066,25 @@ function mollom_validate_captcha(&$form,
   // Check the CAPTCHA result.
   $result = mollom('mollom.checkCaptcha', array(
...
+  // Invoke fallback behavior upon a server error; communication errors are
+  // handled by mollom() already.
+  if ($result === MOLLOM_ERROR) {
+    return _mollom_fallback();
+  }

This is a quite interesting edge-case, which doesn't seem to be handled by mollom() yet.

During manual testing, I entered CAPTCHA values that are not compatible with testing keys. Mollom returned a MOLLOM_ERROR response, which did not bubble up. I was asked to re-enter the CAPTCHA all over again. Until I looked into the logs and found the error message.

In a separate issue, we should analyze whether mollom() shouldn't also trigger the fallback behavior in case of a MOLLOM_ERROR, since this error is only handled here for validation of CAPTCHAs with this patch.

+++ mollom.module	16 Feb 2010 18:30:29 -0000
@@ -1169,9 +1113,9 @@ function mollom_validate_captcha(&$form,
 function mollom_form_submit($form_id, &$form_state) {
   // Flush Mollom session information from database cache and user session.
...
+  if (!empty($form_state['mollom']['response']['session_id'])) {
+    // @todo Delete all sessions here? (concurrent form submissions?)
+    $session_id = $form_state['mollom']['response']['session_id'];
     unset($_SESSION['mollom_sessions'][$session_id]);
   }

I originally wanted to do what the @todo states here, but then reverted that, because I'm not 100% sure whether we also need to account for concurrent form workflows/submissions (of different forms).

+++ tests/mollom.test	16 Feb 2010 18:30:30 -0000
@@ -264,11 +260,12 @@ class MollomWebTestCase extends DrupalWe
   protected function setProtection($form_id, $fields = NULL) {

@@ -287,6 +285,9 @@ class MollomWebTestCase extends DrupalWe
+    // @todo Figure out why on earth we have stale, completely empty form info.
+    mollom_get_form_info(NULL, TRUE);

@@ -1220,6 +1221,9 @@ class MollomFormConfigurationTestCase ex
   function testFormFieldsConfiguration() {
+    // @todo Figure out why on earth we have stale, completely empty form info.
+    mollom_get_form_info(NULL, TRUE);

I still, really, have no idea at all, why we have to reset our statically cached form information data prior to running _any_ test.

Looking at this again, I guess we want to move it into setUp(), as long as we need it.

Powered by Dreditor.

- Double-checked the "CAPTCHA required" form error message manually and removing the additional error message seems to be fine.

- Cleaned up the other two todos from above list.

Now RTBC for real.

1247 passes, 0 fails, 0 exceptions