cache_set should not accept number of seconds as a valid expires-value

BTW: In most cases we don't notice this bug because if a cached item doesn't exist, we simply regenerate it. Mollom is using the cache a little differently, and if the cached item doesn't exist it can't validate the session_id and the Captcha validation fails.

Well, this looks good however storing any data in cache that can't be regenerated is a bad idea. We learned our lesson with update status and form cache really should not be a cache either. Still: this patch is good.

This is the first time I've looked at the Mollom code, but I believe they're doing things right here. The content stored in cache_mollom can and is regenerated as necessary -- but it does require that the cached session can live in the cache long enough for someone to submit a comment, enter their login information, whatever. It was a memcache bug that was causing this content to be "flushed" instantly, breaking Mollom. When this happens, Mollom generates a new session and tries again, as it should using a cache_* table. With the patch attached above, things work as expected -- if the cache is flushed (ie memcache is restarted) Captcha validation will fail and the user will just think they misread the annoying cryptic letters that are Captchas and try again, then it will work...

I believe this is also causing this bug:
#533524: High MySQL queries for 6.x-1.3 vs. 6.x-1.2

The previous patch fixed items that had an expiration defined in seconds, but it left CACHE_TEMPORARY still broken. The attached patch fixes both.

Without my patch I can duplicate the Mollom bug, and I can duplicate the page cache issue in that the page cache is rebuilt far too frequently. With the above patch, Mollom works and the page cache no longer is flushed artificially. I will commit this and roll a 6.x-1.4-rc1 to encourage further testing.

Fix applied:
http://drupal.org/cvs?commit=246944

Reopening.

cache_set() does not take a number of second as the $expire parameter, but only a timestamp. Let's not introduce hacks in memcache for my own programming errors.

See #548078: Mollom should call cache_set() with a timestamp, not a number of seconds.

As Damien noted, this behavior is inconsistent with Drupal's own cache_set().

#548078: Mollom should call cache_set() with a timestamp, not a number of seconds has been committed to CVS, so I don't think there's any reason to keep this extra feature around :)

Memcache supports either method of specifying the cache lifetime. The current memcache module code is still 100% compatible with Drupal core, while also exposing an extra memcache feature. I don't see any problem with this.

(The only reason this looks like a "hack" is because we've added cache stampede protection where we are handling cache expiration rather than memcache... but that "hack" can be quite a scalability boost.)

Automatically closed -- issue fixed for 2 weeks with no activity.

Attached a backport of this patch to 5.x version, as this issue was introduced there too.

#11: It is not 100% compatible with Drupal core. It extends the API and makes it possible to make code that works correctly with your specific cache_set implementation, but not necessarily the one in Drupal core. Granted, it is a minor issue, but why support something that Drupal core doesn’t, if that only serves to make the resulting code non-portable?

By that argument why do we have any contrib modules, as they clearly provide functionality that's not offered by Drupal core.

You can keep your code 100% compatible with Drupal core if that is your requirement. But you should also be able to tap into memcache features not available to core if that's what you need.

#15: We're not talking about new functionality here. In this case, a documented part of Drupal's API changes behaviour when your module is installed. If other modules changed the behaviour of the common API methods we all used, the Drupal ecosystem would be total chaos.