Radioactivity checks class_exists("Memcache"), but this is FALSE when using the Memcache module.

The attached patch also checks $conf['cache_default_class'] == 'MemCacheDrupal').

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tcmug’s picture

Status: Needs review » Needs work

This is most likely caused by the environment not actually having PHP Memcache installed (different than Memcached).

I'd actually fix this by making it possible to either class.

moehac’s picture

I'm only running Memcached, so I made the following change. You might want to consider looking to add support for Memcache and Memcached.

Index: includes/RadioactivityMemcachedIncidentStorage.inc
===================================================================
--- includes/RadioactivityMemcachedIncidentStorage.inc	(revision 1300)
+++ includes/RadioactivityMemcachedIncidentStorage.inc	(working copy)
@@ -17,8 +17,9 @@
    * Connect to the memcache server
    */
   private function connect() {
-    if (class_exists("Memcache")) {
-      $mc = new Memcache();
+    global $conf;
+    if (class_exists("Memcache") || $conf['cache_default_class'] == 'MemCacheDrupal') {
+      $mc = new Memcached();
       $mc->addServer(VAR_RADIOACTIVITY_MEMCACHED_HOST, VAR_RADIOACTIVITY_MEMCACHED_PORT);
       if (@$mc->connect(VAR_RADIOACTIVITY_MEMCACHED_HOST, VAR_RADIOACTIVITY_MEMCACHED_PORT)) {
         return $mc;

infines’s picture

Priority: Normal » Major
Status: Needs work » Needs review

These fixes work for me! However the original patch and #2 need to be rolled into a single patch. I don't know how to do this, otherwise I would!

androidi’s picture

Here's a patch, which works both on PHP Memcache and Memcached modules.

infines’s picture

Status: Needs review » Reviewed & tested by the community

working for me

tcmug’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! Committed to be released soon.

Status: Fixed » Closed (fixed)

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

rv0’s picture

Status: Closed (fixed) » Needs work

If you do a simple egrep -R "memcache" * in the module folder, you'll notice there's plenty of other places that check for Memcache class that need to be adapted.
E.g. to select memcached incident storage on a decay profile.

If I finally get my setup working, I'll provide a patch, but I'm still struggling with other issues atm..

rv0’s picture

Status: Needs work » Needs review
FileSize
3.78 KB

Ok, so I'm digging deeper now.

I got

Error: Call to undefined method Memcached::connect() in
/home/beveren/apps/default/sites/all/modules/contrib/radioactivity/includes/RadioactivityMemcachedIncidentStorage.inc, line 24

Reading http://stackoverflow.com/questions/11926376/memcache-connect-vs-addserver
it seems the "connect" isn't needed, and from the looks of it, even bad practice when you have multiple memcache servers.

Following patch removes the connect function, and borrows some logic of memcache_storage module.
Also fixes issues mentioned in #8
Seems to work just fine for me.

RobKoberg’s picture

In RadioactivityIncidentStorage there is a space missing in the method:
public function addIncident(RadioactivityIncident$incident) {

Then in RadioactivityMemcachedIncidentStorage there needs to be:
public function addIncident(RadioactivityIncident $incident) {

To get rid of compatibility warnings.

rv0’s picture

@RobKoberg
Agreed, but that didn't cause any issues for me.

tcmug’s picture

I need your help man! I've committed this to the repository - can you please verify everything is OK regarding this as I haven't got an environment where I can test the bug(s).

I also changed the missing space issue mentioned in this issue thread.

rv0’s picture

@tcmug
Since I submitted the patch in #9, we are running it on a live website with plenty of traffic and it appears to be working very well.
I will post back here if I notice any issue.

Thanks for this excellent module!

q0rban’s picture

The patch from #9 does indeed appear to make the error go away! Nice work. It introduces some Drupal whitespace and coding standard issues, and unnecessary logic. Attached patch simplifies the connect() method, cleans up the whitespace issues, and follows Drupal coding standards.

q0rban’s picture

One other thing to note is that in #9 and #14, the precedence of Memcached over Memcache was switched (HT @e0ipso for catching this). I'm not sure if this was intentional in #9, or even something we necessarily care about, but I thought I'd mention it.

tcmug’s picture

Status: Needs review » Closed (fixed)

Lets close this. Reopen if needed for memcache vs. memcached.