[D8][PS] Integrate the functionality of the entity_cache module into core

[jenlampton]

...but not as a module... just as part of Backdrop, maybe part of the entity system?

---

Advocate for this issue @hosef

---

Weekly update?

---

PR by @hosef: backdrop/backdrop#2927
Change record: https://api.backdropcms.org/change-records/entity-caching-added-to-core

[damienmckenna]

Comparable D8 issue, for reference: https://drupal.org/node/597236

[duckzland]

+1 for entity cache

[matt2000]

Why shouldn't this be a module?

[quicksketch]

This makes sense being part of core (and always enabled) because entity caching is a level of cache that is reliable and easily cleared and updated automatically when corresponding entities are updated. As long as the appropriate APIs are used (e.g entity_save(), node_save()), adding entity-level caching means that we can save database queries not only for fields, but also for entries in other tables, such as the webform* tables for Webform, path redirects, metadata, or anything else that's not a field. Adding the entity-level cache also means we could remove the field-level cache, since at that point they'd be redundant.

[matt2000]

I agree that it should be in core and enabled by default, just not that it should be something other than it's own module. It can be hidden if you want, but it should still be replaceable by developers.

I don't agree that it is redundant with field level cache. I'm pretty sure the reasons are covered adequately in the D8 issue discussion.

[quicksketch]

I don't agree that it is redundant with field level cache. I'm pretty sure the reasons are covered adequately in the D8 issue discussion.

I couldn't find explanations in the D8 issue for why the field cache and entity cache are both necessary. I can't figure a reason why both are independently needed, other than if you could disable the entity cache then you'd want the separate field cache. However, entire-object caching makes sense to always have enabled because there's no downside for end-users. There's no risk of stale data because we know when the node/user/term is being updated (via entity_save()) and can clear the cache. It's similar to the filter caches that can't ever be disabled. If there's no risk of stale data, the cache should always enabled.

[damienmckenna]

D8 has also changed to storing all field data with the entity bundle itself, rather than as independent objects, so it makes sense to cache the full objects. There's a discussion in https://drupal.org/node/597236 that may be helpful.

[matt2000]

#74

Converted just enough to eliminate WSODs and get the tests to run. I'm waiting to see if the test pass, but wanted to share early.

[quicksketch]

This is a good feature for Backdrop for certain. If it's possible to add to a 1.x minor release let's still try to have this as a replacement for field cache, but if not, I'd love to see this in the 2.0 release.

[hosef]

I think I have everything done for this except for one test case that is failing. I have spent a while trying to debug it, and I have no idea why it is failing.

[docwilmot]

@hosef I'm curious; the test fail is $this->assertEqual($url, $node->{$this->field_name}['und'][0]['url']);. Does $node actually exist at that point, and is it the $node you expect?

[hosef]

@docwilmot The node does exist, but it looks like there isn't a field on the node. When I put a debug message in there to print out the node, then it will be missing the field property entirely.

[docwilmot]

But is it the node you expect (should be NID == 3) or is it one of the default nodes from the install profile?

[hosef]

The nid is 1. The tests don't use the default profile, so there is no content unless you make it as part of the test.

I modified the test to add a couple of debug statements, so now it is as follows:

    $test = db_query("SELECT * FROM {field_data_{$this->field_name}}")->fetchAll();
    debug($test);

    $nid = db_query("SELECT MAX(nid) FROM {node}")->fetchField();
    $node = node_load($nid, NULL, TRUE);
    debug($node);
    $this->assertEqual($url, $node->{$this->field_name}['und'][0]['url']);

The output from the first debug is:

array (
  0 => 
  (object) array(
     'entity_type' => 'node',
     'bundle' => 'page',
     'deleted' => '0',
     'entity_id' => '1',
     'revision_id' => '1',
     'language' => 'und',
     'delta' => '0',
     'field_wdnn9v1r_url' => 'javascript:alert("http://example.com/")',
     'field_wdnn9v1r_title' => 'w87pVUF8',
     'field_wdnn9v1r_attributes' => 'a:0:{}',
  ),
)

And the second is:

Node::__set_state(array(
   'nid' => '1',
   'vid' => '1',
   'type' => 'page',
   'langcode' => 'und',
   'title' => 'Simple title',
   'uid' => '2',
   'status' => '1',
   'created' => '1570843057',
   'changed' => '1570843057',
   'scheduled' => '0',
   'comment' => '0',
   'promote' => '0',
   'sticky' => '0',
   'tnid' => '0',
   'translate' => '0',
   'revision_timestamp' => '0',
   'revision_uid' => '0',
   'in_preview' => NULL,
   'log' => '',
   'body' => 
  array (
  ),
   'name' => 'qUOPSXYE',
   'picture' => '0',
   'data' => NULL,
))

So, we know that there is a node and there is data in the field table, but for some reason the field data is not being attached to the node when loading it.

[docwilmot]

Had a look at the PR. The fail happens because of the call to $reset in $node = node_load($nid, NULL, TRUE) (ie the TRUE). So in DefaultEntityController::resetCache() if you comment out the line cache_flush('cache_entity_' . $this->entityType) the test goes green.

Thats the reason for the fail, but cant tell you the fix; hopefully this is helpful.

[docwilmot]

See the PR itself for the review.

[jenlampton]

Ah, sorry @docwilmot I was reading this issue which is still labeled pr - needs code review and has not had a comment since October. Adjusting labels accordingly.

[quicksketch]

I updated the PR based on @docwilmot's review. Thanks for that!

I also found new calls to cache_clear_all() that should be replaced with the newer and more explicit cache() function instead.

As this could have far-reaching consequences I don't think we should rush it into 1.15, but I'm marking this RTBC for 1.16! We can merge it shortly after branching 1.15.x (which will happen Jan 15).

[hosef]

I updated the documentation for hook_entity_info() in entity.api.php so that it shows the new entity cache key and I made the documentation for field cache easier to understand since it could be read multiple ways before.

[klonos]

I also found new calls to cache_clear_all() that should be replaced with the newer and more explicit cache() function instead.

Are we deprecating cache_clear_all() over cache() in general, or is it only specific use cases? Either way, we should be documenting when this is advised, and/or throwing warnings for developers to change occurrences of cache_clear_all() when appropriate.

[quicksketch]

Are we deprecating cache_clear_all() over cache() in general, or is it only specific use cases?

Yes, I think we should. There is an issue at #2158.

[quicksketch]

It's now been a few weeks since 1.15.0 came out and we've branched for 1.15.x. I've merged backdrop/backdrop#2927 into 1.x for 1.16.0! Thanks so much @hosef for taking the lead on this and really following through! Super awesome!

Thanks @matt2000 for the earliest work. Thanks @docwilmot and @oadaeh for your feedback and reviewing!

[quicksketch]

I'm reopening this until we have a change-record to document how it works and what it changes. In particular describing the way that things like the "new" attribute on comments now has to be set after the cache is loaded, rather than being set in the database loading function.

[jenlampton]

I was going to take a stab at creating a change record for this, but I'm afraid I don't really know how to summarize what changed. I was going to copy what went into D8's change record, but apparently they don't have one either.

[oadaeh]

@hosef told me he was going to create a change record for this, since he believed it was not a big deal. I'll just continuously nag him until he gets it done. :)

[klonos]

@hosef added a draft change record here: https://api.backdropcms.org/change-records/entity-caching-added-to-core

[quicksketch]

Looks good! I edited it slightly and added a section on the load() method of storage controllers.

It's now published. Thanks @hosef!

[jenlampton]

So, this issue can be closed now?