Taxonomy bundles not supported by EntityFieldQuery (followup)

I was looking just for this, thanks Dave!

For the record, the hook used is hook_entity_query_alter(), so the function header should be

function mymodule_entity_query_alter($query) {

Just came across this - thanks for the snipped!:)

Brilliant - I just realized that this is probably the fix for part of what I reported in #1207794: Provide administration form to execute reindexing batch.

Should this a) be considered as a bug report, and b) turned into a patch with a fix for Drupal 8 (for backport, hopefully)?

I definitely support this. Not having a reliable entityCondition('bundle') is a big limitation, and this patch removes half of it.

Btw, the comment:

 * convert the bundle condition into a proprety condition of vocabulary IDs to

has a small typo, proprety => property.

Just a quiet Subscribe and alerting Google that this patch relates to the taxonomy EFQ error:
SQLSTATE[42S22]: Column not found: 1054 Unknown column 'vocabulary_machine_name' in 'where clause': SELECT taxonomy_term_data.tid AS entity_id, :entity_type AS entity_type, NULL AS revision_id FROM {taxonomy_term_data} taxonomy_term_data WHERE (vocabulary_machine_name IN (:db_condition_placeholder_0)) ; Array ( [:db_condition_placeholder_0] => * [:entity_type] => taxonomy_term ) in EntityFieldQuery->execute()
Don't mind me. Just passing through.

I've tested the logic here, though not as a core patch. Approach is sound.

I should probably test the actual patch soon, since it would be nice to not have to handle this in a custom module.

Should probably have a Problem/Solution issue summary here. Solution part is the OP, Problem just needs to be spelled out with some use cases. Mine was #1207794: Provide administration form to execute reindexing batch.

My case: http://drupal.org/node/1345320

Updating function title

Wrote an issue summary; hopefully this will pick up traction now that things in contrib are being flagged as duplicate of this.

I may create a mini-module containing DaveReid's code from the OP as an interim fix.

Relation lets users hit this bug too. #1345320: AJAX error in autocomplete for taxonomy relations
This patch works for me.

I can see this is a good solution for d7, but I'm wondering why vocabulary ids are still numeric? For d8 wouldn't it make sense to replace them with the machine name, and just have that as a column in the taxonomy_term_data table? Would save a conversion or two.

+ * Convert EntityFieldQuerys on taxonomy terms that have an entity condition

EntityFieldQueries?

+ * convert the bundle condition into a proprety condition of vocabulary IDs to

property

I'd set to RTBC, but as you say, there's no tests yet (although, this isn't possibly going to break anything, so test could come later, no?)

Sorry Evan

We can add some tests. :)

For someone writing the test, here's a query that should work on a fresh install but currently fails:

      $query = new EntityFieldQuery();
      $query->entityCondition('entity_type', 'taxonomy_term');
      $query->entityCondition('bundle', 'tags');
      $result = $query->execute();

Thanks guys!

This appears to work nicely in D7, or re-wording, we can edit content again. Exceptions were being thrown on node edit pages with Entity References, Taxonomy Fields & CCK Location fields present, although I didn't find the module that was actually triggering this.

+    if (is_array($conditions['bundle']['value'])) {
+      foreach ($conditions['bundle']['value'] as $vocabulary_machine_name) {
+        $vocabulary = taxonomy_vocabulary_machine_name_load($vocabulary_machine_name);
+        $vids[] = $vocabulary->vid;

taxonomy_vocabulary_get_names() is a bit more suited to this than taxonomy_vocabulary_machine_name_load().

This is a documented limitation with known workarounds, so I disagree it's a major bug, but let's split the difference at major task, since taxonomy machine names aren't in a good state (says the person who worked on the initial patches :().

Patch needs a rewrite, as entity.inc seems to have vanished in D8.

I really don't understand how on earth this can not be a bug. Something is not working in the way it's meant to, and causing an exception. It's also causing problems for contrib developers who have to figure out what's going wrong and then implement a hook for no other reason than core not being able to clean up its mess.

As for downgrading it: downgrade it to 'normal' if you must, but I can't help but feel that the only reason for doing this is to keep the major issue count below the threshold. But surely that isn't the case -- because that would be making a total mockery of the purpose of the thresholds, no?

There are dozens of things about core entities in Drupal 7 that are massively inconsistent, because it went in too close to code freeze to get things remotely straightened out. There are open, critical, tasks to sort that out for Drupal 8. However there is no point opening a major bug for every single thing about entities (or any other core API) that is not fully formed, because it completely dilutes the classification of the issue as to render it meaningless.

On thresholds, I regularly triage issues that are not really bugs, not really major, against the wrong project, duplicates, support requests out of the major bugs queue. Part of the motification for introducing the thresholds was not only so that we'd actually fix major bugs in some kind of timely manner, but also so we wouldn't completely cloud the stability of core by having hundreds (literally) of critical bugs about badly named hooks, missing tests, or other annoyances that aren't actually major functional bugs cluttering up the issue queue, making it impossible to identify what the actual critical, release blocking bugs were until they'd all been manually triaged and/or fixed at the last minute (which unfortunately lasted nearly two years).

There is no requirement in the core API that EntityFieldQuery works with a generic bundle property condition, and the fact that you can't rely on them to have that for EntityFieldQuery is documented in comments. Let alone entities like user that only have one bundle and would also break with that condition. It's an annoying limitation but it's not a functional bug - a bug would be if we claimed it worked and it didn't. If you need to query taxonomy terms by machine name, it's quite possible to figure out the vid and pass it as propertyCondition('vid') when building the query, that again, is documented - this patch even has to remove that documentation. It's great that there's a workaround here that can be backported, but once the patch is in, there will still be the same limitation for comments, and for any contrib entity that has unusual bundle handling.

So if you look at http://drupal.org/node/1181250 and http://drupal.org/node/45111 this looks like major task to me, but I can't be arsed to play issue ping pong with you, so hopefully the re-roll, and some test coverage, will materialize.

Ok fine. I don't want to play ping-pong either. At the end of the day you're the maintainer and it's your call.

I've obviously misunderstood 'bug' as meaning 'stuff that doesn't work', and the documentation for EFQ makes it clear to me that something isn't working:

> This class allows finding entities based on entity properties (for example, node->changed), field values, and generic entity meta data (bundle, entity type, entity id, and revision ID).

Here's a patch:

- rerolled Dave Reid's patch from #5
- made catch's suggested change
- added basic test

Test-only patch to expose the failure as per the core testing policy: http://drupal.org/core-gates#testing

Thanks @joachim for getting this un-stuck. :)

I personally would agree with catch since it was documented that taxonomy_term entities were not supported, but it's probably not worth any debate. Reviewed the patch and found a couple of minor cleanups are needed, plus I'd like to see some more test coverage:

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -1957,3 +1957,35 @@ function taxonomy_taxonomy_term_delete($term) 
+ * Convert EntityFieldQuerys on taxonomy terms that have an entity condition
...
+ * convert the bundle condition into a proprety condition of vocabulary IDs to

Both naught101's points from #13 still need fixing here ("property" is misspelled, and it would be preferable to either spell "queries" correctly, or, better, say "Convert EntityFieldQuery conditions" or some such.

+++ b/core/modules/taxonomy/taxonomy.testundefined
@@ -1895,3 +1895,30 @@ class TaxonomyThemeTestCase extends TaxonomyWebTestCase {
+ * Tests for verifying functionality of taxonomy EntityFieldQuerys.
...
+   * Test a basic taxonomy EntityFieldQuery works.

I'd phrase these as:
Tests the functionality of EntityFieldQuery for taxonomy entities.
and
Tests that a basic taxonomy EntityFieldQuery works.
Reference: http://drupal.org/node/1354#functions

+++ b/core/modules/taxonomy/taxonomy.testundefined
@@ -1895,3 +1895,30 @@ class TaxonomyThemeTestCase extends TaxonomyWebTestCase {
+    // No assertion needed: this simply tests we don't get an exception here.

While this is true, it might be more thorough to assert an expected result (i.e., that the EFQ not only does not throw an exception, but actually works)? E.g., it might be good to have a test for a single vocabulary, for multiple vocabularies etc.

Leaving tagged with "Needs tests" to expand the test coverage. Also adding the Novice tag, both for the cleanup and more test coverage, as more tests are straightforward now that @joachim has written the initial test. Thanks!

OK, so just to be clear, in D7, ignore the docs for taxonomy_term_load_multiple which say "it is preferable to use EntityFieldQuery to retrieve a list of entity IDs", and just use the deprecated $conditions array?

So this doesn't even have the bundle part of the query, or the fix. Theoretically this should work, but I get exceptions locally.

So it passes and yet there are exceptions. And that's supposed to work currently.

So this was broken in #1361232: Make the taxonomy entities classed objects. Not sure if that should be reopened or if this should be bumped to a major bug.
But running tests before and after that commit show that you can no longer do a basic EFQ.

git checkout 5a8e7bd^ to try it yourself.

This issue is already major. That's critical. Opening a new issue.

I filed #1550454: Regression: Following taxonomy entity conversion, Taxonomy EFQ causes warnings and notices.

So this issue is unrelated to that one, but fixing this issue is probably blocked on that one since we need that bug fixed for any automated test for this issue to pass.

This incorporates the fix from #1550454-18: Regression: Following taxonomy entity conversion, Taxonomy EFQ causes warnings and notices for now.

#37 looks great to me. Obviously this is blocked until #1550454: Regression: Following taxonomy entity conversion, Taxonomy EFQ causes warnings and notices goes in and we'll need to reroll after, but so far so good.

Utterly unimportant nitpicks:

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -1659,3 +1659,35 @@ function taxonomy_taxonomy_term_delete(TaxonomyTerm $term) {
+ * Convert EntityFieldQuerys on taxonomy terms that have an entity condition

The EntityFieldQuerys drives me crazy; can we reword it?

+++ b/core/modules/taxonomy/taxonomy.testundefined
@@ -1913,3 +1913,56 @@ class TaxonomyThemeTestCase extends TaxonomyWebTestCase {
+class TaxonomyEFQTestCase extends TaxonomyWebTestCase {
+  public static function getInfo() {
...
+  }
+}

Wayyyy nitpicky but can we get a blank line between the first and last line of the class and its members? This has been going into cleanup patches.

#37: drupal-1054162-36-combined.patch queued for re-testing.

Ok, re-rolled now that the other issue has been commited and also fixed the two points from #38.

Reuploading both to show that the tests still fail on their own.

This looks nice.

OK this has been in the queue for a while and it's been worked on by all the people I'd expect to review it, patch looks good so committed/pushed to 8.x.

#1552396: Convert vocabularies into configuration or a follow-up to that issue may obsolete having to do the alter here. Moving to 7.x for backport.

D7 never got TaxonomyEFQTestCase from #1550454: Regression: Following taxonomy entity conversion, Taxonomy EFQ causes warnings and notices, so I included that as well.

Same changes (except adding the test class and not altering it) as in 8.x and the tests confirm that it's working.

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/f209013

However, it looks like there was some feedback above that was never addressed, such as:

1. The documentation changes from earlier patches in this issue were lost, so the EntityFieldQuery docs are still claiming (now incorrectly) that taxonomy terms don't support bundle conditions.
2. The typo identified in #6 ("proprety" => "property") was never fixed.

Moving back to D8 for those changes.

This patch fixes the typo identified in #6 and mentioned again in #48.

@David_Rothstein, re: comment #48, which documentation changes from earlier patches in this issue were lost? @timplunkett and @zendoodles were helping me look at this issue during Core Mentoring Hours, but I'm kinda slow. I've been out of the loop for a bit.

If you can point me to the earlier patches in this thread that had the docs needed, I can roll a patch to include them.

Thanks!

I think the first hunk in #23 had it.

Two new patches. One for D7, the other for D8. Each patch makes the changes mentioned in #48.

1. Fixing the EntityFieldQuery docs
2. Fixing the ("proprety" => "property") typo

The D7 patch here supersedes the "spellingerror-1054162-48.patch" from #49.

Thanks to @timplunkett and @zendoodles for all the help!

#52 SHOULD have been "Needs review". Because those patches need to be reviewed. Not sure what happened.

Looks good to me.

There is an issue with the patch that was committed when using different storage engines because the vid is not being cast to an integer. Patch will follow in a moment but as it stands this breaks any none mysql implementation eg mongodb.

This is an easy D8 documentation follow-up that shouldn't get lost. Can you open a new follow-up issue?

Spliting off the bug into a different issue

Patch in http://drupal.org/node/1054162#comment-6144012 is the item that is RTBC. Moved other item to #1706850: Entity field queries need to read entity base table schemas to cast correctly.

Reuploading just to shut the bot up.

Committed/pushed the docs fix. Assume this needs 7.x backport as well.

Isn't Transitions patch in #52 good for 7.x. Re uploaded

Looks good.

Committed and pushed to 7.x. Thanks!