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

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)?

Initial patch still needs tests.

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.

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

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.

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?)

We can add some tests. :)

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.

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 :().

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.

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:

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.

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

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?

The last submitted patch, drupal-1054162-30.patch, failed testing.

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.

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

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

#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.

This looks nice.

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

Confirmed this looks clean, straight backport and works on my local install now!

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

I think the first hunk in #23 had it.

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

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.

Spliting off the bug into a different issue

The last submitted patch, d8-combo-1054162-56.patch, failed testing.

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

Looks good.

Automatically closed -- issue fixed for 2 weeks with no activity.