Design flaws in field_attach_query()

Easy to read, well-documented and tested. Committed to CVS HEAD.

Followup for the hooks doc.

Barry: no patch :-)

Also : not sure of what

 * @param $count
 *   The number of results that is requested. This is only a
 *   hint to the storage engine(s); callers should be prepared to
 *   handle fewer or more results.

means for calling code. I.e : "when do I know I got all the results and can stop iterating over f_a_query() ?" when I get less than $count results ? when results is empty ?.
If I trust field_sql_storage_field_storage_query(), it seems "when I get less than $count results" works. Only thing is: results being keyed by object type, counting the actual # of results is (moderately) tedious.

I'm wondering if hook_field_storage_query() could set $cursor to a special value (defined constant equal to -1 ?) when it knows we're done (that would be when $count > 0 after exiting the loop ?)

_taxonomy_clean_field_cache() in #491190: Provide a taxonomy term field type (clean relevant field cache entries when a term is updated or deleted) is going to be one of the 1st actual use case, and I'm not sure which sample calling code to advise there.

- PHPdoc for field_attach_load() and field_attach_load_revision(), $options param:
"Note that returned objects may contain data for other fields, for example if they are read from a cache."
Doesn't seem true, since the code sets $cacheable to FALSE if there is an $options['field_name'], and nothing will be read from cache ?

- Do we still want to have field_sql_storage_field_storage_query() return "stub objects" ? Previously we only returned
$return[$row->type][$id] = $row->entity_id; for FIELD_QUERY_RETURN_IDS.
Why not, I guess. The caller will not be able to build those stub objects with just an id if he needs them (which I suspect is the case for bulk delete ?)

- Optimization in field_sql_storage_field_storage_query: when arriving at the last rows, and the last query returns n < (query range) rows, the code recently committed needs an additional query returning *no* rows before we can exit the do-while loop.
Similarly, with patch #11, FIELD_QUERY_COMPLETE is set only if no results were found, so f_a_query() callers always need an additional call after the one which returned the last actual results (thus an additional 'empty' query).

Attached patch should get rid of those extra queries by keeping two separate counters ($row_count and $obj_count). When the query asked for n rows returns less than n rows, we can exit the do-while and set FIELD_QUERY_COMPLETE (I also changed the code from decremental counters to incremental counters, having ++s mixed with --s was a bit confusing)

Also, removed the references to FIELD_QUERY_RETURN_IDS in the tests.
I didn't change anything related to the first two remarks, waiting for Barry's confirmation.

Doh, same patch without commenting out tests in FieldAttachTestCase (I often comment out unrelated tests so that I can faster run and re-run the test that interests me, and regularly forget to uncomment before rolling patches...)

Sigh. And without the (done) note-to-self TODO.

$cache_read and $cache_write sound good.

Updated f_a_load() with $cache_read and $cache_write.

Which should make this ready.

Test are green, so RTBC.

Rerolled. Getting this one in would make it easier to reroll and review #367753: Bulk deletion.

Doh, here's the patch.

Also, Dries: webchick prefers to leave this one to you, since you're already part of it.

Committed to CVS HEAD. Thanks.