Fix EntityFieldQuery fatal error in certain cases and document bundle limitations

That's obviously braindead. But why do we have a core that passes all tests?

This can not be supported. The bundle for taxonomy_terms are the vocabulary machine name however the taxonomy_term_data table only has a vid. Comments store their bundle in node. Both use a custom entityload class to assemble the entity and indeed the entity documentation talks of object properties not database keys. The only solution is to remove bundle support. Ppl using EFQ can use propertyCondition vid for terms and... they are screwed on comments. Such is life. There is no way to help that in Drupal 7 core. A contrib can do a schema alter and do the denormalization.

The rest should be ok, entity type, entity id and revision id these are ok but bundles are not.

1. Change the bundle in taxonomy module to taxonomy_vocabulary_machine_name instead of vocabulary_machine_name.
2. EntityFieldQuery loads the schema and checks whether bundle key is found in the entity base table.
3. If not, it goes over the foreign key tables, and finds the one that is prefix of the bundle key. In case, taxonomy_vocabulary and node, and verifies that the rest of the bundle key, in case machine_name and type are fields in the table to be joined.
4. Document all this in hook_entity_info.

Phew! We saved the bundle with minimal API change and empowered the whole system with information.

Discussed this in chx with irc, the above plan is option #4 of these:

catch> I don't like this because that leaves views_efq in a bit of a bind.
<catch> 1. Leave it as is and take efq out of the picture.
<catch> 2. add machine_name to comment and taxonomy_term_data tables, use this for bundle key.
<catch> That's a schema change, but it's also a fairly painless one.
<catch> 3. Kill vocabulary id altogether and replace it with machine_name, that's Drupal 8.
<catch> 4. Find a way to tell efq how to query on bundle name when modules like taxonomy or comment do this.

I think that's the best at this stage of the cycle. We need a Drupal 8 issue to figure out a standard for entity bundles (if I remember correctly there was some resistance to storing bundles as strings for taxonomy terms and comments due to the extra storage, queries on ints are more efficient etc., but is that worth the tradeoff to avoid issues like this).

I don't see addressed above the possibility of simply documenting the limitation for Drupal 7, i.e. that you cannot use EFQ::propertyCondition() on Comment or Term bundles. Based on the inline documentation, I see two things that would make this a reasonable plan of action:

1. As the class documentation says, "It is not possible to query across multiple entity types." So whoever writes the query will know if it's to retrieve Comments or Terms.
2. As the propertyCondition() documentation for the $column argument says, "A column defined in the hook_schema() of the base table of the entity." In other words, the choking and dying bojanz first reported is by design, since the node_type column isn't defined in the schema for the entity's base table. EFQ developers would just need documentation at the entity definition level on whether or not the bundle is actually defined in the schema (although they could just open the .install and check).

That said, it seems like a fix is preferable. Just wanted to point out that this isn't a technically "unexpected" bug... maybe just an unintentional limitation.

As to applying chx's solution, it seems like it would work. It's uncomfortable, though, to introduce a JOIN based on a naming convention and the use of foreign keys. I suppose it's the least invasive solution where we are now, but it's going to result in some unwieldy API documentation. : P (This should also be accompanied with documentation in the term and comment schema to indicate altering the foreign key definitions would break EFQ::propertyCondition().)

I suppose at the very least we have core examples to point to, but really we can just say any other entity developers should just make sure the bundle has a column in the entity's base table.

I think, too, that bojanz inadvertently raised a related issue above... that of entities that are defined as single bundle entities. In the case of the commerce_order entity, I don't define a bundle entity key and through trial and error found out this means my entity will have a single commerce_order bundle. Now, there is no column in my commerce_order table for the order type, so it seems the EFQ::propertyCondition() won't work for me here anyways. Maybe it's never supposed to work to query based on bundle if there is no bundle entity key. But what if a contributed module alters the entity info, adds a bundle key, creates an order types table, and then uses a third table to relate orders to their order type (i.e. instead of altering the schema to add a column onto the base commerce_order table) and alters the orders on load to include their type. The proposed solution cannot accommodate that, and shouldn't... the documentation for the $column parameter sets that expectation.

In other words, even with the fix, it can still be broken, and unless I'm misunderstanding EFQ (which is quite likely) still won't accommodate entities that don't support bundles. That might tilt this toward "don't fix, document the limitation", but if the fix is preferable for other reasons, then chx's suggestion would work, should be documented, and the method should be discouraged until the problem can be removed in D8. How exactly it should be removed for comment bundles if not just storing the type in the comment table is a little beyond me this late in the evening. ; )

If your entity type does not have a bundle then dont specify a condition on bundle :) piece of cake, there. But the fix for comment, taxonomy_term and any other entity type that follows the core example of storing the bundle in another table is necessary IMO.

Would it be worse to introduce something other than a [table]_[field] naming convention that gets checked as a fallback? i.e. what if you could specify a bundle entity key of [field] (the regular definition), or [foreign_key]:[field] (which finds the table the foreign key, joins on the key, and adds the [field] to the query)?

Of course, I'm not sure if : is any better (could get even more creative and use ->), but at least people wouldn't normally be using : in bundle names (whereas they're most likely already using _). Also, even if the _ is kept, I wonder if specifying the foreign key outright wouldn't be preferable to specifying a table and then searching the foreign keys for that table. In this regard, you could actually do away with step 1 in chx's solution and instead change the bundle entity key for comments from node_type to comment_node_type (or comment_node:type).

Hmm, of course you can't use a : if you expect the bundle entity key to be a property on the loaded entity... duh. However, perhaps you specify the foreign key to use for the bundle somewhere else in the entity's info array. Just musing... point about referencing the foreign key instead of a table name would still stand.

We actually miss information about the bundle object here. If we would have the bundle 'db table', it could check for the "bundle keys | bundle" property and query for that using the foreign-keys info. Let's just make any storage-object an entity for d8 so we have a proper and unique place for info about it...

Still, comments and terms are a bit different now. Terms use the usual "entity object - bundle object - bundle name" approach, while comments go directly with the on load determined "node_type" property. It might work to translate the comment case to "entity object - bundle object - bundle name" too, with using "node" as bundle object and "type" as bundle key.

Then for d7, we would just need something like a "bundle db table" property.

1. Change the bundle in taxonomy module to taxonomy_vocabulary_machine_name instead of vocabulary_machine_name.
2. EntityFieldQuery loads the schema and checks whether bundle key is found in the entity base table.
3. If not, it goes over the foreign key tables, and finds the one that is prefix of the bundle key. In case, taxonomy_vocabulary and node, and verifies that the rest of the bundle key, in case machine_name and type are fields in the table to be joined.
4. Document all this in hook_entity_info.

Let's not magically bake more information in a single info-key. The 'entity keys' are about the object properties, if we need information on how to find the property column in the DB, that's something different and thus it should be another info key.

> Then for d7, we would just need something like a "bundle db table" property.

I wonder if it this property doesn't connect too tightly the idea of an abstracted entity that "doesn't care" about it's storage, to the DB?

>I wonder if it this property doesn't connect too tightly the idea of an abstracted entity that "doesn't care" about it's storage, to the DB?

The whole EFQ-entity-query "backend" is db specific, just as the default drupal entity controller. This properties are used by the db-specific implementations, you still can do others. Though I'm not aware of anyone having done that already?

There are not many use cases for bundle queries on comments. Recent comments on a node type would often use node_comment_statistics since that gets you just one comment per node. The vid on terms is fine. We can't drop it for nodes. So documenting the limitation seems good here. Well that and fixing the fatal error.

bojanz then let's fix EFQ not to error out and document. Let's do the absolute minimal necessary.

Calling that variable having is a really bad idea. Anything else is a lot better. bundle_present or whatnot. But having is a SQL keyword and superb confusing.

I suspect yes. http://drupal.org/files/issues/938462_2_0.patch is indeed RTBC.

We can try harder. Field tables store bundle. (MongoDB also does. It's somewhat mandatory for fields to store it because they have no clue whether there is such a thing as an entity base table.) We have an entity type and a bundle here... field_info_instances($entity_type = NULL, $bundle_name = NULL) can deliver us the instances and then we can bring the table of any instance in. Is this something we want to do?

Discussed with bojanz and we both agree that for Drupal 7, we still want to go ahead with the simple patch indicated above. It'd be a hack to join in a random field table.

That looks good. I would have sworn we fixed that test already like that... good job.

Committed to CVS HEAD. Thanks.