EntityFieldQuery currently returns an incorrect entity count when you query a field that has multiple translations.

If you create a node with a translatable body field, and add 3 translations, querying for the entity like this:

$query = new EntityFieldQuery();
$entities = $query->fieldCondition('body')->execute();

will return one entity, but if you do the same thing with a count

$query = new EntityFieldQuery();
$count = $query->fieldCondition('body')->count()->execute();

the result is three.

This also means that $query->ordered_results will have duplicates.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

loganfsmyth’s picture

Status: Active » Needs review
FileSize
713 bytes

This was enough to get it working for me.

I don't know if using $field['translatable'] directly is acceptable, or if we need to use something like this, http://api.drupal.org/api/drupal/modules--field--field.multilingual.inc/..., but then you need to have entity_type nailed down.

plach’s picture

Status: Needs review » Needs work

From a quick review the fix looks correct, I think we need some tests covering the bug.

loganfsmyth’s picture

Status: Needs work » Needs review
FileSize
2.65 KB

Added a basic test that creates a single entity with a translatable field in two languages, with cardinality one.

This patch also includes adding entity_query.test to entity.info's file list, because it seems to have been missed when entities moved the a module. I've seen that particular fix one of the other entity related patches too, but it hasn't been committed yet.

plach’s picture

Stupid coding standard things to be fixed below. Attaching a test-only patch which is supposed to fail to prove that the tests capture the bug. Also, this will need to be backported.

+++ b/modules/entity/tests/entity_query.test
@@ -1048,6 +1048,42 @@ class EntityFieldQueryTestCase extends DrupalWebTestCase {
+    // Make a test field translatable AND cardinality 1

Missing trailing dot.

+++ b/modules/entity/tests/entity_query.test
@@ -1048,6 +1048,42 @@ class EntityFieldQueryTestCase extends DrupalWebTestCase {
+    // Set fields in two languages with 1 field value

Missing trailing dot.

+++ b/modules/entity/tests/entity_query.test
@@ -1048,6 +1048,42 @@ class EntityFieldQueryTestCase extends DrupalWebTestCase {
+    // Look up number of results when querying a single entity with multilingual field values.

Comment does not wrap at column 80.

-1 days to next Drupal core point release.

Status: Needs review » Needs work

The last submitted patch, efq-translatable-field-count-1292922-4-test.patch, failed testing.

loganfsmyth’s picture

Here's an update for coding standards, and a test-only patch to match.

plach’s picture

Status: Needs review » Needs work

Grrreat, but...

+++ b/modules/entity/tests/entity_query.test
@@ -1048,6 +1048,43 @@ class EntityFieldQueryTestCase extends DrupalWebTestCase {
+    // Look up number of results when querying a single entity
+    // with multilingual field values.

...comments should wrap as near as possible to column 80. In this case:

    // Look up number of results when querying a single entity with multilingual
    // field values.

See http://drupal.org/coding-standards for details.

loganfsmyth’s picture

Ah, didn't realize that, thanks.

chx’s picture

Is DISTINCT what you want? Shouldn't this be a documentation patch to urge you to specify a language?

Status: Needs review » Needs work

The last submitted patch, efq-translatable-field-count-1292922-8-test.patch, failed testing.

loganfsmyth’s picture

Status: Needs work » Needs review

I think in this case DISTINCT makes the most sense, as we may not know or want to query by the language. Also, the distinct is already there in any case whenever the cardinality isn't '1', so I felt this change wouldn't be a drastic addition. It doesn't really make sense that you could get one result entity when you run the query itself, but then get a count of more than one when you do a count query. Even if it were documented, it would be a fairly strange behavior to have.

As an example of not knowing the language to query by, I noticed this issue while working on Entity Translation because we want to look up all entities with values for a certain field, and then only later look up field values based on that particular entity's language.

plach’s picture

I also think that distinct is the way to go here: since an entity has only one language there is no point in explictly specifying one to obtain the correct entity count. Unless @chx disagrees with this reasoning, I'd say #8 is RTBC.

chx’s picture

Status: Needs review » Reviewed & tested by the community

It is.

plach’s picture

Issue tags: +translatable fields
webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

That testX() function needs some PHPDoc. I added:

  /**
   * Tests querying translatable fields.
   */

And committed/pushed to 8.x. Thanks!

Needs a backport for D7.

loganfsmyth’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.42 KB
1.72 KB

Backported to D7 and added webchick's PHPDoc.

plach’s picture

Status: Needs review » Reviewed & tested by the community

nice!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Thank you!

Committed and pushed to 7.x.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.