Conditions on multi-column fields in EntityFieldQuery

Fixing category, and also does this bug exist in D8 EFQ?

> Use the same alias for fields on the same field for the same delta, for instance for two conditions on different columns of the same field.

That's what delta group is for.

I am so sorry for not reviewing this more carefully. Now I did. I think it's good to go. I am slightly surprised the existing tests didn't catch this.

As for D8, this is all gone and a new system is in place with clean condition groups.

I think we need to get the tests into Drupal 8 first. (Unless there are equivalent tests already there? But I didn't see any.)

Looking at the code a bit:

     $table_alias = $tablename . $key;
+    // Use the same alias for fields on the same field for the same delta, for instance
+    // for two conditions on different columns of the same field.
+    if (!empty($query->fieldConditions[$key]) && is_int($query->fieldConditions[$key]['delta_group'])) {
+      $table_alias = $tablename . '_delta_' . $query->fieldConditions[$key]['delta_group'];
+    }

1. Is it safe to remove $key from the table alias? (I.e., is it possible that two different fields on the same table will be queried in a way that happens to use the same delta group, and if so, will that break things?)
2. I don't understand the origin of the is_int() check. Is it documented that 'delta_group' has to be an integer? (All the documentation I see says "arbitrary identifier".)

+    $this->assertEqual(1, count($result), t('One result should be returned, got @count', array('@count'
....
+    $this->assertNotNull($query, t('Precondition: the query should be available'));

Minor, but don't use t() in assertion messages. Use format_string() for cases like the first, and just a regular untranslated string for cases like the second.

+/**
+ * Implements hook_query_TAG_alter().
+ *
+ * Saves the query in a global variable to allow tests.
+ */
+function field_sql_storage_query_test_alter($query) {
+  $GLOBALS['test_query'] = $query;
 }

I think it's a little more standard to use a variable_set() here (or state()->set() in Drupal 8).

Dont change the version, EntityFieldQuery is gone completely from D8 and entity_query that replaces it is totally different.

I'm very happy with the improvements this patch achieves.

I ran across this issue via #1649398: Performance issues with many relations and the patch in #14 seems to have the performance of my relation-heavy site under control. No problems so far.

I came here via #1970156: MySQL can only use 61 tables in a join: field_data_endpoints on node_save. The patch from #14 does solve the issue (albeit temporarily - as the number of Relations grow so the number of joins will grow until they exceed MySQL's limit again), but when running all tests on all modules which are part of my site, a number of tests fail. Is that to be expected with a core patch like this?

The parts I've looked at look pretty good to me now, but the tests still need to go in Drupal 8 first - again, unless someone can point to where equivalent tests already exist? (It doesn't matter if the API changed; the tests are just testing the functionality and we need the tests to verify that the bug doesn't exist and won't exist again.)

Couple small issues in the D7 patch (I didn't review the whole thing though), mostly code style:

1. Why is $query_tables initialized to NULL rather than an empty array?
2. Replace "Generate" => "Generates" in the code comment.
3. Replace $fieldkey with $field_key.
4. Replace $hasgroupconditions with $has_group_conditions.
5. There should be a blank line before @return (see https://drupal.org/coding-standards/docs).
6. t() should still be removed from the test assertion messages (especially now that patches have been committed in the interim to clear those out from pretty much everywhere in Drupal core - including, I believe, the field_sql_storage.test file - so we don't want to reintroduce it).

@burningdog, do the same tests fail without this applied, or are there new test failures caused by this patch?

Core tests should in theory always pass but I think it's known that some of them are environment-specific and won't always pass for everyone.

Contrib tests, who knows... it depends entirely on the project.

THANK YOU, works like a charm

I still think it should be possible to test this in Drupal 8 too. Assigning to myself to work on that, probably sometime in the next week or two.

If I'm wrong, mea culpa in advance because it would mean that I unnecessarily delayed this patch from getting in :(

There's absolutely nothing that is relevant in D8 to this patch: the whole subsystem is just gone.

Rerolled for latest D7

Back to rtbc then

30: multicolumn-1859084-30.patch queued for re-testing.

Looks like this is still RTBC.

30: multicolumn-1859084-30.patch queued for re-testing.

RTBC+1

The latest code has been marked RTBC for 4 months now. What's needed for this to get in an upcoming release?

@David_Rothstein let me know if it's useful to put this in a production environment & report back.

I'm still not sure this can't be tested in Drupal 8, but I never got around to working on it and it missed one release because of that (sorry) so let's just get it in for now.

However, this:

+++ b/modules/field/modules/field_sql_storage/field_sql_storage.test
....
+/**
+ * Implements hook_query_TAG_alter().
+ *
+ * Saves the query in a global variable to allow tests.
+ */
+function field_sql_storage_query_test_alter($query) {
+  $GLOBALS['test_query'] = $query;
 }

We can get away with a global variable, but not with putting this directly in a .test file (and with a non-test-module namespace) :) I didn't notice before that that's where it was.

I cleaned that up in the attached patch and moved it to the field_test module instead. Everything else looked good and seems to work well, so since I only changed the tests I'll commit this if it still passes tests.

It's worth noting (and I will in the commit message) that this patch only fixes the issue when a delta or language group are being used, rather than in the general case. For example, something like this:

  $query = new EntityFieldQuery();
  $query->fieldCondition('body', 'value', 'first excluded value', '!=');
  $query->fieldCondition('body', 'value', 'second_excluded_value', '!=');
  $result = $query->execute();

still results in an unnecessary inner join with this patch applied. So there might be room to fix this further... However, limiting it to queries with a delta or language group is fine for now (and means the impact is much less in case the patch winds up breaking something, which is good also).

> I'm still not sure this can't be tested in Drupal 8,

I am. The whole subsytem is rewritten from the ground up. There's not even a trace of this code in D8. It's done 100% differently. I wrote both codebases, you can believe me.

Committed to 7.x - thanks!

I have no doubt that the code is very different in Drupal 8... what I'm not sure of is whether Drupal 8 actually has some kind of similar tests (to ensure that the generated queries don't have an unnecessary number of INNER JOINs)... In any case, I'll mark this "Fixed" for now, but still leave assigned to me to remember to potentially look into that.

Drupal 7.32 was a security release, so this is now scheduled to be in 7.33.