Backport caching from 7.x-3.x

function delete() of DBObject should clear the static cache no?

I believe what gagarine suggested needs to be implemented. After we delete something our loadAll caches become stale, and the cache for that individual EntityType/Bundle also.

+++ w/eck.classes.inc
@@ -97,24 +97,28 @@ class DBObject implements Iterator{
+    $call = get_called_class() . '--' . __FUNCTION__ . implode('_', func_get_args());

1. If we want to clear the static cache on save() and delete() then we need to clear all static caches related to a specific database table. Therefore I think we should keep one static per table, but make that static an array which is keyed by property and value. We could also key by "$property_$value", I have no opinion on that. In save() we could also compare old and new values (in the case of an update) and then only clear those (sub-)caches that have changed, but I think that would be overkill.

2. Even though this is only called from a static context in ECK, load() is not a static function. Therefore I think it is problematic to cache by the class name. If there is a single class that allows for $this->table to differ between instances, the wrong cache could be used.

Therefore I think we should use the 3 things that are actually dynamic and able to differ as identifiers for the cache key: $this->table, $property and $value. Per the point above IMO only $this->table should be part of the actual cache ID.

Because Drupal standards dictate that the cache ID should start with the function name, I suppose we should prefix the cache ID with "DBObject::load" (I find the "::" notation to be more standard than "--" but I don't have any strong feelings on that.)

Putting it all together I propose "DBObject::load:$table" (again, I don't really care about the delimiters much.)

I will roll a patch that implements that suggestion together with the clearing of the static cache on save() and delete().

The patch didn't apply to latest 7.x-2.x, so here's a straight re-roll. I noticed that this is assigned to @bastnic, so I hope I'm not stepping on any toes here. Anyway I'll be providing interdiffs along with the following patches, so in case @bastnic has worked on this in the meantime we should be able to incorporate everything together.

Here we go. I haven't tried this, but will do so now. This isn't the slickest code I've ever written, but I think it should work and IMO is a fairly simple solution to the problem.

Note for reviewers:

+++ b/eck.classes.inc
@@ -101,26 +101,27 @@ class DBObject implements Iterator{
-  protected function load($property, $value){
...
+  protected function load($property, $property_value){

Although not technically necessary I made this change because $value was used below in the foreach. It did not currently cause any errors but IMO this is generally a dangerous practice and I find it rather confusing.

All right. This one actually works. I had to quite a bit of plumbing, though. The static caching is now entirely handled in DBObject itself. EntityType and Bundle don't do any queries on their own anymore. DBObject now has a loadAll() method.

I had to make the $table (and the new $nameField) property static in the process. For loadAll() to be a static method there is no way around that.

Arghh, empty patch.

Hi

I have a problem using this patch and an entity type that doesn't use any bundle.

Notice: Undefined index: inline_images in get_bundle_crud_info() (Zeile 180 von /srv/www/ganske/htdocs/sites/all/modules/contrib/eck/eck.entity.inc).

I didn't debug this too far but "inline_images" are not using bundles. They are now displayed in the wrong view mode.

I tried ECK latest dev with and without the path. With the patch the viewmode is broken and I get tons of watchdog entries. Without everything runs fine.

I am not sure if this really is a bug in the code or if this messes up some configuration. But I believe this needs more work.

@tstoeckler Maybe we can develop a test how we can fix this. Lets chat about it.

Hi Kars-T,
I was going to start trying to debug the issue you mentioned, when I realised that ECKs UI becomes quite a bit useless when not using bundles. Are you creating these entities from code? How are you working with these entities?

I will go ahead and test the patch to make sure that things work well with the cases that ECKs UI encourages (entity types with at least one bundle).

I have a feeling that this patch might be somehow exalting issues of entity types without bundles, but I do not believe that those problems are directly related to this patch. All this patch should be doing is keeping us from touching the database multiple times in the same request, and the fixes that tstoeckler made should keep us from having stale caches in case that some Entity type/ Bundle configuration changes through the middle of the request.
I can see that ECK could have some issues with entity types without bundles since we save path configurations at the bundle level, but it is strange that this is affecting view modes.

I will report back once I get a little bit more time to look at the code, and try a few things.

Just a note, that I was trying to write some tests for the behavior Kars-T found, but got stuck in other fixes halfway.

public static function loadByEntityType($entity_type)

do problems with the patch,because
you load the results with

$results = $self->load('entity_type', $entity_type->name);

but this return only a array and no object.
The result is a lot of notices like "this is no object".

Version:
eck 7.x-2.0-rc1+25-dev

Here's a straight re-roll for now, the patch didn't apply to latest 7.x-2.x.
Now on to investigate #16 (and probably also #13 still).

@tstoeckler thank you for your work. A "complete" caching solution has been committed to 3.x that includes db caching on top of static caching.

I know 3.x will not be ready for while, but if we can test the solution and debug it, when we can roll it back to get the benefits in the 2.x branch.

No commit credit was given to you in the 3.x branch, but I will make sure to fix that when we commit the reroll to 2.x.

That looks quite good. With my patch I tried to consolidate the entire caching logic in the DBObject class, but that was trickier that I thought at first, which led me to rewrite half of the eck.classes.inc file, ... :-/

One thing I did notice is that I don't think the cache gets invalidated on save()? On delete() I see a drupal_flush_all_caches(), but nothing similar on save(). I might have missed something.

Anyway, no on to backport that code for 7.x-2.x.

Now that this has been fixed in 3.x don't know if you want to demote this to "normal", but leaving for now.
Here's a backport from 3.x. I just did a diff between the entirety of 2.x and 3.x and just removed everything that I thought was unrelated. I didn't test this yet.
I saw that instead of handling the cache clear directly in save() you do that in the form submit, which I guess should work just as well. I suppose it's a possible gotcha for people using the API but I guess ECK is all about the UI, so ... :-)

Will now test this.

EDIT: Added important missing letter :-)

Trivial but fatal oversight.

Even though it doesn't seem like it, I would like to think of ECK as important because of its APIs, so I think that the cache invalidation should happen in the classes, not in the form submit functions.

I opened #1979658: Clear caches in EntityType::save() and Bundle::save() instead of the form submit handlers for that and will post a patch there soon. If you don't mind I would like to keep this backport issue separate. Changing title for that purpose. See you over there :-)

Rerolled patch on top of #1979658-4: Clear caches in EntityType::save() and Bundle::save() instead of the form submit handlers.

Oops, empty patch file...

This now includes #1977336: Notice: Trying to get property of non-object in Bundle->save() (line 377 in order to fix adding bundles from the UI. Specifically, the patch in #1977336-5: Notice: Trying to get property of non-object in Bundle->save() (line 377 is the interdiff.

Btw, I also tested that the caching actually works. 1000 calls of EntityType::loadAll() are 10 times faster with this patch while 1000 calls of Bundle::loadAll() are more than 100 times faster.

Awesome to know!

Thanks a bunch for the backport. I will test it out this week and hopefully get it committed.

Has anyone run into this error while using the backport? Just curious, I haven't tested it out yet. It may be a blocker that will need to be sorted out before committing.

#1973104: Indirect modification of overloaded property in ECKEntity class

is ECKEntity coming with the cache backport? why?

No there is no caching for ECKEntity objects, but the issue listed in #29 is affected by that changes made for cacheing in EntityType and Bundle. At least I have a hunch that it is, and is why I posted the question above. It would be good to make sure before committing.

Ah! I just re-read you comment @fmizzell. Yeah my mistake, we don't have to worry about the issue I mentioned in #29 above since the ECKEntity class is not being backported.

Rerolled. BTW someone needs to run this nodule through Coder..

This fixes coding standards violations in the lines affected by the patch, but there's a lot of other work to be done to bring it up to shape.