Global fallback and status handling

+++ mollom.admin.inc	18 Jan 2010 23:32:24 -0000
@@ -266,13 +266,7 @@ function mollom_admin_settings() {
   if ($keys) {
     if (!$_POST) {
       // When a user visits the Mollom administration page, automatically
-      // clear the server list.  This causes the client to fetch a fresh
-      // server list from the server.
-      // @todo Add a timeout interval. Also consider hook_enable() together with
-      //   hook_cron().
-      variable_del('mollom_servers');
-
-      // Verify the key and output a status message.
+      // verify the key and output a status message.
       _mollom_verify_key();
     }
   }

btw, deleting the server list was the cause for the bug that invalid API keys were never reported.

Powered by Dreditor.

+++ mollom.admin.inc	18 Jan 2010 23:32:24 -0000
@@ -335,10 +329,7 @@ function _mollom_verify_key() {
-  else {
-    drupal_set_message(t('We contacted the Mollom servers to verify your keys: the Mollom services are operating correctly. We are now blocking spam.'));
-    return TRUE;
-  }
+  return TRUE;

Also, I removed this positive status message, because it can be totally wrong -- when no forms are protected.

Powered by Dreditor.

Posting what I have now... still failing tests (due to [my] wrong usage of xmlrpc server functions... bad/non-existing documentation... cross-linking #621618: Revamp MAINTAINERS.txt...)

All tests pass now. Will implement the main change now, which isn't contained yet.

If "operable" is not acceptable (IMHO, it's the most precise term), then the following synonyms would be possible: ready, usable, workable, verified, ok, working, overall, summary, total, ...

Out of those options, I'd choose "operable" or "ready".

The goal is to have a positive term (not negated) that indicates the overall status of the module's configuration and communication, and will be used as $status['operable'].

I've also suggested the (ugly) workaround idea to return two different data types depending on the overall status; simply $status === TRUE if everything is alright, or $status == array(...) with individual indicators as array keys if something is wrong. However, just mentioning this, I don't really like the idea either ;)

+++ mollom.module	19 Jan 2010 23:09:48 -0000
@@ -1178,12 +1216,17 @@ function mollom($method, $data = array()
+      'text' => 'Refreshed servers: %servers',

This is the identical watchdog message that is already used some lines below in that function; also keeping it as short as possible but still meaningful.

Powered by Dreditor.

ok, now including the final change.

Alrighty - this new test finally reveals and proves the wrong error handling of the Mollom server response in case we have invalid keys.

Running the new "Status handling" test, we get the following errors when trying to save the settings form with 'foo' + 'bar' as keys:

Error 1000: http://xmlrpc1.mollom.com - Mollom could not find your public key 'foo'. - mollom.getServerList
Error 1000: http://xmlrpc2.mollom.com - Mollom could not find your public key 'foo'. - mollom.getServerList
Error 1000: http://xmlrpc3.mollom.com - Mollom could not find your public key 'foo'. - mollom.getServerList
All servers unavailable: --, last error: 1000 - Mollom could not find your public key 'foo'.
Refreshed servers:
All servers unreachable or returning errors. The server list was emptied.

The problem obviously is that we are invoking mollom.verifyKey, but we have an empty server list at this point. Since mollom.getServerList also requires valid keys, mollom.verifyKey is not even invoked, because we don't have a server list. Hence, we get a NETWORK_ERROR in the end - instead of a MOLLOM_ERROR.

Alright, this one should work. Also contains a big @todo, but not sure whether it needs to be resolved here. Reviews welcome.

Unrelated to this patch, I finally got this 1 fail about a session_id mismatch in the "Server responses" test (once). Based on the debug output in the test log, it seems like the returned session_id changed without any server redirect or refreshing of the server list. That should not happen - or at least, we currently expect it for the two aforementioned occasions only. This is not tackled in this patch and belongs into a separate issue.

So we discussed to use different return values for _mollom_status() --- however, actually not sure whether this is really prettier.

Clarified and cleaned up _mollom_status() a bit.

Additionally, syncing tests with HEAD.

#13 is RTBC. And attached patch applies the same changes to HEAD.

http://drupal.org/cvs?commit=321704

Re-introducing the positive status message.

Adjusting tests accordingly.

Needs forward-porting though. :)