Implement a generic Mollom PHP class

Very much agreed. Would be a great fit to bring this in and maintain it, while offering flexibility for other solutions to be able to override class methods. For instance a Mollom::request() or Mollom::xmlrpc() method that creates a HTTP request with cURL would be overridden in Drupal's implementation to use drupal_http_request() or xmlrpc().

Better title.

Clicked on this issue for the second time, because I got mistaken by the issue title and thought "wait, at least D7 leverages PHP5" ;)

Setting this to postponed until likely a 7.x-2.x branch. There are a lot of things I need to focus on first..

Hi,

I'm the maintainer of the Mollom Wordpress plugin (http://wordpress.org/extend/plugins/wp-mollom/). Over the past two years, I've maintained a stable branch of the plugin which was, well, mostly rapid prototyped. The way the plugin was built hampered development.

One of my biggest nags was the lack of a generic API which other plugin Wordpress could easily use. Wordpress doesn't have a concept of a unified form API like Drupal does, for one.

So, I've been busy working on a totally revamped version of the Wordpress version over the past months. While revising the entire architecture, I separated Wordpress specific code from my implementation of the Mollom API. A nice class was the net result. I was considering putting it up for peer review on Github as a separate project, until Dries asked me to post it here.

Anyway, the class is attached as a 'patch' with this comment.

How does it work?

Contrary to Tijs Verkoyens' class, this class doesn't come with a specific XML-RPC I/O base layer. I felt other developers should be able to use their preferred XML-RPC libraries instead of being restricted to a single specific implementation.

The class takes an instance of type MollomRPCClient and uses that instance for all it's I/O. The type is defined through an interface. If you use a stock XML-RPC library like the Incutio XML-RPC library, you'll need to write a wrapper class which implements this interface. In case of Drupal, this will mean implementing a wrapper class around the xmlrpc() function. (Maybe this can be done in a more efficient way. But this worked for me.)

This is how I bootstrap the class in my own plugin within a separate static factory function:

$xmlrpc_instance = new MollomRPCClientWrapper('0.0.0.0', false, 80, 10); // this is a wrapper class which implements the interface

$mollom = new Mollom($private_key, $public_key, $xmlrpc_instance); // this is the actually instance of the Mollom class

$mollom_servers = get_option('mollom_servers', NULL); // Retrieves the serverlist from cache
$mollom->setServerList($mollom_servers);  // This sets the cached serverList.

The factory function returns the $mollom object which is now ready to be used.

It would be great to join efforts. So feedback is welcome and I'm happy to answer questions!

corrects a typo in the setPrivateKey() function.

Agreed a generic mollom backend would be good. Unfortunately for mollomapi.module to be able to use this, it either needs to be in a separate module or the mollom module should not expose its usual UI.

I'm actively working on this, in combination with #1104292: Replace XML-RPC with REST API.

The code lives in the 7.x-class topic branch (mollom.inc) and is still undergoing major changes. Conversion started on a very low-level. Still contains lots of old/temporary code from an initial test conversion, including code for XML-RPC protocol that will be completely removed. Not really ready for review yet, but might provide some early clues on the rough edges.

mmm, ok, this is huge. :)

Might make more sense to look at the actual branch: http://drupalcode.org/project/mollom.git/tree/refs/heads/7.x-class

Some more clean-ups.

This is ready for review. The test failures cannot be resolved without further REST API adjustments.

That said, all of the Drupal tests are still running against the local dummy/fake REST server in this patch, because the production REST testing API differs too much from the specification.

Minor tweaks, resolving another @todo:

- Moved _mollom_get_version() into Mollom class.
- Fixed MollomDataTestCase.

On configuration CRUD methods: There were individual read/write/delete methods earlier, but it was a lot of duplicate code. All Mollom client implementations for other platforms that I've looked into use and support a simple key/value store, so in the end, it felt needless to make these methods more granular than they need to be, forcing everyone to implement individual CRUD methods for every value when it's really about a trivial key/value configuration store only. Almost all clients will relay those operations to an existing component/subsystem in their system. An extremely simple implementation would merely use file_get_contents(), file_put_contents(), and unlink().

Spent some time to think about the exception, logging, and error code/message handling. When I changed query() and request() to use exceptions instead of passing around arbitrary error codes, the entire code became a lot cleaner and more predictable.

However, the surrounding code and logic for handling error codes and messages, as well as the internal logging of which requests were attempted, still remained and rather became more crufty, since throwing an exception halts the code execution. Therefore, I came up with this idea and asked around for any objections. None were raised, so I'm going to implement that change now.

As intended and imagined, the idea heavily simplifies the internal error handling and logging. (commitdiff)

Looks like I forgot to mention that we have special needs for internal error handling in the Mollom class -- we need to track the "state" and possible errors in various locations within the class. Almost all errors and exceptions are not supposed to bubble up to the end-user; instead, the wrapping CMS plugin/module needs to react "nicely" in case troubles. Whereas possible troubles have varying severities - the client-side server balancing in the class may be able to recover from an error or bogus state, and in case it does not, the attempted recovery operations need to be logged, so a site administrator is able to provide these details in bug reports/support requests they send us.

Updated cumulative patch.

.

@todo: Move mollom.inc into includes/mollom.class.inc and put a README.txt next to it. Check whether it makes sense to also move mollom.drupal.inc in there.

For the sake and hope of reviews, attached is a diff between 7.x-1.x and 7.x-2.x branches.

Let me iterate over the remaining @todos. Some of them are very clear already and just need to be implemented, but on some others, I'd appreciate any kind of feedback:

+++ includes/README.txt
@@ -0,0 +1,24 @@
+-- USAGE --
+
+* @todo

Let's add a very simple usage scenario here; e.g., checking a content for spam.

+++ includes/mollom.class.inc
@@ -0,0 +1,1283 @@
+ * @todo Refine exceptions by necessity of having to react differently.
+ *   - All 5xx HTTP codes, MollomRedirectException, and MollomResponseException
+ *     should make the client try the next server.
+ *   - Only AUTH_ERROR and 404 are not recoverable.

I think I've checked more than once in the meantime whether we can refine the exception classes, but they do actually trigger different behaviors in some calling methods. Thus, I think this @todo is obsolete.

+++ includes/mollom.class.inc
@@ -0,0 +1,1283 @@
+ * @todo Doing a "verifyKey" with an empty server list leads to two/duplicate
+ *   and unnecessary GET site/$id requests, one for the server list, subsequent
+ *   ones for whatever/verifyKey. Some resources can be cached?

While the observation and symptom is correct, the client actually has to perform the subsequent POST to update its client information. Without the preceding GET (in the edge-case of having no server list), it can't perform the POST. Thus, I think this @todo is obsolete, too.

+++ includes/mollom.class.inc
@@ -0,0 +1,1283 @@
+/**
+ * A catchable Mollom exception.

Let's move the exception classes to the bottom of the file - it's wonky and distracting to see them first, instead of the main Mollom class.

+++ includes/mollom.class.inc
@@ -0,0 +1,1283 @@
+  public $publicKey = '';
...
+  public $privateKey = '';
...
+  public $servers = array();
...
+  public $serversInit = array('http://rest.mollom.com');

These should be protected now. Public visibility was only needed for development and debugging.

+++ includes/mollom.class.inc
@@ -0,0 +1,1283 @@
+  /**
+   * Loads a configuration value from client-side storage.
...
+   */
+  abstract public function loadConfiguration($name);
...
+  /**
+   * Saves a configuration value to client-side storage.
...
+   */
+  abstract public function saveConfiguration($name, $value);
...
+  /**
+   * Deletes a configuration value from client-side storage.
...
+   */
+  abstract public function deleteConfiguration($name);

I still think that these configuration CRUD methods are much easier to deal with for implementors of the class.

The only possible downside I could see is that mayhaps not all implementations are using or supporting a variable data store. With these configuration CRUD methods, they would have to manually track the possible configuration setting names and adjust their storage accordingly. Whereas with individual abstract loadPublicKey(), saveServers(), etc methods, the entire Mollom client class implementation would turn invalid in case of an API change or addition.

Happy to change these into individual methods if there's a strong opinion or reasoning to do so.

Regardless of that, I don't think these methods need to be public.

+++ includes/mollom.class.inc
@@ -0,0 +1,1283 @@
+   * @see Mollom::log

There should be a ->log() method for adding/recording log messages.

+++ includes/mollom.class.inc
@@ -0,0 +1,1283 @@
+  public function getAuthentication() {

Still need to implement OAuth request signing. Working on it.

+++ includes/mollom.class.inc
@@ -0,0 +1,1283 @@
+  public function getServers() {
+    // If there is no server list yet, consult the local configuration.
+    if (empty($this->servers)) {
+      $servers = $this->loadConfiguration('servers');

In case we will change the configuration CRUD methods into individual methods, then we need a solid and consistent naming pattern for methods; e.g.,

- loadFoo() to retrieve Foo from local storage,
- getFoo() to retrieve and prepare Foo in whatever means (which may or may not involve calling loadFoo())

+++ includes/mollom.class.inc
@@ -0,0 +1,1283 @@
+          $this->log[] = array(
+            'type' => 'debug',
+            'message' => 'Refreshed servers: %servers',

Log message severities/types should follow a consistent pattern; i.e.,

- 'debug' for low-level request debugging data,
- 'info', 'notice', etc. for higher-level operation info.

+++ includes/mollom.class.inc
@@ -0,0 +1,1283 @@
+    // Initialize refresh variable.
+    $refresh = FALSE;
+    // Send the request to the first server; if that fails, try the other
+    // servers in the list.
+    // @todo The Mollom instance "sticks" now, as it's statically cached. Thus,
+    //   the array cursor in $this->servers is retained across multiple queries.
+    //   I.e., subsequent queries will continue to communicate with the
+    //   "current" server, which might not be the first.
+    //   PRO: In a scenario that uses the statically cached class instance
+    //   (low-level scripts and possibly subsequent form submissions in the
+    //   future) and in which a server redirects, subsequent queries will
+    //   continue to use the current/second/next server in the list instead of
+    //   restarting on the first; i.e., potentially less "wasted" requests.
+    //   CON: Higher chance to reach the end of the server list, unless we allow
+    //   to iterate two times over the server list until we consider a request
+    //   to fail.
+    while ($server = current($this->servers)) {

Changing the default server in the static cache (not configuration) definitely makes sense.

However, we need to implement a "continuous" loop, which starts at the current index, and allows to cross the array end boundary (going back to start), until we reach the originally current index. Probably sounds more complicated than it is.

In addition, the Mollom backend team is going to investigate whether clients can be allowed to update the default server in their locally stored server list.

+++ includes/mollom.class.inc
@@ -0,0 +1,1283 @@
+        // Try the next server in the list.
+        $next = next($this->servers);
...
+        // @todo $next may be FALSE, confusing users looking into logs.
...
+          'message' => 'Server %server redirected to %next.',
...
+            '%next' => $next,

($next ? $next : '--')

+++ includes/mollom.class.inc
@@ -0,0 +1,1283 @@
+  public function getSites() {
+    $result = $this->query('GET', 'site', array(), array('list'));
+    // In XML, 'list' is a string when blacklist is empty.
+    // @todo Move into query().
+    if (isset($result['list'])) {
+      // parseXML() can only convert multiple sub-elements into an indexed array.
+      if (is_array($result['list'])) {
+        $result['list'] = array_values($result['list']);
+        return $result['list'];
+      }
+      return array();
+    }
...
+  public function checkContent(array $data = array()) {
...
+    // parseXML() can only convert multiple sub-elements into an indexed array.
+    if (isset($result['content']['languages']) && is_array($result['content']['languages'])) {
+      $result['content']['languages'] = array_values($result['content']['languages']);
+    }
...
+  public function getBlacklist($publicKey = NULL) {
...
+    $result = $this->query('GET', 'blacklist/' . $publicKey, array(), array('list'));
+    // In XML, 'list' is a string when blacklist is empty.
+    // @todo Move into query().
+    if (isset($result['list'])) {
+      // parseXML() can only convert multiple sub-elements into an indexed array.
+      if (is_array($result['list'])) {
+        $result['list'] = array_values($result['list']);
+        return $result['list'];
+      }
+      return array();
+    }

Still not sure what to do about these special cases.

The 'list' case could probably be moved into ::query() (or even ::parseXML()), because it is generic.

The 'content']['languages' case is too specific though to be moved elsewhere.

+++ includes/mollom.class.inc
@@ -0,0 +1,1283 @@
+   * @todo Rename to createCaptcha().
+   */
+  public function getCaptcha(array $data = array()) {

That renaming suggestion still makes sense to me; getCaptcha() doesn't "get" a CAPTCHA image/audio file (also not using HTTP GET), it creates a CAPTCHA resource.

+++ includes/mollom.class.inc
@@ -0,0 +1,1283 @@
+    if (!isset($data['type']) || !in_array($data['type'], array('image', 'audio'))) {
+      // @todo Public method should not throw a MollomException.
+      throw new MollomException('Unknown CAPTCHA type.', 0, NULL, $this);

Just return FALSE here. (Same for other methods.)

Needs to be documented in @return.

+++ includes/mollom.class.inc
@@ -0,0 +1,1283 @@
+   * @todo List parameters.
+   */
+  public function getBlacklist($publicKey = NULL) {

I've to admit I didn't think about list parameters yet, since the Drupal module does not use them yet.

- 'offset' and 'count' can be passed in.
- 'listCount', 'listOffset', and 'listTotal' are returned (next to 'list').

The request parameters can be simple $offset and $count arguments to the method.

The list response parameters are a bit trickier though. Best idea I currently have would be to set them as public properties on the Mollom class, so in case you need them, your code looks like this:

$mollom = mollom();
$result = $mollom->getBlacklist($publicKey, 0, 50);
$list_size = $mollom->listCount;
$total_available = $mollom->listTotal;

Anyone any thoughts on that?

+++ includes/mollom.class.inc
@@ -0,0 +1,1283 @@
+   * @todo Combine into saveBlacklistEntry().
+   */
+  public function createBlacklistEntry(array $data = array(), $publicKey = NULL) {
...
+  public function updateBlacklistEntry(array $data = array(), $publicKey = NULL) {

The suggestion to merge these two methods still makes sense to me. There was a "larger" difference when we used HTTP PUT, but now that there's only GET and POST, the two methods are almost identical, and it doesn't really matter the term is inserted or updated, as long as it's there. ;)

+++ mollom.admin.js
+++ mollom.admin.js
@@ -8,12 +8,12 @@ Drupal.behaviors.mollomBlacklistFilter = {

The JavaScript changes need manual testing.

+++ mollom.drupal.inc
@@ -0,0 +1,326 @@
+  /**
+   * Mapping of configuration names to Drupal variables.
+   *
+   * @see Mollom::loadConfiguration()
+   */
+  private $configuration_map = array(
+    'publicKey' => 'mollom_public_key',
+    'privateKey' => 'mollom_private_key',
+    'servers' => 'mollom_servers',
+  );

Let's drop this mapping completely, add a database update, and simply use the setting names directly with a "mollom_" prefix; i.e., leading to:

variable_get('mollom_publicKey')
variable_get('mollom_privateKey')
variable_get('mollom_servers')

The case-folding bug in Drupal core might prevent us from using uppercase letters in the names; in that case, simply convert the $name to lowercase.

+++ mollom.drupal.inc
@@ -0,0 +1,326 @@
+  protected function request($method, $server, $path, array $data, array $expected = array()) {
...
+    // @todo The above is all that may vary between client implementations. All
+    //   of the below logic should live in a separate method that clients don't
+    //   have to re-implement:
+    //     handleResponse($http_code, $http_message, $response)
+    //   Or alternatively, since handleResponse() is required for every request,
+    //   move the above into a new sub-method invoked from request():
+    //     executeRequest($method, $server, $path, array $data)
+    //   and make it return $http_code, $http_message, array $response.

This is one of the primary remaining @todos we should resolve.

Clients should only have to implement the actual code/logic to execute a HTTP request to Mollom, but they should not have to re-implement the logic to handle and "interpret" the response. By moving that code into the Mollom class, we ensure a consistent behavior across all client implementations.

Working on it.

+++ mollom.info
@@ -4,8 +4,7 @@ core = 7.x
+files[] = includes/mollom.class.inc
+files[] = mollom.drupal.inc

Still not sure whether it would make sense to also move the Drupal implementation into includes. It's a nice example for how a real-world implementation should look like.

Still undecided, but rather leaning towards moving it into includes.

+++ mollom.install
@@ -119,19 +126,27 @@ function mollom_schema() {
+      // @todo Add Content API result 'reason'.
+      // @todo Add CAPTCHA API results 'solved', 'reason'.

Still need to add those.

+++ mollom.module
@@ -1391,11 +1357,12 @@ function mollom_process_mollom($element, &$form_state, $complete_form) {
+  // @todo Put the Mollom instance into $form_state.

Good idea ;)

+++ mollom.module
@@ -1804,141 +1775,29 @@ function mollom_form_submit($form, &$form_state) {
+function mollom($class = NULL) {
...
+    // @todo Testing mode configuration is not covered by tests.
+    if (variable_get('mollom_testing_mode', 0)) {
+      $class = 'MollomDrupalTest';
     }

One of the most weird @todos, but yeah, we don't have tests for the testing mode yet. ;) Leaving that for later though.

+++ tests/mollom_test.module
@@ -3,6 +3,16 @@
+ * @todo Extract testing server into a new mollom_test_server.module. The
+ *   mollom_test.module serves as good example for how to implement Mollom
+ *   support in a Drupal module, but 90% of it pertain to the testing server
+ *   now, so it's hard to explain people what they should look at.
...
+/**
+ * @defgroup mollom_test_xmlrpc Mollom XML-RPC fake server functions
+ * @{
  */

Now that the code is in 7.x-2.x, let's separate the testing server into a separate testing module.

While being there, also remove the obsolete XML-RPC stuff.

Updated issue summary.

I've resolved most of the todos outlined in #26 in the past days.

The issue summary contains a full list of summarized todos.

OAuth request signing is not pushed to 7.x-2.x yet, as I'm waiting for a couple of backend fixes.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Hi,

I've started playing around with the class. Implementation in a mollom.wordpress.inc class seems to be fairly easy. At the moment, I'm trying to get the verifyKey call to work. Anything beyond that should be fairly trivial.

I did get an error though: unknown function t() on line 604 in mollom.class.inc. Seems that there are four stray t() functions encapsulating a few strings which might get displayed either through Wordpress, Drupal or some other framework/app reusing the class.

I'm not sure about the saveConfiguration, deleteConfiguration and loadConfiguration functions. Handling of the public/private keycombo happens through the interface. The only use case I can see for those is caching the serverList. Unless you expect this to be extended to other values being (un)set in the future. Wordpress does have its' own get_option(), update_option() functions so no problems there.

The request() function is very useful. I've noticed Wordpress has improved the way HTTP calls can be made, so I'm now trying to get it to work with the wp_remote_get() and wp_remote_post() functions. So far, this looks promising, but I have to dig a bit deeper.

The best part of this approach is this: if the base class gets an update, as long as the abstract functions aren't touched, it should be just a matter of copy and paste without touching application specific code to keep things in sync.

Great work! :-)

While being there, also remove the obsolete XML-RPC stuff.

I've noticed that the verifyKeys call is geared towards the REST API while the query() method still uses the old XML RPC API. At the moment, the call redirects to http:///site/
instead of http://rest.mollom.com/v1/site/

Okay. I've gone through the class and the tests and removed the XML-RPC stuff:

* Cleaned out the query() method. This might need some revision. As it stands, the server response tests pass.
* Added a public $restEndpoint property with a reference to http://rest.mollom.com
* Removed the refreshServers() method
* Removed the getServers() method
* Removed the MollomRefreshException class
* Removed the MollomRedirectException class
* Refactored the MollomDrupalTest accordingly
* Added variable_set('mollom_testing_mode', 1); to MollomWebTestCase to get the tests moving

I first tried to make $restEndpoint a class const first, but failed because overriding const is, well, not that easy. We coud use get_called_class() in query() to retrieve the correct const definition, but this would require the method to be declared static.

Look at your trailing whitespaces with dreditor or another tool.

Upfront: Note that the most recent and stable code lives in the 7.x-2.x-maas branch currently. We'll merge that back into 7.x-2.x very soon.

I did get an error though: unknown function t()

Thanks for spotting those - I just removed them.

I'm not sure about the saveConfiguration, deleteConfiguration and loadConfiguration functions. Handling of the public/private keycombo happens through the interface. The only use case I can see for those is caching the serverList. Unless you expect this to be extended to other values being (un)set in the future. Wordpress does have its' own get_option(), update_option() functions so no problems there.

Yeah, that has been idea and purpose of these simple key/value configuration helpers. Drupal equally has its own configuration store, so each application-specific implementation of the Mollom class can simply invoke its own/existing functions.

I've noticed that the verifyKeys call is geared towards the REST API while the query() method still uses the old XML RPC API.

I'm a bit confused by that statement, since the most recent code contains nothing for the XML-RPC API anymore...?

The ->verifyKeys() method is a rather simple wrapper around ->updateSite(), passing client and version information to Mollom. It should be invoked when the API keys are added or changed for a site.

At the moment, the call redirects to http:///site/ instead of http://rest.mollom.com/v1/site/

Can you check whether you get a proper server list returned in the ->refreshServers() method?

Removed [...bunch-of-methods...]

I'm not sure why you attempted to do this? :) The client-side server fallback mechanism is still used in the REST API, and one of the major goals for the generic PHP class is to ensure the correct logic for that for all Mollom client implementations ;)

Your client should only have to implement the ->request().

Would love to learn why you thought that these methods were no longer needed. Perhaps we need to adjust or improve the documentation? We already added some simple example lines to README.txt, but potentially we need some more technical details there.

Oh. I didn't check out the 7.x-2.x-maas branch but the 7.x-2.x branch. :-o So, that's why. Those changes were made against the code that lives in the latter branch.

I checked http://mollom.com/api/rest to see if there were major changes compared to the XML RPC version. I didn't find any reference to the fallback system so I removed it from the class.

Take the Site API for instance: http://mollom.com/api/rest#site The docs say that one should POST a call to POST http://rest.mollom.com/v1/site/{publicKey} to verify the keys. The class in the 7.x-2.x branch assembled this URL: http://rest.mollom.com/v1/123.456.789.000/site/{publicKey} where 123.456.789.000 is the IP address of the first server on the retrieved server list, and tried to send a POST request to it.

I did understand that the verifyKeys() is a wraps updateSite(). :-)

Since the REST docs don't say anything about refreshing the servers, I assumed that all calls now go directly to http://rest.mollom.com and load balancing happens on the server side from now on.

Anyhow, I should checkout the 7.x-2.x-maas branch and use the code which lives over there. Or I could wait until the two branches are merged. Enough todo's on the Wordpress plugin where I don't need the class atm. ;-)

I'm tentatively calling this issue fixed for now.

We'll move the generic PHP class to github today or tomorrow. (Doing some last-minute adjustments today.)

+1 for moving to a library for better re-use.

The Mollom class has been extracted and pushed to https://github.com/Mollom/MollomPHP

Includes commit history. Some obvious minor clean-ups (e.g., readme) will happen shortly.

We'll also have to figure out how to manage some organizational things (e.g., versioning, testing, etc), but at least there's a unique origin now.