Code clean-up

+++ mollom.admin.inc	17 Nov 2009 14:47:24 -0000
@@ -3,26 +3,23 @@
- * Administrative page callbacks for the mollom module.
+ * Administrative page callbacks for Mollom module.

Not sure we have a standard that says these need to be a certain way. The only thing I can find is http://drupal.org/node/1354#files which for .install files should be "Install, update and uninstall functions for the XXX module." which is what's currently used.

+++ mollom.admin.inc	17 Nov 2009 14:47:24 -0000
@@ -30,24 +27,28 @@ function mollom_admin_settings() {
-        '<p>'. t("Mollom can be used to block all types of spam received on your website's protected forms. Each form can be set to one of the following options:") .'</p>'.
-        '<ul><li>'. t("<strong>Text analysis and CAPTCHA backup</strong>: Mollom analyzes the data submitted on the form and presents a CAPTCHA challenge if necessary. This option is strongly recommended, as it takes full advantage of the Mollom anti-spam service to categorize your posts into ham (not spam) and spam.") .'</li>'.
-        '<li>'. t("<strong>CAPTCHA only</strong>: the form's data is not sent to Mollom for analysis, and a remotely-hosted CAPTCHA challenge is always presented. This option is useful when you wish to always display a CAPTCHA or want to send less data to the Mollom network. Note, however, that forms displayed with a CAPTCHA are never cached, so always displaying a CAPTCHA challenge may reduce performance.") .'</li>'.
-        '<li>'. t('<strong>No protection</strong>: Mollom is not used with this form.') .'</li></ul>'.
-        '<p>'. t("Data is processsed and stored as explained in our <a href=\"@mollom-privacy\">Web Service Privacy Policy</a>. It is your responsibility to provide any necessary notices and obtain the appropriate consent regarding Mollom's use of your data. For more information, see <a href=\"@mollom-works\">How Mollom Works</a> and the <a href=\"@mollom-faq\">Mollom FAQ</a>.", array('@mollom-privacy' => 'http://mollom.com/service-agreement-free-subscriptions', '@mollom-works' => 'http://mollom.com/how-mollom-works', '@mollom-faq' => 'http://mollom.com/faq')) .'</p>',
+      '<p>' . t("Mollom can be used to block all types of spam received on your website's protected forms. Each form can be set to one of the following options:") . '</p>' .
+      '<ul><li>' . t("<strong>Text analysis and CAPTCHA backup</strong>: Mollom analyzes the data submitted on the form and presents a CAPTCHA challenge if necessary. This option is strongly recommended, as it takes full advantage of the Mollom anti-spam service to categorize your posts into ham (not spam) and spam.") . '</li>' .
+      '<li>' . t("<strong>CAPTCHA only</strong>: the form's data is not sent to Mollom for analysis, and a remotely-hosted CAPTCHA challenge is always presented. This option is useful when you wish to always display a CAPTCHA or want to send less data to the Mollom network. Note, however, that forms displayed with a CAPTCHA are never cached, so always displaying a CAPTCHA challenge may reduce performance.") . '</li>' .
+      '<li>' . t('<strong>No protection</strong>: Mollom is not used with this form.') . '</li></ul>' .
+      '<p>' . t("Data is processsed and stored as explained in our <a href=\"@mollom-privacy\">Web Service Privacy Policy</a>. It is your responsibility to provide any necessary notices and obtain the appropriate consent regarding Mollom's use of your data. For more information, see <a href=\"@mollom-works\">How Mollom Works</a> and the <a href=\"@mollom-faq\">Mollom FAQ</a>.", array('@mollom-privacy' => 'http://mollom.com/service-agreement-free-subscriptions', '@mollom-works' => 'http://mollom.com/how-mollom-works', '@mollom-faq' => 'http://mollom.com/faq')) . '</p>',
       '#collapsible' => TRUE,

Lost indentation for form element property.

+++ mollom.module	17 Nov 2009 14:47:06 -0000
@@ -3,31 +3,91 @@
+ * Implements hook_help().

Are we using the Drupal 7 style "Implements hook_x()." in Drupal 6 now?

+++ mollom.module	17 Nov 2009 14:47:06 -0000
@@ -899,8 +985,11 @@ function _mollom_debug($message) {
+ *
  * @return
  *   An array of statistics.
+ *
+ * @todo This is dead code? Not invoked anywhere.

Not sure we put blank lines between @params and @returns. Also, this function is going to be used on the statistics page and possibly for a block.

I'm on crack. Are you, too?

Oddly enough, the patch doesn't apply for me.

...
Hunk #32 succeeded at 1032 (offset -1 lines).
2 out of 32 hunks FAILED -- saving rejects to file mollom.module.rej
patching file mollom.pages.inc

Probably because I just changed hook_cron() to hook_flush_caches(). Sorry. :/

Re re 1: We need to file an issue to fix core @file documentation then, since basically none of the Drupal 7 or 6 @file directives use the proper module case. And we should document this somewhere.

Re re 4: Correct, it is not currently utilized.

mollom_get_statistics() needs to move to a re-usable Mollom.class.php -- but obviously not part of this patch.

Reviewed the code and it looks good. I ran the tests locally, and they all pass. So committed to CVS HEAD. Thanks sun.

Thanks sun!

Committed to CVS. Thanks!

+++ tests/mollom.test	19 Nov 2009 13:46:13 -0000
@@ -7,54 +7,83 @@
-  function setUpMollom() {
+  function setUp() {
+    // Call parent::setUp() allowing Mollom test cases to pass further modules.
+    $modules = func_get_args();
+    $modules[] = 'mollom';
+    call_user_func_array(array('parent', 'setUp'), $modules);
+

Very nice change, thanks.

+++ tests/mollom.test	19 Nov 2009 13:46:13 -0000
@@ -343,7 +366,7 @@ class MollomFallbackTestCase extends Mol
+    // @todo Test this more rigorously once we can hijack drupal_mail().

This means DWTC->assertMail() wasn't available yet, but now it is. I'm working on adding tests for it with fixing the e-mail reporting issues.

+++ tests/mollom.test	19 Nov 2009 13:46:13 -0000
@@ -362,7 +385,7 @@ class MollomFallbackTestCase extends Mol
-    variable_set('mollom_servers', array('http://fake1.mollom.com'));
+    variable_set('mollom_servers', array('http://fake-host'));

Not sure why this needs to be changed. Some DNS (OpenDNS) do weird stuff if you don't enter a proper domain name. Using a fake, valid domain name ensures same result among all tests. Maybe we could use mollom.example.com so we don't hit Mollom servers.

This review is powered by Dreditor.

Hah, missed that in my previous cleanup. That was the only test that caused E_NOTICE errors to pop up in tests because I use OpenDNS. Trying them all tests with proper fake hosts now.

Ok, ignore what I said. I think I changed that around the time we had some instability with testing and the Mollom servers in developer mode. Everything works fine just using 'fake-host'.

+++ mollom.admin.inc	19 Nov 2009 12:55:55 -0000
@@ -17,9 +17,13 @@ function mollom_admin_settings() {
       // Verify the key and output a status message.
+      // @todo This must be a form #validate handler; the form must not be
+      //   submitted if validation fails.
       _mollom_verify_key();

I'm not sure verifying the keys should be a validation-stopper. For instance, if the Mollom servers are down, users should still be able to save their keys. Not a commit-stopper though, but keep it in mind. I also changed all the @todo's for testing mail to ' @todo Test mail sending with assertMail() now that it is available.'

This review is powered by Dreditor.

Nice clean-up. Some feedback:

+++ tests/mollom.test	19 Nov 2009 13:46:13 -0000
@@ -7,54 +7,83 @@
+    // Call parent::setUp() allowing Mollom test cases to pass further modules.
+    $modules = func_get_args();
+    $modules[] = 'mollom';
+    call_user_func_array(array('parent', 'setUp'), $modules);

The code comment is a bit cryptic: 'to pass further modules'?

+++ tests/mollom.test	19 Nov 2009 13:46:13 -0000
@@ -343,7 +366,7 @@ class MollomFallbackTestCase extends Mol
+    // @todo Test this more rigorously once we can hijack drupal_mail().

This is possible in Drupal 7. Might be good to mention that explicitly.

+++ tests/mollom.test	19 Nov 2009 13:46:13 -0000
@@ -449,20 +468,16 @@ class MollomServerListRecoveryTestCase e
+   * @todo Test this more rigorously once we can hijack drupal_mail().

See above.

I'm on crack. Are you, too?

We believe this one was committed by David. Marking 'fixed'.