If both extensions are installed, setting a cache item causes the module to try with both the new and old extension APIs. Because the API in use on an instance should depend on the object type (not what extensions are installed), I've updated the code to use the instanceof operator, which is more appropriate.

The factory function still checks which extensions exist and prefers Memcached.

Comments

david strauss’s picture

I marked this "major" instead of "critical" because you can work around the problem by only having one extension installed.

longwave’s picture

Priority: Major » Normal

The ->set() code was already changed in #837758: Memcached::set() expects at most 3 parameters, 4 given in dmemcache.inc on line 42 so the original problem no longer exists and the patch no longer applies. Using instanceof rather than class_exists is technically more appropriate but I can't see how it will ever cause a problem.

catch’s picture

Title: Conditions for certain operations fall through improperly when both PECL memcache and memcached are installed » Allow a choice of extension (and default to memcache due to bugs in memcached)
Version: 6.x-1.5 » 6.x-1.x-dev
Status: Active » Needs review
StatusFileSize
new4.42 KB

Slightly re-purposing this, started on a patch at #1131454: Default to Memcache extension but the code here is a prerequisite to making that work. Re-rolled with the changes from there, and the instanceof change from here.

catch’s picture

StatusFileSize
new4.61 KB

Added a @see for the memcached bug.

jeremy’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Needs review » Patch (to be ported)

I tested this with both extensions enabled together, and extension enabled individually. When testing the override functionality I wrote 'memcached' with a lower case 'm' and ran into problems -- so added logic to force only the first character upper case and the rest lower. I also updated the README:
http://drupalcode.org/project/memcache.git/commit/11511cd

Needs to be ported to Drupal 7.

catch’s picture

Status: Patch (to be ported) » Needs work

This introduced a regression, my fault #1133964: Memcache isn't connecting to all servers fixes it and that needs to be included here before we can commit, so marking CNW.

catch’s picture

Status: Needs work » Fixed

Ported all this to 7.x

Status: Fixed » Closed (fixed)

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