Send statistical data

+++ mollom.module	7 Aug 2010 12:52:49 -0000
@@ -1398,6 +1398,31 @@ function mollom($method, $data = array()
+    if (!empty($info['distribution_name'])) {
+      $platform_name .= ' ' . $info['distribution_name'];
+    }

I didn't even remember that there was a property called 'distribution_name'. ;-)

Sounds like core should provide better APIs for this kind of stuff.

+++ mollom.info	2 Sep 2010 15:25:28 -0000
@@ -8,3 +8,6 @@ files[] = mollom.pages.inc
+; Dummy version required for testing. Do not remove.
+version = 7.x-1.x-dev

This is going to be prone to errors. We must remember to update this field for every release (which I'll fail to do).

Not sure I understand why it is required for testing. I don't see any tests for this functionality in the latest patch.

+++ mollom.module	2 Sep 2010 15:35:36 -0000
@@ -1565,6 +1571,47 @@ function _mollom_watchdog($messages, $se
+ * Returns version information to send with mollom.verifyKey.

I'd explain why Mollom captures this information and how Mollom might use it. Without an explanation, some people might think it is 'spyware'.

+++ mollom.module	2 Sep 2010 15:35:36 -0000
@@ -1565,6 +1571,47 @@ function _mollom_watchdog($messages, $se
+  // Fetch Mollom module information.
+  $mollom_info = db_query("SELECT info FROM {system} WHERE type = 'module' AND name = 'mollom'")->fetchField();
+  $mollom_info = unserialize($mollom_info);

Would be nice if there was an API for this ... ;-)

Maybe we need to update the code comment in the .info file then? The version number has nothing to do with testing, it sounds like.

Alternatively, we could use a CVS keyword like $Revision: $. It is better in some ways (i.e. auto-magical and more precise than "7.x-1.x-dev"), but worse in others ways (i.e. no easy correlation with an actual release).

I think this patch needs work (in addition to tests failing):

+++ mollom.module	3 Sep 2010 00:22:02 -0000
@@ -1429,6 +1429,12 @@ function mollom($method, $data = array()
+  // Add statistical data for mollom.verifyKey, which is normally invoked on
+  // Mollom's administration pages only.
+  if ($method == 'mollom.verifyKey') {
+    $data += _mollom_get_version();
+  }

I don't understand why we need to hard-code this in mollom(). That is bad practice. We should update the caller instead:

$status['keys valid'] = mollom('mollom.verifyKey', _mollom_get_version());

+++ mollom.module	3 Sep 2010 00:22:02 -0000
@@ -1565,6 +1571,59 @@ function _mollom_watchdog($messages, $se
+  if ($cache = cache_get('mollom_version')) {
+    $data = $cache->data;
+    return $data;
+  }

Why not return $cache->data;? No need to assign it to a temporary variable first.

+++ mollom.module	3 Sep 2010 00:22:02 -0000
@@ -1565,6 +1571,59 @@ function _mollom_watchdog($messages, $se
+    'client_version' => !empty($mollom_info['version']) ? $mollom_info['version'] : VERSION . '',

$mollom_info['version'] is always set now. We can remove the conditional/check.

+++ tests/mollom.test	3 Sep 2010 00:37:41 -0000
@@ -98,14 +98,17 @@ class MollomWebTestCase extends DrupalWe
-      $this->admin_user = $this->drupalCreateUser(array(
+      $permissions = array(
         'administer mollom',
         'access administration pages',
         'administer content types',
-        'administer comments',
         'administer permissions',
         'administer users',
-      ));
+      );
+      if ($this->profile == 'standard' || in_array('comment', $modules)) {
+        $permissions[] = 'administer comments';
+      }
+      $this->admin_user = $this->drupalCreateUser($permissions);
     }

I don't understand this change. Makes the code unnecessarily harder to read, it seems.

+++ tests/mollom.test	3 Sep 2010 00:37:41 -0000
@@ -2349,6 +2354,22 @@ class MollomDataTestCase extends MollomW
+    // Verify that we additionally sent version data.
+    $data = $this->getServerRecord('mollom.verifyKey');
+    $info = _mollom_get_version();
+    $this->assertSame('platform_name', $data['platform_name'], $info['platform_name']);
+    $this->assertSame('platform_version', $data['platform_version'], $info['platform_version']);
+    $this->assertSame('client_name', $data['client_name'], $info['client_name']);
+    $this->assertSame('client_version', $data['client_version'], $info['client_version']);
+  }

Even if _mollom_get_version() returned nothing or when _mollom_get_version() returned incorrect information, assertSame() would still pass. Both use _mollom_get_version(). If we want to check that values were set, we need to use something other than assertSame().

Looks great now. Committed to CVS HEAD.

Should be ported to DRUPAL-6--1.

Committed to CVS HEAD. Thanks!