Introduce caching of certain salsa entities

1. +++ b/salsa_entity.inc
   @@ -262,7 +263,57 @@ class SalsaController extends EntityAPIController implements EntityAPIController
   +
   +    // Check whether entities of current could be cached in the DB.
   

   of current.. missing part of the sentence?

   Also, "in the DB" is not necessary, might also be in memcache or so.

2. +++ b/salsa_entity.inc
   @@ -262,7 +263,57 @@ class SalsaController extends EntityAPIController implements EntityAPIController
   +      // Gather the cache ids to use cache_get_multiple (faster).
   

   function reference should use ()

3. +++ b/salsa_entity.inc
   @@ -262,7 +263,57 @@ class SalsaController extends EntityAPIController implements EntityAPIController
   +          list (,,$id) = explode(':', $cid);
   

   coding standard is " , , $id" I think.

Basically, the previous patch was dropped because of the way we use variable_set / variable_get('salsa_entity_mock_max_attendee') in salsa_event.test and salsa_entity.mock.inc. Once the object is cached, further setting of salsa_entity_mock_max_attendee has no effect on the loaded object, so the only way to overcome this I see in flushing cache in tests or to write better tests.

In any case, my patch is based on the previous one, with a few improvements (please review).

I'm not sure for which entities to enable caching (I followed patch-1), maybe a good idea may be to list all entities in the backend and to allow to users to select entities they want to cache and to set cache lifetime? In any case, there will be more work probably, please specify...

When working on patches, make sure that you provide an interdiff so that it is easier to review the changes. Otherwise I have to search for the changes that you made.

I noticed you introduced a new cache bin, I guess to make invalidations easier. Makes sense. However, we should also implement resetCache() in the entity controller and support to invalidate specific entities or all of them.

And if we update an existing entity or delete it, we should then call $this->resetCache(array($id));

+++ b/salsa_entity.inc
@@ -262,7 +263,56 @@ class SalsaController extends EntityAPIController implements EntityAPIController
+      // Collect the cache ids to use cache_get_multiple() faster.
+      foreach ($ids as $id) {

Reads a bit strange now, you can leave faster away.

Once you have resetCache() support, you should use that in the tests instead of the low level function, e.g. entity_get_controller('salsa_something')->resetCache().

And no, this does not need to be configurable, at least not in a first issue.

What would be nice however is to have support for loading all entities of a given type. We use that in a few places, for example for loading salsa groups.

To do this, you need to detect the case when $ids === FALSE. In that case, do a cache get for salsaType:ALL or something like that, which should contain a list of salsa keys, then switch out $ids with a list of those, which should result in them being loaded from the cache if present. Only do this is if caching is enabled for a given type, with the same expiration.

And before returning, if $ids was FALSE, write it into the cache if we had a cache miss before.

Please check resetCache() implementation. There are warnings in tests that currently have no idea how to solve them.

What would be nice however is to have support for loading all entities of a given type. We use that in a few places, for example for loading salsa groups.

To do this, you need to detect the case when $ids === FALSE. In that case, do a cache get for salsaType:ALL or something like that, which should contain a list of salsa keys, then switch out $ids with a list of those, which should result in them being loaded from the cache if present. Only do this is if caching is enabled for a given type, with the same expiration.

And before returning, if $ids was FALSE, write it into the cache if we had a cache miss before.

In relation to this, to implement this case in the query() or cacheGet() method? If we look at EntityAPIController::load(); it seems that the parent method works approximately as you want, now when we have cacheGet() and cacheSet() methods implemented?

1. +++ b/salsa_entity.inc
   @@ -112,6 +112,16 @@ class SalsaController extends EntityAPIController implements EntityAPIController
   +    $cache_ids = array();
   +    foreach ($ids as $id) {
   +      $cache_ids[] = $this->getCacheId($id);
   +    }
   +    if (!empty($cache_ids)) {
   +      cache_clear_all(array($cache_ids), 'cache_salsa_entities', TRUE);
   +    }
   

   Do not call this directly, that's why you added resetCache(), call that instead.

2. +++ b/salsa_entity.inc
   @@ -223,6 +233,24 @@ class SalsaController extends EntityAPIController implements EntityAPIController
   +    // Update persistent cache.
   +    $cache_ids = array();
   +    if (isset($ids)) {
   +      foreach ($ids as $id) {
   +        $cache_ids[] = $this->getCacheId($id);
   +      }
   +    }
   +    if (!empty($cache_ids)) {
   +      cache_clear_all(array($cache_ids), 'cache_salsa_entities', TRUE);
   +    }
   

   You have too much array nwsting, $cache_ids is already an array.

   You also need to handle the case where $ids is NULL, so the else case of that IF. In that case, you need to do a wildcard cache clear that starts with the salsa type, so that *all* ID's are invalidated.

   The existing code all needs to happen inside the existing if (isset($ids)).

Please check now, interdiff should be ok.

Submitting full patch. As I tested, there is no failed tests, but it is evident that patch causes exceptions

addcslashes() expects parameter 1 to be string, array given Warning database.inc 984 DatabaseConnection->escapeLike()

I think it's related to how we use the wildcard in cache_clear_all(), It is logical that cache_clear_all($cache_ids, 'cache_salsa_entities'); do its job without wildcard set, but in that case cached entity object is not cleared.

Please check in more details.

1. +++ b/salsa_entity.inc
   @@ -122,9 +122,12 @@ class SalsaController extends EntityAPIController implements EntityAPIController
   +
   +    // Update caches.
   +    $this->resetCache(array($ids));
   

   You still have too many arrays here. $ids is already an array, you are creating an array of arrays.

2. +++ b/salsa_entity.inc
   @@ -223,13 +226,33 @@ class SalsaController extends EntityAPIController implements EntityAPIController
   +      cache_clear_all($cache_ids, 'cache_salsa_entities', TRUE);
   

   This shoudn't need the wildcard argument?

1. Done.

2. I think so, but in that case event test will fail, seems to me as cache is not cleared ... in any case it is wrong to satisfy tests in this way.

Events test improved.

Ok, I decided go ahead and commit this. We don't have too much test coverage here, but we need to do manual testing with this anyway.

Opened #2480211: Support caching of entity_load('salsa_group') for the load all caching idea.