Make EntityFieldQuery work with multilingual properties

Put on sprint in anticipation of that one landing soon :)

I do think EFQ should be aligned, such that it works for fields and non-fields the same way, i.e. there is just a single addCondition() method. That doesn't necessarily need to part of that issue though.

First, thanks for the patch.

This whole data table business needs comments. The data_table variable needs to change to data_table_alias because it's not a table, after all -- there is no table called 'data'. There's $table = isset($data_table_schema['fields'][$column]) ? $data_table : $base_table;. I would be happier with initializing $data_table_alias to FALSE and make this line $table = data_table_alias ? $data_table_alias : $base_table;. If that's not viable we need a comment on what sort of propertyCondition necessitates a column-by-column choice. Also, if we do not per-column then this choice might be moved out of the loop and only did once. Perhaps insttead of data_table_alias we could use $property_table = $base_table; and then override it with the 'data table' key if there is one.

$sql_field = !empty($entity_info['entity keys'][$key]) ? $entity_info['entity keys'][$key] : $key;. Really?? We expect people to put uuid and langcode into sql fields not called uuid and langcode?

Also, I do not see tests.

We should be able to simple remove the overriden loadByProperties() method in EntityTestStorageController in #1184272: Remove deprecated $conditions support from entity controller then this should already add basic test coverage and serve as a proof that this works. Might need more specific tests, though.

...coming from #1188394-49: Make title behave as a configurable, translatable field (again)

It would be great if the issue summary had some basic information of what needs testing here. I really want to help with this and testing is as far as I can go. Let me know.

Tagging.

Re-roll of the original patch with the overridden loadByProperties() method in EntityTestStorageController which means this does now actually test that part of EFQ. I do get two failures locally, so there seems to be an issue...

I'm no efq expert, but from a overall stance this looks good. The introduction of the "data table" concept though is something storage controller specific, so it should also live in its storage specific query implementation.
We don't need to do that now, but I think we really want to move efq-query execution to a storage-query controller/plugin/manager.

I'm not an expert, but this looks solid

+++ b/core/modules/entity/lib/Drupal/entity/EntityFieldQuery.phpundefined
@@ -961,4 +1013,114 @@ class EntityFieldQuery {
+      // Every properties needs a new join on the data table.

properties -> property

I'm not so used to the EFQ, so would definitely be a candidate which has to learn the EFQ with the new parameters.

When I got this right, I would need this:

$query = new EntityFieldQuery();
$query->entityCondition('entity_type', 'node');
$query->propertyCondition('title', 'english_title', NULL, 'original');
$query->propertyLanguageCondition('en', NULL, 'original');
$query->propertyCondition('title', 'german_title', NULL, 'translation');
$query->propertyLanguageCondition('de', NULL, 'translation');
$result = $query->execute();

but why we don't use:

$query = new EntityFieldQuery();
$query->entityCondition('entity_type', 'node');
$query->propertyCondition('title', 'english_title', NULL, 'en');
$query->propertyCondition('title', 'german_title', NULL, 'de');
$result = $query->execute();

because this cannot be possible?:

$query = new EntityFieldQuery();
$query->entityCondition('entity_type', 'node');
$query->propertyCondition('title', 'english_title', NULL, 'original');
$query->propertyCondition('title', 'german_title', NULL, 'original');
$query->propertyLanguageCondition('en', NULL, 'original');
$result = $query->execute();

so in my understanding the reason why we need the 'propertyLanguageCondition' is to group the different languages, but then we could directly use the langcode as group?
or is there a case which I'm missing?

Looks solid to me too. minor thing before i look deeper:

+++ b/core/modules/entity/lib/Drupal/entity/EntityFieldQuery.phpundefined
@@ -420,19 +427,48 @@ class EntityFieldQuery {
+  public function propertyCondition($column, $value, $operator = NULL, $langcode_group = NULL) {
+    $group = !empty($langcode_group) ? $langcode_group : 0;

simpler: $langcode_group = 0
(also in some other places)

not meant to change status

Tagging for VDC too.

Tagging for VDC too.

+  // The entity base keys require joining the entity base table.
+  $entity_base_keys = array('langcode', 'uuid');

So this hardcodes that base table sql fields. I think this is already denormalized to hook_entity_info, so it should be read from there.

-    '#submit' => array('language_admin_add_form_submit'),
+    '#submit' => array('language_admin_add_custom_form_submit'),
   );

Unrelated.

+    list($data_table, $data_table_schema) = $this->getPropertyDataSchema($entity_type);
+    $specifier = $order['specifier'];
+    $table = !empty($data_table_schema['fields'][$specifier]) ? $data_table  . '_' . $order['langcode_group'] : 

Does this ensure we have joined to the table already? I guess we should have something like views' $this->ensureTable(). Also, as we've already the base table schema fields, why not just assume it's in the data table if it' not in the base table and not a field?

+  public function propertyLanguageCondition($langcode = NULL, $operator = NULL, $langcode_group = 0) {
+    // We have a separate method here to ensure there is a distinction at API
+    // level between properties and metadata, even if from the implementation
+    // perspective both conditions are implemented the same way. However this
+    // might not be the case in other storages.
+    // @todo Actually we could also implement the same functionality and keep
+    // this distinction by using language codes as group identifiers:
+    //
+    // $query->propertyCondition('title', 'english_title', NULL, 'en');
+    // $query->propertyCondition('uid', 1, NULL, 'en');
+    // $query->propertyCondition('title', 'german_title', NULL, 'de');
+    //
+    // We probably want to move to this approach when refactoring EFQ to work
+    // with storage-independent entities. For now we are keeping the current
+    // approach for consistency with field meta conditions.
+    return $this->propertyCondition('langcode', $langcode, $operator, $langcode_group);

hm, that makes me feel I cannot query for the entity language any more? Can I?

Any remaining concerns or should this go for RTBC finally? :)

All right, this has been reviewed by various people from various angles. All concerns are now resolved, there are plenty of docs, so let's get it in!

I must say I find it quite confusing that $this->propertyCondition('langcode') does something else than $this->entityCondition('langcode'). Then as pointed out in #24 the whole API is not very nice to use, but yep - this patch just follows what we already have for fields. So I guess it's best to move on with this now, but work on improving the API in a follow-up.

We already have #1754174: Add entity_query() wrapper for EntityfieldQuery to simplify common usage as a start.

This deals with the rapidly changing entity landscape and is at a definite risk of commit conflicts.

#33: ml_efq-1691952-33.patch queued for re-testing.

+++ b/core/modules/entity/lib/Drupal/entity/EntityFieldQuery.phpundefined
@@ -420,19 +427,61 @@ class EntityFieldQuery {
+   *   An arbitrary identifier: conditions in the same group must have the same

Is it really arbitrary? Presumably you'd want to put a langcode in here most of the time? What's the difference between a 'group' and a 'langcode group'?

+++ b/core/modules/entity/lib/Drupal/entity/EntityFieldQuery.phpundefined
@@ -420,19 +427,61 @@ class EntityFieldQuery {
+  /**
+   * Adds a condition on the property language.
+   *
+   * @param $langcode
+   *   The language code that the properties belonging to the language group
+   *   should match.
+   * @param $operator
+   *   The operator to be used to test the given value.
+   * @param $langcode_group
+   *   An arbitrary identifier: conditions in the same group must have the same
...
+    // We have a separate method here to ensure there is a distinction at API
+    // level between properties and metadata, even if from the implementation
+    // perspective both conditions are implemented the same way. However this
+    // might not be the case in other storages.
+    // @todo Actually we could also implement the same functionality and keep

OK reading this it starts to make more sense. So $langcode is the language code. Additionally we're allowing you to pass a langcode group in. No... I still don't get it just reading the comments at all.

+++ b/core/modules/entity/lib/Drupal/entity/EntityFieldQuery.phpundefined
@@ -420,19 +427,61 @@ class EntityFieldQuery {
+    // @todo Actually we could also implement the same functionality and keep
+    // this distinction by using language codes as group identifiers:
+    //
+    // $query->propertyCondition('title', 'english_title', NULL, 'en');
+    // $query->propertyCondition('uid', 1, NULL, 'en');

This looks a bit saner - why's it a @todo?

If there really is a good reason to not use langcodes for langcode groups (then presumably we could set the default to NULL and default to the langcode passed in if it's not set explicitly too?) then this could do with much better comments about what you'd need to pass in and why. Maybe query examples in @code tags?

None of the rest of this merits a needs work, just noting:

+++ b/core/modules/entity/lib/Drupal/entity/EntityFieldQuery.phpundefined
@@ -824,7 +883,7 @@ class EntityFieldQuery {
-        $this->addCondition($select_query, $base_table . '.' . $sql_field, $this->entityConditions['revision_id']);

These don't look related to this patch at all, but it's more readable this way so I don't mind them slipping in.

+++ b/core/modules/entity/lib/Drupal/entity/EntityFieldQuery.phpundefined
@@ -858,6 +919,15 @@ class EntityFieldQuery {
+    foreach (array('uuid', 'langcode') as $key) {
+      if (isset($this->entityConditions[$key])) {
+        $sql_field = !empty($entity_info['entity keys'][$key]) ? $entity_info['entity keys'][$key] : $key;
+        if (isset($base_table_schema[$sql_field])) {
+          $this->addCondition($select_query, "$base_table.$sql_field", $this->entityConditions[$key]);
+        }
+      }

Ouch that's nasty. So we're loading the schema just to check if the uuid or langcode keys are defined. Is there an actual issue open to require a naming convention on the table instead of the entity keys stuff?

+++ b/core/modules/entity/lib/Drupal/entity/EntityFieldQuery.phpundefined
@@ -961,4 +1031,104 @@ class EntityFieldQuery {
+  public function addPropertyOrderBy(Select $select_query, $entity_type, array $order) {
+    $entity_info = entity_get_info($entity_type);
+    list($data_table, $data_table_schema) = $this->getPropertyDataSchema($entity_type);
+    $specifier = $order['specifier'];
+    $table = !empty($data_table_schema['fields'][$specifier]) ? $data_table  . '_' . $order['langcode_group'] : $entity_info['base table'];
+    $select_query->orderBy("$table.$specifier", $order['direction']);

This is going to be nasty for performance, but I assume it will only affect queries that are actually multilingual?

As mentioned above, we already cache the schema fields of the base and revision table in hook-entity-info, couldn't we just assume a property is in the data table if it's not in the main table or in the revision table?

Nah I 'd really like to see the docs improvements here, because I genuinely can't understand what's in there at the moment, and I'd like to not commit new features that I don't understand the API docs. $delta is a lot easier to grok than $langcode vs. $langcode_group.

I don't think we have any direct need for this support right now in EFQ, so if the refactoring is happening elsewhere, then it might not make sense to keep this among the top current issues, and we can possibly return here later. I don't think Views is planning to use EFQ, they were not much interested in this issue even though asks and tagging for VDC and there is minimal use of EFQ in core so far. If/once people want to make EFQ use spread, they would hit this issue anyway and we would get more eyeballs.

We have little more than 10 weeks to go, so time is becoming very precious.

I think the difference is that $delta_group doesn't sound like anything, whereas $langcode_group does a bit.

Really the docs improvement might only need to be a couple of example queries in @code tags.

OK thanks for the code example. I'm still not comfortable with the terminology but don't have a better suggestion so committed/pushed to 8.x.

We'll need a change notice for the API addition.

Looks understandable to me. Good code example, IMHO.

Removing Avoid commit conflicts tag

Updated issue summary.