Noticed a few things that need fixing, and didn't want to kill kittens in #885014: Add pager and tablesort to EntityFieldQuery, so here it goes.

1) field_attach_create_bundle() is called with the params reversed.

field_attach_create_bundle('bundle1', 'test_entity_bundle_key');
field_attach_create_bundle('bundle2', 'test_entity_bundle_key');
field_attach_create_bundle('test_bundle', 'test_entity');

Here we pass it $bundle, $entity_type, but it should be in reverse ($entity_type, $bundle)

2) The ordering methods sometimes get 'ASC' and 'DESC', and sometimes 'asc' and 'desc'. Since the core default is uppercase, it should be the same here.

3) We create entities for the "test_entity_bundle_key" entity type, which has two bundles: bundle1 and bundle2.
Those entities have values in a sample field, but that field is only attached to bundle1 and not bundle2.
This makes it impossible to test ordering by bundle if the query goes through field storage.
So two of the tests I added in #902830: EntityFieldQuery: field_sql_storage can't handle entityOrderBy (fatal error) look kinda silly because they test ordering by bundle, but the bundle is the same for each entry.
I will also need this for the tablesort integration tests.

CommentFileSizeAuthor
#9 907760.patch11.94 KBbojanz
#1 907760.patch10.32 KBbojanz

Comments

bojanz’s picture

bojanz commented
Status: Active » Needs review
StatusFileSize
new 907760.patch10.32 KB

Here's a patch.

chx’s picture

chx commented

Note: field_attach_create_bundle in core does particularly nothing :) so it's no wonder the param swap went undetected.

bojanz’s picture

bojanz commented

Yeah, I've noticed, that's why it's in the "misc" patch ;)

Status: Needs review » Needs work

The last submitted patch, 907760.patch, failed testing.

bojanz’s picture

bojanz commented
Status: Needs work » Needs review

#1: 907760.patch queued for re-testing.

EDIT: And...green.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

This cleanup makes sense. We have verified together that no specific filtering functionality gets lost with these changes, the fltering is like greater than zero in all cases, the specific filter tests are not affected.

bojanz’s picture

bojanz commented
Assigned: Unassigned » bojanz
Status: Reviewed & tested by the community » Needs work

Our conversation on IRC got me thinking.
Sure, this patch doesn't make the tests less precise. But are they as precise as they can be?
I went through the code once again and found a case (4 tests) which could be better.

// Test entity sort by bundle.
    $query = new EntityFieldQuery();
    $query
      ->entityCondition('entity_type', 'test_entity_bundle_key')
      ->entityOrderBy('bundle', 'ASC')
      ->propertyOrderBy('ftid', 'ASC');
    $this->assertEntityFieldQuery($query, array(
      array('test_entity_bundle_key', 1),
      array('test_entity_bundle_key', 2),
      array('test_entity_bundle_key', 3),
      array('test_entity_bundle_key', 4),
      array('test_entity_bundle_key', 5),
      array('test_entity_bundle_key', 6),
    ), t('Test sort entity bundle in ascending order.'), TRUE);

    $query = new EntityFieldQuery();
    $query
      ->entityCondition('entity_type', 'test_entity_bundle_key')
      ->entityOrderBy('bundle', 'DESC')
      ->propertyOrderBy('ftid', 'DESC');
    $this->assertEntityFieldQuery($query, array(
      array('test_entity_bundle_key', 6),
      array('test_entity_bundle_key', 5),
      array('test_entity_bundle_key', 4),
      array('test_entity_bundle_key', 3),
      array('test_entity_bundle_key', 2),
      array('test_entity_bundle_key', 1),
    ), t('Test sort entity bundle in descending order.'), TRUE);
    
    // Test entity sort by bundle, with a field condition.
    $query = new EntityFieldQuery();
    $query
      ->entityCondition('entity_type', 'test_entity_bundle_key')
      ->fieldCondition($this->fields[0], 'value', 0, '>')
      ->entityOrderBy('bundle', 'ASC')
      ->propertyOrderBy('ftid', 'ASC');
    $this->assertEntityFieldQuery($query, array(
      array('test_entity_bundle_key', 1),
      array('test_entity_bundle_key', 2),
      array('test_entity_bundle_key', 3),
      array('test_entity_bundle_key', 4),
      array('test_entity_bundle_key', 5),
      array('test_entity_bundle_key', 6),
    ), t('Test sort entity bundle in ascending order, with a field condition.'), TRUE);

    $query = new EntityFieldQuery();
    $query
      ->entityCondition('entity_type', 'test_entity_bundle_key')
      ->fieldCondition($this->fields[0], 'value', 0, '>')
      ->entityOrderBy('bundle', 'DESC')
      ->propertyOrderBy('ftid', 'DESC');
    $this->assertEntityFieldQuery($query, array(
      array('test_entity_bundle_key', 6),
      array('test_entity_bundle_key', 5),
      array('test_entity_bundle_key', 4),
      array('test_entity_bundle_key', 3),
      array('test_entity_bundle_key', 2),
      array('test_entity_bundle_key', 1),
    ), t('Test sort entity bundle in descending order, with a field condition.'), TRUE);

In this case, even if the entityOrderBy call doesn't work, the propertyOrderBy will order things well enough for the tests to succeed. Let's make that better while we're at it.

Edit: So I fixed the tests, and found that the last two (field storage ones) are failing.
Debugged the query, and it turns out that the query is first ordered by property ftid, and then by the bundle, which means that it doesn't get ordered by bundle at all. This is because field storage adds the orderings in a different order than propertyQuery.

bojanz’s picture

bojanz commented
Status: Needs work » Needs review
StatusFileSize
new 907760.patch11.94 KB

Attaching a patch with the first two tests mentioned above fixed.
The other two are indicative of a larger problem, so I didn't touch them. Will be handled in #908798: EntityFieldQuery does not handle ordering correctly.
Let's get this one in.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

Thanks for the extreme through work!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Great work! Committed to HEAD.

Status: Fixed » Closed (fixed)

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