Convert $conditions to use EntityFieldQuery then load by ID from cache

Those will not be retrieved from cache.

You should avoid using $conditions (it's deprecated), use EntityFieldQuery instead, then load via the IDs retrieved from there.

I've been considering changing Entity Cache to do this internally but have not got to this yet, however I'll re-purpose this issue to a feature request for that.

I just got burned by this.

I have a site which had a menu wildcard loader (which is the main way of loading content on this particular site) which used node_load_multiple with some conditions over $node->type to make sure it would only get the proper type.

These particular nodes have roughly 25 fields each.

I noticed on a single node page, I was getting roughly 900 queries per page, which seemed odd. Turns out the node was getting loaded about 22 times per page Multiplied by 25 fields ~550 queries.

When I turn off, EntityCache, this drops down to roughly 400. This is because EntityCache, on a miss seems to load all the fields individually and doesn't cache them, while DrupalDefaultEntityController uses cache_fields table. Perhaps an improvement could be made here, so EntityCacheControllerHelper is no worse than DrupalDefaultEntityController.

Then I removed the use of node_load_multiple() this dropped to ~200.

So this is a very dangerous small little bug, which a lot of people could be suffering from. Yes we should stop using conditions as it appears to be deprecated...but if we do with EntityCache, at the very least, it should perform no worse than DrupalDefaultEntityController.

There is a slow example in core that would profit majorly by this:

https://api.drupal.org/api/drupal/modules%21taxonomy%21taxonomy.module/f...

Here is some basic work. I am not 100% sure whether I got the conditions for the entity query branch properly, but at least some
manual testing worked as expected (both with and without passed in IDs).

I managed to fix all the test exceptions but not the test fails.

The problem is that the test calls toentity_load()
with the same $conditions twice, while using just drupalPost()for updating the entity.

Based upon that its not really possible to update the static caching in the context of the test, and the test fails.

In a normal API case $controller->resetCache()is called when saving an entity,
so this is not a bug in real life.

Attached is a patch for core which would help, but also a patch which uses the subclassing of the PageEditTestCase
to reset the static cache via hacking drupalPost().

I thought about this a day and I am okay with hacking the drupalPost seeing as it is really just limited to that class and we really want to run the upstream tests.

RTBC

1. +++ b/entitycache.module
   @@ -77,6 +92,42 @@ class EntityCacheControllerHelper extends DrupalDefaultEntityController {
   +      // ($entities is set), we don't want to query again.
   

   How do we know that $conditions isn't different each time? And how do we know which entities to return when $conditions is passed? Right now it looks like it'll return all of the statically cached IDs regardless of when they were loaded?

   I'd expect to see static tracking of EntityFieldQuery results (i.e. array of entity ids from $result[$entity_type] with a hash of $conditions + $ids as the key - then we can know which $ids to return each time and whether the specific query was already run or not.

2. +++ b/entitycache.module
   @@ -77,6 +92,42 @@ class EntityCacheControllerHelper extends DrupalDefaultEntityController {
   +      $query = (new EntityFieldQuery());
   

   Why the extra parentheses?

Thank you for you review!

How do we know that $conditions isn't different each time? And how do we know which entities to return when $conditions is passed? Right now it looks like it'll return all of the statically cached IDs regardless of when they were loaded?

That bit is a bit tricky, but I think we don't need it at the moment, even with the approach of having a $conditions hash we would gain a bit of performance.
At the moment DrupalDefaultEntityController::cacheGet()implements static "querying", so if you pass in the same $conditions
twice it will in the first run execute the query, and in the second run, use all the statically stored entities "query" them in PHP
and then return all of them.

The needed test fix in https://www.drupal.org/files/issues/interdiff_8082.txt kind of prooves that the static caching for itself works.

Why the extra parentheses?

Well, I first thought about using a chainable code, but it turned out that the parentheses looked ugly, so I decided against it, but kept them :) That is fixed now.

At the moment DrupalDefaultEntityController::cacheGet()implements static "querying", so if you pass in the same $conditions
twice it will in the first run execute the query, and in the second run, use all the statically stored entities "query" them in PHP
and then return all of them.

Sorry I completely forgot that this existed, despite having written it :(. However while we get the loaded entities from cache where possible, doesn't the default controller still query for other entities based on the fact it can't know whether they were all loaded or not?

The situation I'm thinking about is:

entity load 1 loads entities a, b, c and d by ID.

entity load 2 with conditions will return a, b, c, e and f. It's the e and f that I can't figure out in relation to the patch. The static cache 'querying' in the default controller does ensure that d doesn't get returned, but it can't help with e and f.

+++ b/entitycache.module
@@ -77,6 +92,42 @@ class EntityCacheControllerHelper extends DrupalDefaultEntityController {
+        return static::entityCacheLoad($controller, $entity_ids);

There's an issue here with not correctly passing down the $revision_id argument into the self-referential static::entityCacheLoad() call. It's stripped out earlier in the function since the interface provides no facility for revision-keyed caching, but it's not reintroduced as it should be in this call. Returned results will be incorrect. (I've fixed this in my patch.)

The situation I'm thinking about is:

entity load 1 loads entities a, b, c and d by ID.

entity load 2 with conditions will return a, b, c, e and f. It's the e and f that I can't figure out in relation to the patch. The static cache 'querying' in the default controller does ensure that d doesn't get returned, but it can't help with e and f.

Yep, you're absolutely right. It cannot be known from where this code is whether the result set returned from the static cache "query" would be the same as the set returned from the database, so it's still necessary to do the EFQ.

I've refactored this patch in a way that I think satisfies this requirement and makes it a bit clearer. Rather than enmeshing ourselves in a big if statement by reconciling the cache result set with the potential db result set after static cache retrieval, I've moved the EFQ construct up before static cache retrieval. Now, if there are ANY conditions (apart from $revision_id), then we perform the EFQ in order to create the complete and exact list of required ids, then re-enter the method with that list and no conditions (or just the $revision_id).

for fun bonus points, it's basically a mapping of f(ids, conditions) -> f(ids), which more or less means we've created a partial application of the function...bwahaha.

Let's see what the testbot has to say.

Needed to pass the keys of $passed_ids, not the values

...it's scary that tests didn't fail on this earlier, as this was wrong before.

i have no idea wtf is going on with that polljs test

derp derp derp. left in a print_r(), which should explain why an ajax test got all bruised up.

I like the partial application and converting one function in another :D.

RTBC - Looks fantastic, great work!

Committed/pushed to 7.x-1.x, thanks!