Mollom cache is not cleared when a form is unprotected

Also, because mollom_form_cache() caches in the database instead of a static variable, it would be more elegant to have an explicit mollom_{set|get|clear}_cache() functions instead of cramming it in one function. No reason to put everything in one function with a less intuitive API.

@David: Thanks! I'll work out some tests. Adding a mollom_form_delete() is also a good idea.

@Dries: We can surely add a separate mollom_form_cache_reset(), but aside from that, a mollom_form_cache_set() would not make sense, since the cache data is auto-generated, so there is only a _get() and _clear(). So not sure I follow - to my knowledge, there is no situation in which we would want to clear the static cache or database cache only. While the database cache is required (as the code path to build the lists is relatively expensive), the additional static cache exists as performance optimization only; i.e., to not re-retrieve the identical data from cache during a single request.

But speaking of, I'm not happy with that manually crafted db/static cache pattern we introduced for D7, so I actually just happened to suggest #924616: Make database cache leverage static cache by default today -- which would allow us to get back to simple invocations of cache_get(), cache_set(), cache_clear_all() in almost all locations where a static cache wraps a cache_get() -- from which there are plenty.

mollom_form_cache() does not use static caching. That said, if we don't have a _set(), it might be better to keep it in one function. Sorry for side-tracking us.

Looking at the code, I'm not sure the function name makes sense:

 * Returns a cached mapping of protected and delete confirmation form ids.
 * ... snip ...
 */
function mollom_form_cache($reset = FALSE) {

The fact that the function caches isn't really that important, and could probably be omitted from the function name. This is illustrated by the fact that the function's PHPdoc doesn't even mention the cache. See what I mean?

mollom_form_cache() itself does not use a static, but mollom_form_alter() uses one for the returned database cache. That is, because mollom_form_cache() is invoked form other places, which don't necessarily need a static cache, as they only retrieve the data once. To keep it simple, we could change that -- though not contained in attached patch(es) yet.

The patch ending in "fail" only contains the tests, to make sure the tests are actually catching the bug.

Great -- I like the new approach in #4 and committed it to CVS HEAD. I guess we'll want to backport this to Drupal 6 so updating the status.

Since there is no drupal_static() in D6, the patch looks slightly different. That said, it might make sense to change HEAD accordingly, after fixing the bug in D6.

As there is also no assertUrl() in D6, that is also different. (btw, I wrote the new assertUrl() in D7 based on Mollom's tests ;)) I hope this patch passes.

Committed to DRUPAL-6. Thanks.

Feel free to hate me for this, but I'd really like to keep branches in sync. :)

Committed to HEAD.