Port to Drupal 7

Incorporated the patch from #562374: Mollom incompatible with Pressflow's (and Drupal 7's) session handling -- causes CAPTCHAs to fail, which fixes a couple of tests.

Also fixed a couple of more issues. Getting closer. ;)

re 1) Nope, my revamp actually removed all Form API hacks ;)

re 2) Thanks for committing that fix.

re 3) Good guess, but I seriously think that this category is not really appropriate...

Alright. I was finally able to understand why the tests are failing... #663992: Verbose debug output is cached by the browser OMG. :P

I fear that this will end in yet another >100 KB patch...

oh my. It definitely helps to clear the entire browser cache. Spent another couple of hours looking at totally weird but entirely stale debug pages. :-/

1 failing test remains.

The remaining failing test/assertion cannot be resolved within Mollom module - see the @todo I added.

Actually, there are two larger @todos added in this patch. One is bound to Mollom.module's architecture, the other one is about D7's render system, and the latter is the one I mean here.

Note that this patch is still rolled against DRUPAL-6--1; I believe that it should apply cleanly against HEAD though.

Lastly, this patch is still a straight port of the current code, not taking into account any new options in D7. The latter should always be a separate step in porting modules. (You know what happened to CCK in D6, eh? ;)

oh, I *love* your reviews! :)

Fixed those.

Not sure about the mollom_form_load() comment though... I guess you mean similar code in mollom.test -- but that code doesn't unserialize the configured fields, just checks whether a $form_id has been stored.

Fixed some last bits. I think this is ready to fly. We cannot resolve the remaining fail without entirely rewriting the implementation (which we should still try, but I'm not even sure whether it's possible at all).

Oh. My. God.

I quickly patched batch.inc + form.inc in D6 to get the total elapsed time for running the tests to compare them with D7....................

D6 == 1 min 37 sec
D7 == 4 min 5 sec

errr, and we want to release? No way.

uh oh - something went wrong during syncing DRUPAL-6--1 with HEAD... All synced files went into the tests/ subdirectory ;)

EDIT: See http://drupal.org/cvs?commit=304076 for what I mean.

+++ mollom.install	19 Dec 2009 21:28:23 -0000
@@ -260,16 +258,28 @@ function mollom_update_6106() {
+    ->fields(array('did' => 'SUBSTR(did, 6)')) // @todo Expressions in UPDATE?
...
+    ->fields(array('did' => 'SUBSTR(did, 9)')) // @todo Expressions in UPDATE?

Need to talk to Crell or grep throughout core to find out whether this is possible.

+++ mollom.module	19 Dec 2009 21:51:45 -0000
@@ -125,28 +125,22 @@ function mollom_help($path, $arg) {
+function mollom_init() {
+  // Expire all mollom session IDs as soon as possible.

Rename to mollom_exit() and eventually move below mollom_form_submit().

+++ mollom.module	19 Dec 2009 21:51:45 -0000
@@ -288,12 +283,19 @@ function mollom_report_access($entity, $
+      'title' => t('Administer Mollom-protected forms and Mollom settings'),
...
+      'title' => t('Post content without checking'),

The human-readable permission strings need work.

+++ mollom.module	19 Dec 2009 21:51:45 -0000
@@ -288,12 +283,19 @@ function mollom_report_access($entity, $
+    'access mollom reports' => array(
+      'title' => t('Access Mollom reports'),
+    ),

Actually, I implemented this for testing purposes only. Can probably be removed, sorry.

+++ mollom.module	19 Dec 2009 21:51:45 -0000
@@ -498,9 +500,13 @@ function mollom_get_form_info($form_id =
 function mollom_form_load($form_id) {
-  $mollom_form = db_fetch_array(db_query_range("SELECT * FROM {mollom_form} WHERE form_id = '%s'", $form_id, 0, 1));
+  $mollom_form = db_query_range('SELECT * FROM {mollom_form} WHERE form_id = :form_id', 0, 1, array(':form_id' => $form_id))->fetchAssoc();
   if ($mollom_form) {
     $mollom_form['fields'] = unserialize($mollom_form['fields']);
+    // @todo Is this a bug in D7?
+    if (!$mollom_form['fields']) {
+      $mollom_form['fields'] = array();
+    }

Investigate whether the old code was wrong or whether D7 has a bug here, and why D6 behaved differently.

+++ mollom.module	19 Dec 2009 21:51:45 -0000
@@ -994,11 +992,22 @@ function mollom_validate_analysis($form,
+function mollom_validate_captcha(&$form, &$form_state) {
...
+  // @todo Problem: This error is output also in case we have no valid keys or a
+  //   communication error, in this request or perhaps even in the last already.
+  //   Thus, in case the fallback method blocks all submissions, a user gets TWO
+  //   error messages, and one of them being totally nuts, because *there is no*
+  //   CAPTCHA in the form, because we didn't reach the servers in the first
+  //   place. However, we also do not really want to invoke lots of XML-RPC
+  //   requests for the case when we (could) *know* that we won't get a usable
+  //   $result, because the servers are empty or keys are invalid - unless the
+  //   site administrator revisits Mollom's settings page again and fixes the
+  //   keys/servers.
+
   // 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.'));

@@ -1012,6 +1021,12 @@ function mollom_validate_captcha($elemen
+  // Cancel validation if we have communication issues.
+  // @todo These should really be bitwise OR'ed error code constants...
+//  if ($result == NETWORK_ERROR || $result == MOLLOM_ERROR) {
+//    return;
+//  }

We probably want to add an internal system variable to track the current "healthiness" of the module's configuration - i.e. if there are no keys, or invalid keys, or whether we have no valid server list, then always invoke the fallback behavior. To be implemented as new (first) #process or #element_validate of the 'mollom' element.

+++ mollom.module	19 Dec 2009 21:51:45 -0000
@@ -1057,6 +1072,13 @@ function mollom_pre_render_mollom($eleme
+    // @todo D7: A horrible effect of the new rendering system: If we cannot
+    //   reach Mollom servers, then mollom() will invoke _mollom_fallback(),
+    //   which outputs a message. However, this is #pre_render. The main page
+    //   content is render()'ed *within* page.tpl.php. Which is also where
+    //   $messages are rendered. But $messages are rendered before $content!
+    //   I already briefly discussed this chicken-n-egg problem with moshe, but
+    //   as of now, we have no ideas. 19/12/2009 sun
     $result = mollom('mollom.getImageCaptcha', $data);

I already spent hours tinkering about this, but still have no idea how we could resolve that.

That's quite a critical regression, because it basically means that no renderable array in the page template can output a message. The only idea I had so far was to eventually inject all messages in an ESI-style approach, i.e. replacing a replacement token/macro in the fully rendered page.tpl.php HTML markup with the actual messages. Quite a hack. Perhaps we simply went too far with usage of render() for the main page content (if the $content variable in page.tpl.php was an already rendered HTML string, then this problem would not exist).

As a workaround for Mollom, it would only be resolvable, if we would revamp the entire implementation, and perform all form alterations and validations during element and form validation.

+++ mollom.module	19 Dec 2009 21:51:45 -0000
@@ -1070,26 +1092,36 @@ function mollom_pre_render_mollom($eleme
-    // Otherwise, hide the CAPTCHA if there was a communication error.
-    elseif ($result === NETWORK_ERROR) {
+    // Otherwise, hide the CAPTCHA.
+    else {

There are a couple of fixes like this that need to be back-ported. Created #665270: Backport fixes from port to D7 for that.

+++ mollom.module	19 Dec 2009 21:51:45 -0000
@@ -1362,15 +1396,17 @@ function node_mollom_form_info() {
+function mollom_node_insert($node) {
+  mollom_data_save('node', $node->nid);
+}
...
+function mollom_node_delete($node) {
+  mollom_data_delete('node', $node->nid);
 }

You are right that we can now try to leverage the 'post_id' mapping property that was intended for this purpose (to be processed in mollom_form_submit()).

In addition to that, I considered whether we can perhaps simplify the current 'report delete callback' that can be defined in hook_mollom_form_info() - and change it to the $form_id of the real/original delete confirmation form of the entity instead.
Hence, instead of duplicating functionality (we always delete), we would only integrate with the existing deletion functionality/workflow.

+++ mollom.module	19 Dec 2009 21:51:45 -0000
@@ -1389,6 +1425,26 @@ function mollom_form_node_admin_content_
+function mollom_node_view($node, $build_mode) {
+  // Only show the links if the module is configured.
...
+    $node->content['links']['mollom'] = array(
+      '#theme' => 'links',
+      '#links' => array(
+        'mollom_report' => array(

This is another bug in Drupal core -- Node module registers $node->content['links']['node'] and assigns theme_links() for the defined links in there. The current design of node/comment links seems to have intended to build separate lists of links for each module.

That, however, looks very wonky in the rendered output, and also doesn't make much sense from the generated HTML markup.

Not sure whether there is already an issue for that, but since the fix is likely to be an API change (just skipping the second array level?), we should fix it soon. Not sure whether I'll have time for that.

This review is powered by Dreditor.

Attached patch does the trivial fixes.

I just ran the tests again (via GUI) and still get

894 passes, 1 fail, 0 exceptions, and 213 debug messages

whereas this 1 fail belongs to

Fallback behavior
77 passes, 1 fail, 0 exceptions, and 19 debug messages

and is the fail I documented + explained in-code as well as above.

Indeed, very odd.

But anyway, let's try whether we can revamp all of this for D7 now. :)

erm, also included -- can we remove that duplicate tests/tests/ directory? ;)

Thanks for removing the test files!

uhm, wait - do we automatically disable page caching if there is a session for the current user in HEAD? If this is true, then we need to update Mollom's code in various locations. For now, I didn't merge that changed comment.

All tests pass for me.

What's left? - Entirely killing the #pre_render.

+++ mollom.module	24 Dec 2009 14:21:01 -0000
@@ -991,9 +1000,13 @@ function mollom_validate_analysis($form,
 function mollom_validate_captcha(&$form, &$form_state) {
...
+  // Prevent the page cache from storing a form containing a CAPTCHA element.
+  $GLOBALS['conf']['cache'] = CACHE_DISABLED;

errr, this is the wrong location to prevent page caching ;)

I'm on crack. Are you, too?

Fixed that.

Resolved a couple of @todos:

- Removed #pre_render.

- Implemented hook_modules_uninstalled().

- Removed debugging code.