Add pager and tablesort to EntityFieldQuery

Talked this over with chx. He's posting an explanation, and I'm assigning this to myself

Patch coming in 10, 9, 8, 7...

The pager stuff is trivial and done.

However, I'm having trouble with tablesort.
It works okay in PagerQuery, but in field_sql_storage I am having problems (because of the conversion of the efg_header array to a orderByHeader array, and guessing the field names, table prefixes....)
Would have been much easier to just do our own ordering and skip tablesort.inc completely.

Will return to it tomorrow to try and sort out the uglyness.

Edit: Works, now it's only ugly. Should have something prettier for tomorrow.

Here's a work in progress patch.
It's not big, the problems I thought existed (with the joins) actually disappeared once I looked at the code rested & clear of mind.

As you can see, it still has some ugly moments.
Let me know your thoughts, and how it can be prettified.

My usage example looks like this:

  $header = array(
    'title' => array('data' => t('Title'), 'property' => 'title'),
    'type' => array('data' => t('Type'), 'entity' => 'bundle'),
    'author' => t('Author'),
    'secret' => array('data' => t('Secret'), 'field' => array('field_name' => 'field_secret', 'column' => 'value')),
    'status' => array('data' => t('Status'), 'property' => 'status'),
    'changed' => array('data' => t('Updated'), 'property' => 'changed', 'sort' => 'desc')
  );
  
  $query = new EntityFieldQuery;
  $query->entityCondition('entity_type', 'node');
  $query->pager(20);
  $query->tablesort($header);
  $result = $query->execute();
  
  $nodes = node_load_multiple(array_keys($result['node']));

  foreach ($nodes as $node) {
    $options[$node->nid] = array(
      // load the data here...
    );
  }

  $form['nodes'] = array(
    '#type' => 'tableselect',
    '#header' => $query->tablesort,
    '#options' => $options,
    '#empty' => t('No content available.'),
  );
  $form['pager'] = array('#markup' => theme('pager', array('tags' => NULL)));

Don't have a good feeling about using $query->tablesort directly at the end of the example, but since the array is transformed at query time it's either that or a getter function...

I can post the example module with the next iteration of the patch.

Here's a new patch.
Still no tests, but everything else should be much better and more polished.
Tried to better replicate the cramped core coding style (I usually put a blank line after the else, foreach closing braces for example, but the existing code doesn't do that...)

Updated to remove the ternary operator discussed on IRC.

Blocking issues that have been committed:
#902830: EntityFieldQuery: field_sql_storage can't handle entityOrderBy (fatal error)
#903110: Multiple pager support is partially broken.
#907760: EntityFieldQuery tests cleanup

Blocking issue that is ready to be committed:
#908798: EntityFieldQuery does not handle ordering correctly

And... new patch!
Passes all tests on my machine (with #908798: EntityFieldQuery does not handle ordering correctly applied.)

Changing status so that the bot doesn't choke.

Oh, didn't realize you wanted to go all the way, and emulate it.
Totally overlooked the benefit (having it implemented automatically for every storage driver), that sounds awesome.

Will cook something up in the next few days.

Here's a patch to look at.

Tablesort is now incredibly simple. I am berating myself about not thinking of it myself, instead of coming up with the #11 complex monstrosity.

Question: We here also have the if/elseif/elseif combination that checks for the order types (entity, property, field). Would such a use case be served better with a switch instead? We use if/elseif/elseif in #908798: EntityFieldQuery does not handle ordering correctly too, so I stayed with it for consistency.

However, I really don't like the new pager implementation. I hate it, actually.
The code doesn't even work (the clone fails), it's just there for reference (I don't care at the moment since I'm arguing against the approach)

1) We need to duplicate the $element setting logic that we fixed in #903110: Multiple pager support is partially broken.. And it's not covered by tests like in PagerDefault. Right now PagerDefault::$maxElement doesn't work since $maxElement is a protected property. Unless that is changed, EntityFieldQuery paging can't coexist with another ordinary pager on the same page (because of element overlapping).
2) The pager code still needs to reside in finishQuery because of the need to get the total number of results, and we can only do that after the whole query is assembled. And a field storage engine (like mongodb or cassandra) might not even call finishQuery because it does ranges or something else differently. That means we gain nothing by going through all the trouble of duplicating code (and objects...)

So, I'm suggestion a mixed approach for the final patch: the new tablesort + the old pager code.

That was the wrong patch, see this one instead.

Also, what I noticed when testing with a small module (and not just with simpletests), in order for the tablesort to work, the 'field' needs to be initialized, for example:

  $header = array(
    'title' => t('Title'),
    'type' => array('data' => t('Type'), 'type' => 'entity', 'specifier' => 'bundle', 'field' => ''),
  );

If 'field' is not present, the tablesort header cell is not a link (there's an isset() check in tablesort_header())

Also, do this loop before the tablesort calls and make sure to += field '' if there is ordering info in there so that tablesort is happy.

Something like that. We need to pass $headers by reference and then change it, since tablesort_header is called outside of EFQ.

However, my main points against the current pager still stand. As it is, it can't work. Currently it throws a fatal error with the example module, and the tests crash, so that's a good hint it sucks :)

Let's discuss it on IRC.

Discussed, edited, reviewed on IRC.

Nice new patch.
Passes all tests on my local install.

This one should have no leading whitespace.
It also fixes the leading whitespace problems that I snuck in #908798: EntityFieldQuery does not handle ordering correctly.
Since these two patches should go in together, I won't reroll the old one unless requested.

Good catch, no idea why I didn't move it when I made initializePager.

New patch, addressing the concerns above, and a comment or two more.
Too bad we can't call initializePager() in execute, that would remove the need to call it in every query callback... But still, adding one line to each field storage driver is not bad.

After consultation with chx moved initializePager to execute(), allowing pager support for all drivers with no code changes (plus a way for it to be disabled).

Edit: the current initializePager() call placement leads to possible issues with query altering. That needs to be taken care of (moving the init call to the top of the method or adding a flag showing that the query has already been altered).

/**
   * TRUE if the query has already been altered, FALSE if it hasn't.
   *
   * Used in alter hooks to check for cloned queries that have already been
   * altered prior to the clone.
   *
   * @var boolean
   */
  public $altered = FALSE;

Not sure if pager should be mentioned, or if it's enough.
Didn't see any EFQ alter docs, should I add a line to the execute() comments?

Let's try this one.

Updated.

#908798: EntityFieldQuery does not handle ordering correctly landed. The whitespace fixes got in there after all, so I'm rerolling this one, and letting the bot test it.

else if => elseif.

Code looks fine and testing of efq_views worked fine.

This is a pretty huge patch for this stage in the release cycle.

Can you explain why this is important and needs to be committed for D7? Then feel free to bump back to RTBC.

@webchick
You're fast!

This patch took a long time to mature because of other discovered issues that blocked it (multiple pagers, broken ordering, etc, etc).
So instead of August 31st we are having this conversation on October 9, which means I won't get upset if this gets postponed to Drupal 8.
It's in the "nice to have" category.

Points for considering it:
1) It breaks no APIs, changes no schemas, doesn't create no extra work for anyone using EntityFieldQuery.
2) Most of the patch size is due to tests (10 for tablesort, 4 for pagers) and docs. The added functionality is simple.
3) By giving a developer an easy way to use pagers and tablesorts with EntityFieldQuery, he can create full admin screens using EntityFieldQuery.
That was the only missing piece. With this, the D7 admin screens could use EFQ, that's how complete it is.
Since pagers and tablesorts are pretty essential for many use cases, it would be a shame for contrib authors to have to reinvent it in every module separately.
So in short: this completes the puzzle, and makes EntityFieldQuery 100% awesome.

Note that this patch adds a way for to check if a query has been altered, which is definitely needed for D7, even if the pager is implemented from contrib, so if this gets postponed, that should go in as a separate patch.

I'll be on IRC if you want to discuss this. Thanks!

#34: 885014.patch queued for re-testing.

#33: 885014.patch queued for re-testing.

Ok, I talked to chx about this one last time, and he pointed out that:

a) This is the last patch before EFQViews is ready
b) Even though it's late for these kinds of changes, EFQ itself wasn't added until June or something.
c) Despite its size, there is no possible way for it to break anything, because the only thing currently using it is EFQViews
d) This patch has been running live on Examiner.com severing a quinti-billion page views per second for several months and haven't broken anything.

Also, it's like bojanz's first patch, and he is awesome. ;)

So, given all that...

Committed #34 to HEAD. Great job!