Regression: Following taxonomy entity conversion, Taxonomy EFQ causes warnings and notices

Is it possible that this is also related to the entity_load() rename. Something is passing invalid arguments to entity_load_multiple(). Try adding a "debug(debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS))" right before the test exception happens, that should lead you right to place where this is happening.

An interesting note, if you debug($results); in the working state (before the entity conversion), the result is an array of stdClass objects with only tid set.

Currently in 8.x, EFQ still only returns the tid, but it now returns an array of TaxonomyTerm objects. And since it only passes in the tid to the constructor, vital things like vid and vocabulary_machine_name are set to NULL.

Updated issue summary.

There is NO WAY we should do this.
Just proving that it works.

This reverts entity_create_stub_entity to the D7 version of it, aka before #1184944: Make entities classed objects, introduce CRUD support.

Hm, this seems like it might even be the right fix.

For the entity types that have been converted (everything except file), EFQ gives completely different results than D7.

Given

$query = new EntityFieldQuery();
$query->entityCondition('entity_type', 'node');
$query->range(0, 1);
$result = $query->execute();
var_dump($result);

Here is the result of D7:

array(1) {
  ["node"]=>
  array(1) {
    [101]=>
    object(stdClass)#62 (3) {
      ["nid"]=>
      string(3) "101"
      ["vid"]=>
      string(3) "101"
      ["type"]=>
      string(4) "page"
    }
  }
}

And here is D8:

array(1) {
  ["node"]=>
  array(1) {
    [352]=>
    object(Drupal\node\Node)#48 (18) {
      ["nid"]=>
      string(3) "352"
      ["vid"]=>
      string(3) "352"
      ["type"]=>
      string(5) "event"
      ["langcode"]=>
      string(3) "und"
      ["title"]=>
      NULL
      ["uid"]=>
      NULL
      ["status"]=>
      NULL
      ["created"]=>
      int(1335542356)
      ["changed"]=>
      NULL
      ["comment"]=>
      NULL
      ["promote"]=>
      NULL
      ["sticky"]=>
      NULL
      ["tnid"]=>
      NULL
      ["translate"]=>
      NULL
      ["revision_timestamp"]=>
      NULL
      ["revision_uid"]=>
      NULL
      ["entityType":protected]=>
      string(4) "node"
      ["enforceIsNew":protected]=>
      NULL
    }
  }
}

At least in D7 it was clear this was a stub entity. In D8 its not the case, because it's run through entity_create first. Not doing that and just having a stub entity be a stdClass actually seems more correct to me.

+++ b/core/modules/taxonomy/taxonomy.testundefined
@@ -1913,3 +1913,40 @@ class TaxonomyThemeTestCase extends TaxonomyWebTestCase {
+    ksort($result);

This should be asort(), I'll reroll after someone looks at this.

+++ b/core/modules/entity/entity.moduleundefined
@@ -177,17 +177,16 @@ function entity_extract_ids($entity_type, $entity) {
-  // @todo Once all entities are converted, just rely on entity_create().

Looking at the todo here, it looks like this function is supposed to be removed in favor of entity_create() once all classes have been commited and now it's just File that's missing.

So I'd argue that we should fix the usage of this funciton, not revert it.

Well when I think about what a stub entity is, I don't think it SHOULD be run through entity_create.

The return value says "An entity object, initialized with the IDs provided."
Not "A classed entity object, initialized with the IDs provided and everything else set to NULL."

You have the entity_type and enough info to load the full entity yourself.

Ok, discussed this with timplunkett in IRC and we came to an agreement. To quote myself:

(20:42:21) Berdir: timplunkett: ok. I think I'm beginning to agree :) Let's revert the stub function . This is a temporary measure to not unecessarly break BC *yet* without gaining anything through that. Open a follow-up to discuss the EntityStub idea and vid/vocabulary_machine_name mess should be fixed elsewhere as well.
(20:44:47) Berdir: timplunkett: my reasoning was that we are in D8 and do not need bandaid fixes. But the function *is* a bandaid already so it doesn't matter and we can improve it without having a critical sitting on our head :)

The entity stub follow-up is at #1551140: Remove stub entities, replace entity_create_stub_entity(), more detailed explanation of the idea can be found there.

I haven't found an issue for cleaning up the vocabulary_machine_name/vid mess, surprisingly. Does one exist?

Looks good to me. Thanks for debugging this!

And no, we need to create an issue for the vocabulary_machine_name/vid mess. (great simplification ahead)

As I mentioned in #13, that ksort should have been asort. Leaving at RTBC.

#1054162: Taxonomy bundles not supported by EntityFieldQuery (followup) is a backportable fix for EFQ. A prober solution would not require this fix in the first place. The current situation with vid/machine_name is the same as if the node_type table had an auto-increment key and nodes would reference that and not the machine_name. If we fix that then we can either revert that fix if it has been commited by then or move it down to 7.x.

Created an issue #1551774: Replace taxonomy_term_data.vid with vocabulary_machine_name

I think this is the kind of patch I can commit to 8.x in order to fix brokenness, but the last couple of comments have me confused about whether this is actually RTBC.

#18 is RTBC. the discussion in the last two comments is not about this patch but how to continue in follow-up issues.

Follow-ups look fine to me. Committed/pushed this one to 8.x.