Bulk deletion

Subscribe.

The field deletion problem makes my head hurt every time I think about it :)

I can't claim to completely grok FieldAPI so please correct me where I'm wrong.

Browsing through the code, I noticed that every field_attach_* API function and every hook_field_storage_* implementation requires the use of field_attach_extract_ids() to retrieve structured data from the amorphous $object argument. This makes sense because they need $id, $vid and $bundle to be useful, but this begs the question of why they are not formally part of the API (in the function definition) to begin with.

I might be missing something, but it seems to me if we can break out this required "key data" from the amorphous $object argument into the API (function definitions), we could avoid this problem by not including $object argument at all in the functions that do not insert/update data (_load, _load_revision, _delete, etc).

For example, it might work something like this:
(ignoring the multiple load problem for now)
function field_attach_load($bundle, $obj_type, $id, $vid = NULL, $cacheable = TRUE);
function field_attach_delete($bundle, $obj_type, $id, $vid = NULL, $cacheable = TRUE);
function field_attach_insert($bundle, $obj_type, $object, $id, $vid = NULL, $cacheable = TRUE);
function field_attach_update($bundle, $obj_type, $object, $id, $vid = NULL, $cacheable = TRUE);

So now we only use $object for "real" data, not as a structure that also holds keys used for internal use.

This change would have these positive effects:
1. The API is much clearer about what is required in the different field_attach_* functions (no guessing about what needs to be in an $object).
2. The hook_field_attach functions that don't insert/update data are intentionally prevented from relying on $object data that may or may not be available in any given circumstance (for example, it would prevent the author of a field module from writing in dependencies on a certain property of $object being available that is provided by another field that could be later removed by an administrator at any time)
3. hook_fieldable_info doesn't feel like a "normal" drupaly hook being called time after time from field_attach_extract_ids() and used just to parse a structured object. Since we no longer have to use it to describe the structured data in $object, it can be used to expose data as one would expect (eg the human readable name and bundles keys)
4. We would then be able to delete field data normally when the object doesn't exist. Actually, we would be able to delete the field even if the row in field_config didn't exist, since we'd just need to pass in the "key data" that is saved in field_config_instance and the field_data_X tables without $object_loading or field_attach_loading anything.
5. The delete of a single object could then be implemented without touching any fields at all (until bulk-delete). For example, say we're Ubuntu and we have distro-release nodes that contain DVD ISOs. A node_delete of a distro-release would simply do the regular node_delete stuff and mark fields deleted. Then, cron comes back later and deletes the ISO that may have caused the delete UI to stall for multiple seconds.

Does this make any sense? Am I missing something that requires us to use $object?

No time tonight to delve into this, just a quick note that our initial use case ("e.g. for image fields, a file needs to be removed from the filesystem" - timeout issues ensue) is actually flawed. My bad, I'm the one who brought it to the table at the FiC sprint :-(. I think filefield D6 was quite a bit in flux back then...

CCK D6 is particularly broken on the question of 'notifying field types of values being deleted', and this in fact doesn't stop imagefield/filefield from working, because filefield is in fact a 'file reference' field. Removing a reference to a file from a node doesn't mean you want to delete the file, and the same file can be referenced in different objects, and/or in different fields. Fielfield actually handles its own reference counts.

Just editing a node and changing a field value from 'a' to 'b' "deletes" value 'a', and we currently don't have any mechanism for field types to react on this - and AFAIK this has not stopped the existence of rich D6 field types. Deleting a field "deletes" a lot of values, but is it structurally different ?

I'm not saying this invalidates the need for bulk deletion - maybe we do need it and my point is moot, but given the costly implications on the code and APIs, we might want to take a step back and reconsider :
Are things really bad if we cannot warrant field types that they'll be notified whenever a "value" gets out of the system ?
It might be an issue if a "value" is an actual resource, but possibly not if it's a *reference* to a resource, and those resources have their own lifecycle. And a "foobar"-field needs to be implemented the 'reference' way if it wants to address the "I want to reuse existing foobars" common request anyway.

It's late, the above is quite possibly cryptic or irrelevant or both, sorry about that.

OK, I delved a little more into D6 filefield, and its reference system (which is quite similar to the one in D7) doesn't handle the 'delete field' or 'delete bundle' cases (stale files). Pondered around this a little, but I have to withdraw and apologize for the noise: I currently see no way to avoid 'delayed bulk delete' in Field API...

re #11: "pbs.module is capable of implementing this system".
Does pbs need to do anything on purge ?
As a 'secondary storage', it seems it can simply delete the rows (object delete) or columns (instance / field delete) immediately without bothering about notifications and delayed purge ?

I started to review the patch, but got caught by the clock. Looking great ! Here are my remarks so far (mainly minor points, I didn't start to look at the meaty places...):

- FieldBulkDeleteTestCase::setUp()
Couldn't we use the "API" func field_test_create_bundle() instead of variable_set('field_test_bundles', $this->bundles); ? I guess it has to do with the odd location for the parent::setUp(); call ?

- double spacing in PHPdocs for field_info_field(), field_info_field_by_id(), field_attach_query()

- _field_invoke_multiple(): $deleted = isset($options['deleted']) ? $options['deleted'] : 0;
Could be moved to the // Merge default options. section at the beginning of the function.
+ Shouldn't this 'deleted' option be added to field_invoke() too, for consistency ?

- field_attach_query(): hook_field_attach_pre_query() should receive the $limit param too ?

Delved a little more in the juicy bits of the code functions.
As you noted in a couples TODO, I suspect some purge hooks could be refactored or merged with existing hooks. We can see that when the code settles.

- _field_info_collate_fields() collects information about 'deleted' fields (in the field_ids key) but not about deleted instances ? is that not needed ?

- field_attach_query($limit): field_sql_storage_field_storage_query() queries on *values*, not full objects, and the results are currently sorted by delta only, so $query->range(0, $limit); will produce strange results (all 0 delta values, truncated objects...).
Maybe it's in fact OK for the purge stuff (which mainly operates on values rather than objects), but it could produce very suprising behavior with other uses of field_attach_query().

- do we need both hook_field_storage_delete() and hook_field_storage_purge() ?
in field_sql_storage.module at least, one calls the other.
+ (minor) having field_sql_storage_field_storage_purge() used both as a hook implementation and as a helper function called directly by field_sql_storage_field_storage_delete() is a little confusing when maintaining or debugging code. Some other parts of core do that I think, and this always makes me tick.

- in PHPdoc below @defgroup field_purge wraps to less than 80 chars.
+ double spacing

- field_purge():

  $fields = field_read_fields(array('deleted' => 1), array('include_deleted' => 1));
  foreach ($fields as $field) {
    if (!isset($field['bundles']) || count($field['bundles']) == 0) {

I don't think field_read_fields() returns $field['bundles'] ? That information is added in field_info_collate_fields().

- field_purge_data(): do we need to clear field cache at the end ? I'd think the cache does not include deleted-but-not-purged-yet values, so it's not stale after the actual purge ?

- Similarly, field_cache_clear() in field_purge_instance() and field_purge_field() is a bit tough. We need to clean the cached 'field info', since they contain informations about deleted fields, but the actual data cache could persist. Our field_cache_clear() is probably not granular enough...

- The newly introduced $options['deleted'] in _field_invoke_multiple() is currently used nowhere ?

"Solution: if f_a_query() has $limit and gets no complete objects to returns, it issues a second SQL query with LIMIT range,offset until it gets a complete object"

Agreed, but how do we know we didn't fetch all the values for the object ?
Is it problematic if the query has no limit and we control we only fetch $limit objects in a PHP loop that retrieves individual rows ? I'm not sure if it solves the memory / performance issues.
Other than that, I completely agree f_a_query() requires a $limit param. Question: does it also mean it needs an $offset param ?

"- The newly introduced $options['deleted'] in _field_invoke_multiple() is currently used nowhere ?
- It is used to control how instances to operate on are retrieved."

Sorry, I meant I see no actual call to _field_invoke_*() that makes use of it - which might explain why a bug in there doesn't affect the tests :-)

"why aren't we just using f_a_load() to get all the values for all of the objects returned by the query? I looked at the f_a_query() code and discovered... we are"

Not exactly, we're only calling hook_field_load() and hook_field_attach_load() on the values returned by hook_field_storage_query().
That is, the main difference between f_a_load() and f_a_query() is that hook_field_storage_query() is used instead of hook_field_storage_load() to provide the stored values.

"hook_field_storage_query() should only return object ids, and f_a_load() should be used to retrieve the data"

The difference is that hook_field_storage_query() only returns the subset of values that match the conditions.
I'm not completely sure whether that's a limitation or a feature...

About the PHPdoc : hm, true, but I'd say it's rather the one-liner that is inaccurate ;-). The rest of the PHPdoc clearly states 'not all values'.

"Perhaps I do not need to care about whether f_a_query() returns values or objects; I can ask it to return only ids, and then use f_a_load()"
Well, this sidesteps the issue for the bulk delete case, but leaves the question of $limit problematic for FIELD_QUERY_RETURN_VALUES.

Taking a step back. So, we need $limit. But then we also need $offset, because there's no way to get all results without - unless the operation done on a bunch of results deletes the corresponding rows, in which case you always use offset 0. That's the case for bulk delete, but not for other f_a_query() use cases.

The problem is that
- for the code calling _f_a_query(), the only unit that makes sense for $limit / $offset is 'full objects' (# of objects to retrieve, # of objects to skip). $limit in 'values/db rows' is unpredictable: leads to incomplete objects, and you can get n < $limit results while there are still results to be loaded.
- but the SQL query built in field_sql_storage_field_storage_query() is only able to use 'db rows' as a unit.

The following logic for field_sql_storage_field_storage_query($limit) should ensure it actually returns $limit (or less) full objects, whatever the result mode (return ids or return values):

$query = "the query, without range for now, sorted by obj_id and delta"
$offset = 0;
while (the previous iteration returned $limit rows) {
  $query->range($offset, $limit);
  $results = $query->execute(); 
  foreach ($results as $row) {
    $offset++;
    if (we hit ($limit + 1) different objects) {
      break the while loop;
    }
    put the row into our $results array;
  }    
}

For the $offset, the only way I see right now is that f_a_query() takes $offset by ref, and field_sql_storage_field_storage_query() changes it into 'the value that you need to provide if you want the next bunch of results".
$limit is in 'objects', $offset is in 'whatever makes sense for the storage engine' (you'll usually start at 0 anyway)

Not completely straightforward, but that's the best I can think of

"It seems like we can start with ->range(0, $limit), no?"
well, the others in the following while() iterations will need to adjust $query->range($offset, $limit); anyway

Sure, I'm not suggesting we run an unlimited query; rather: we *build* the query without range, then execute it several times in a while() loop that adds the relevant range each time.
Anyway, I'm pretty sure we mean the same thing :-)

and if anything changes in the data, we'll end up returning objects with incorrect data items

You mean, if the calling code does data writes between f_a_query(some bunch) and f_a_query(next bunch) ? Hm, yes, I guess that would mess the $offset pretty bad...

please remind me why we absolutely must support FIELD_QUERY_RETURN_VALUES

Well - it all started in #392494-5: Fields API needs to allow searching of scalars, by yourself ;-)
I wouldn't die if we removed that and went for "Call f_a_query() to get ids, then load the objects if you need the values".
Only thing we lose AFAICT is : If I want only to reason / act on the field values that match the actual conditions, I need to filter them out of the loaded objects myself.

But: doesn't FIELD_QUERY_RETURN_IDS have the exact same $limit / $offset mismatch issues ? The query it runs is still based on 1 row = 1 value != 1 object...
FIELD_QUERY_RETURN_VALUES just adds to the 'SELECT' part - unless I'm mistaken, this is not where our problem lies.

I'm not sure there's a way to work with $limit / $offset without persisting the 'query result' resource. I see this turn into a 'design a mini DBTNG API, for generic, non-PDO, non SQL backends' :-(

/me feels a bit dizzy looking at the can of worms that is pluggable storage while we current don't gain any actual benefits...

Agreed on the conclusions. Off with RETURN_VALUES - doh, that's where 80% of the work on f_a_query() went :-)

The 'single field' mode for _field_invoke(_multiple) was primarily introduced for RETURN_VALUES and value massaging, but I don't think we need to remove it.
field_view_field(), for instance (shorthand API function to display a single field) doesn't currently use it, but technically it should.

No time to review tonight.

Firing purge on cron with a variable-based size sounds fine IMO. We could add a UI for the variable, but there's currently no other 'Field API setting', and I'm not sure this warrants one...

I guess field_ui could add a dsm('Field foo was marked as deleted. Data will be purged progressively. Click [[here|link_to_batch_page]] to purge right now'). Doesn't sound too difficult, but that definitely doesn't belong in this patch.

Awesome work. Remarks below are mainly details.

- _field_invoke(), _field_invoke_multiple():
$options['deleted'] is 0/1 based, while $options['default'] is TRUE/FALSE based. It's really a boolean flag, isn't it ?
$options['field_name'] is changed to $options['field_id'] in _field_invoke_multiple(), but left untouched in _field_invoke() (though field_purge_data() uses _field_invoke($options['field_id'])). Ideally, I'd say both functions should support both options ?

- _field_invoke_multiple();
There are a couple places where $instance['field_id'] could be replaced by the newly added $field_id variable.

- I'm wondering if the $field_id param for field_attach_query() couldn't accept a field_id *or* a field_name. Would just require we don't allow field names to be all numeric. Seems reasonable to me, we already limit to [a-z0-9_].

- field_sql_storage_field_storage_load()
foreach ($instances as $field_name => $instance) {: field_read_instances() results are just numerically indexed arrays, so we cannot assume $field_name as keys.

- field_sql_storage_field_storage_query()

+    // TODO: This probably does not match an index.
+    // TODO: I do not think we need to sort at all any more.
+    //->orderBy(array('etid', 'entity_id', 'delta'))

We still need to sort on etid, entity_id, since the code assumes we rows for a given object in a sequence.
Currently the data tables have a unique index on array('etid', 'entity_id', 'deleted', 'delta'), so this should be good enough, right ?

+      
+      $return[$row->type][$id] = field_attach_create_stub_object($row->type, array($row->entity_id, $row->revision_id, $row->bundle));
+      --$count;

Looks like a wrong reroll after #512236: Design flaws in field_attach_query(), this hunk is not needed anymore (code is already present above).

- field_sql_storage_field_storage_delete() relies on $instances = field_info_instances($bundle);, and thus purges data only for non-deleted instances.
So if an instance is marked as deleted, and just after that, a node is deleted, field_attach_delete($node) will leave the data for the marked-as-deleted instance - but they will be caught by the next purge, right ? (Just to be sure)

- field_purge():

   // TODO: Perhaps the include_deleted argument to this
  // function should be replaced by inspecting the query parameters.

I don't think I get this.

- field_purge_instance() and field_purge_field() should call _field_info_clear_cache() instead of field_cache_clear() ?

- _field_info_clear_cache() should be ..._cache_clear(). Also, not sure if we want the function private or public.

- Still trying to sort out TODO: refactor field_purge_data() with field_attach_delete()? :-)

- shouldn't field_sql_storage.module implement hook_field_storage_purge_field() ?

- field.test, FieldBulkDeleteTestCase::getInfo()

      'name'  => t('Field bulk delete tests'),
      'description'  => t("Bulk delete fields and instances, and clean up afterwards."),

Core doesn't translate those strings anymore + double spacing before the => (those were fixed in the rest of field.test meanwhile)

- field.test: missing PHPdoc for _generateStubObjects() and testDeleteFieldInstance()

- field.test, setUp():
Minor: field_test_create_bundle() doesn't actually need the $desc param.
Minor: why 'bb' and 'bf' for bundles and fields names, instead of just 'b' and 'f' ?

- field.test: do we need the tearDown() at all ? I though the target db was reinitialized after each test method ?
It seems we could live with no tearDown() and $this->fields = array(); $this->instances = array(); moved at the beginning of setUp() ?

- field.test: A few one-line comments miss a trailing point. Multiline comments could wrap at a longer line length. A comment in testPurgeInstance() has a double spacing.

- I created #523842: introduce FieldWebTestClass wrapper class to avoid redefining _generateTestFieldValues() in each test class. Should not hold this patch back, obviously.

- Not for this patch, but it seems we're missing a hook_field_storage_pre_delete() ? How does PBS react on object deletion ? (would probably be best in a separate thread, unless you have a no-brainer 'no need' answer...)

- Probably not for this patch either, but 'fieldable comments' might raise the question of 'immediate delete vs bulked delete' for single object deletion too :-/
A comment being deleted triggers deletion of all its subthread...

I gave this another careful reading:

- Fixed a couple remaining double spaces

- Unified the PHPdocs for field_id/field_name options in _field_invoke()/_field_invoke_multiple().

- About // TODO: Perhaps the include_deleted argument to field_read_instances() should be replaced by inspecting the query parameters. and

f_a_query() takes a list of query parameters including 'deleted'. field_read_instances() takes a list of query parameters, with a separate list of parameters for 'deleted' and 'inactive'. I'm thinking that 'deleted' at least should just be a normal query parameter; for consistency, it should default to FALSE.

The difference is that a condition on 'deleted' in f_a_query() restricts results to either 'deleted only' or 'non-deleted only', while field_read_instances() does either 'non-deleted only' or 'both deleted and non-deleted'. Which IMO fits the usual use cases of both functions, so the 'inconsistency' is OK by me.
Of course, this means that if you want to get 'only non-deleted instances', you need a somewhat verbose field_read_instances(array('deleted' => 1), array('include_deleted' => 1)); - but apart from field_purge(), I see no real use case for this.
I removed the related TODOs in field_purge().

- Updated the PHPdoc for _field_info_collate_fields(), and reworked the function to trigger a single field_read_fields() call, and not store non-deleted fields twice in the cache.

- Which outlines: _field_info_collate_fields() collates all fields (including deleted) in 'field_ids', but only collects non-deleted instances.
Which translates to: we provide field_info_field_by_id() but not field_info_instance_by_id().
If we did, then field_purge() could use field_info_fields_by_id() and field_info_instances_by_id() instead of field_read_fields() / field_read_instances(), and get rid of the "We cannot use function X because it does Y" comments and the dance between field_read_*() and field_info_*() in field_purge().
I have no strong bias on this, just a suggestion.

Other than this last suggestion, this looks ready to me :-)

The more I think about it, the more I'm convinced that single object deletion should be bulked as well.
- It would address the questions of 'merge field_purge_data() and field_attach_delete()'.
- It would simplify the explanations in @defgroup field_purge quite a lot :-). One single model for data deletion.
- 'Fieldable comments' bring the most obvious cases where a single-object delete can cascade to arbitrary amounts of data deletion (node delete -> all attached comments should be deleted; comment deleted -> the comment subthread should be deleted). But 'cascade deletion through nodereference fields' is a long-standing D6 request (optional, obviously), that would also imply such scenarios, I'm sure we can think of others...
As I said earlier, I'm perfectly fine with keeping this for a followup, though.

I see your point on dropping 'include_deleted' in field_read_[fields|instances]() in favor of a 'deleted = TRUE|FALSE' in $param, and I agree.
This is actually inline with my intent in #535034: Clean-up how fields and instances are prepared for runtime. to make field_info_*() the primary provider for $field and $instances definitions.
If _field_info_collate_types() collects both deleted and non-deleted fields/instances, there should be very little use cases for field_read_*().

Then it's "just" a question of what is the right set of helper _info functions to get $fields and $instances out of _field_info_collate_types():
fields, instances, by name, by id, deleted + non-deleted, only non-deleted.
- 'deleted + non-deleted' vs 'only non-deleted' sound like an 'include_deleted' param to me ;-).
- I make a second attempt with my suggestion to have $name arguments for field_info_fields() and field_info_instances() accept ids too... would get rid of the _by_id variants...

That would be:
field_info_fields($include_deleted = FALSE);
field_info_instances($bundle, $include_deleted = FALSE);
field_info_field($name_or_id);
field_info_instance($name_or_id);

Looks just fine to me. Yay !
Updated patch just fixes 80 chars line-wraps on comments and a couple double spaces, and moves field_cron() with other core hooks towards the top of field.module.

Minor nitpick: I think we could be a bit more ambitious on the default value for the 'field_purge_batch_size' variable. Maybe 50 or 100 instead of 10 ?
OTOH, it's not completely clear to me what tasks filefield will actually perform when purging a value...

Well, I don't think this should hold the patch anyway. This is RTBC.

Found a few other comments to fix in field.test.

+ FieldBulkDeleteTestCase::setUp():

    // TODO: Create a second field that we do not delete so we can
    // test that its data stays around.

It *looks* like this is actually done, but I'll let you confirm.

A rather quick, coding-standards only review:

+++ modules/field/field.attach.inc	6 Aug 2009 04:25:26 -0000
@@ -152,6 +152,11 @@ class FieldQueryException extends FieldE
+ *  - 'field_id'
+ *    The id of the field whose operation should be invoked. By default, the
+ *    operation is invoked on all the fields in the objects' bundles.
@@ -161,22 +166,35 @@ class FieldQueryException extends FieldE
+ *  - 'deleted'
+ *    If TRUE, the function will operate on deleted fields as well as
+ *    non-deleted fields. If unset or FALSE, only non-deleted fields are
+ *    operated on.

Yes. The other code gets it wrong. But we actually do:

 *  - 'field_id': The id of the field whose operation should be invoked. By
 *    default, the operation is invoked on all fields in the objects' bundles.

There is no need and no point in introducing wrong code just to adhere to equally already existing, wrong code.

(and elsewhere)

+++ modules/field/field.attach.inc	6 Aug 2009 04:25:26 -0000
@@ -161,22 +166,35 @@ class FieldQueryException extends FieldE
-    if (empty($options['field_name']) || $options['field_name'] == $field_name) {
+    if ((empty($options['field_id']) || $options['field_id'] == $instance['field_id']) && (empty($options['field_name']) || $options['field_name'] == $field_name)) {
@@ -259,22 +287,36 @@ function _field_invoke_multiple($op, $ob
-      if (empty($options['field_name']) || $options['field_name'] == $field_name) {
+      if ((empty($options['field_id']) || $options['field_id'] == $field_id) && (empty($options['field_name']) || $options['field_name'] == $field_name)) {

empty() should be replaced with !isset() here to

a) safe some cycles

b) make the actual condition more understandable

c) make any sense of the actual condition.

+++ modules/field/field.attach.inc	6 Aug 2009 04:25:26 -0000
@@ -259,22 +287,36 @@ function _field_invoke_multiple($op, $ob
+  // contain data for a single field for each name, even if that field is deleted,

Minor: This line exceeds 80 chars.

+++ modules/field/field.crud.inc	6 Aug 2009 04:25:26 -0000
@@ -681,3 +684,218 @@ function field_delete_instance($field_na
+ *   * Invoking the Field Type API hook_field_delete() for each field on the
+ * object. The hook for each field type receives the object and the specific
+ * field being deleted. A file field module might use this hook to delete
+ * uploaded files from the filesystem.
+ *   * Invoking the Field Storage API hook_field_storage_delete() to remove
+ * data from the primary field storage. The hook implementation receives the
+ * object being deleted and deletes data for all of the object's bundle's
+ * fields.
+ *   * Invoking the global Field Attach API hook_field_attach_delete() for all
+ * modules that implement it. Each hook implementation receives the object
+ * being deleted and can operate on whichever subset of the object's bundle's
+ * fields it chooses to.

Lists in PHPDoc, we usually format as:

 * The list:
 * - One.
 * - Two is a list item that is much longer than eighty chars and must thus be
 *   wrapped onto the next line.
 * - Three.
 

+++ modules/field/field.module	6 Aug 2009 04:25:26 -0000
@@ -178,6 +182,16 @@ function field_theme() {
+ * Purges some deleted Field API data, if any exists.

s/some//

+++ modules/field/field.test	6 Aug 2009 04:25:26 -0000
@@ -1755,3 +1762,232 @@ class FieldInstanceCrudTestCase extends 
+   * Given an array of potentially fully-populated objects and an
+   * optional field name, generate an array of stub objects of the
+   * same fieldable type which contains the data for the field name
+   * (if given).
...
+   * Verify that deleting an instance leaves the field data items in
+   * the database and that the appropriate Field API functions can
+   * operate on the deleted data and instance.
+   *

PHPDoc summary missing - contains a description.

22 days to code freeze. Better review yourself.

Documentation in here is excellent; great job capturing the original thought behind the issue. It's kind of crazy that we jump through these kinds of hoops for fields, when we don't jump through them for anything else: nodes, files, etc. But I guess you have to start somewhere. :P

+++ modules/field/field.crud.inc	6 Aug 2009 04:25:26 -0000
@@ -681,3 +684,218 @@ function field_delete_instance($field_na
+function field_purge($batch_size) {

I got turned around in the tests trying to figure this function out, especially from these lines:

+      // Purge the data.
+      field_purge(10);
+
+      // Purge again to purge the instance.
+      field_purge(0);

At first I thought 10 was the field ID, but then I had no idea why 0 was passed in the second time. I had to go looking through the docs to discover that it's a *batch* purge operation, which I never would've guessed.

I thought this was named field_purge for symmetry with other field API functions, but at http://api.drupal.org/api/file/modules/field/field.crud.inc/7 I don't see any other two-word functions. Could we rename this function to field_batch_purge() or something similar that is more indicative of what it does?

Also, should $batch_size default to 0? that seems to be the most common value.

One other thing: I applied this patch and then installed CCK and it still didn't make fields delete. I guess this is because the purge stuff was missing. But it makes it really hard to "test" this functionality without a UI around it to see how it affects users. I guess the important thing is to get the API change in now so we can tweak that stuff after code freeze.

The rest of these comments are minor, and sun may have already caught in his review above (I haven't read it yet cos I was trying to come into this fresh). Fix those up, and I think this is good to go from my perspective.

+++ modules/field/field.test	6 Aug 2009 04:25:26 -0000
@@ -1755,3 +1762,232 @@ class FieldInstanceCrudTestCase extends 
+class FieldBulkDeleteTestCase extends DrupalWebTestCase {

Line of summary PHPDoc please.

+++ modules/field/field.test	6 Aug 2009 04:25:26 -0000
@@ -1755,3 +1762,232 @@ class FieldInstanceCrudTestCase extends 
+    $this->bundles = array('bb_1' => 'bb_1', 'bb_2' => 'bb_2');

drupal_map_assoc() might be slightly cleaner, but since there are only two here it's probably ok.

+++ modules/field/field.test	6 Aug 2009 04:25:26 -0000
@@ -1755,3 +1762,232 @@ class FieldInstanceCrudTestCase extends 
+   * but depends on this class' setUp().

class's

+++ modules/field/field.test	6 Aug 2009 04:25:26 -0000
@@ -1755,3 +1762,232 @@ class FieldInstanceCrudTestCase extends 
+  function testPurgeInstance() {
...
+  function testPurgeField() {

A line of summary PHPDoc needs to be added to these testX functions.

+++ modules/field/field.test	6 Aug 2009 04:25:26 -0000
@@ -1755,3 +1762,232 @@ class FieldInstanceCrudTestCase extends 
+      $this->assertEqual($stubs[$obj->ftid], $obj, "hook_field_delete() called with the correct stub {$obj->ftid}");

Single quotes around the assertion.

+++ modules/field/modules/field_sql_storage/field_sql_storage.module	6 Aug 2009 04:25:26 -0000
@@ -337,39 +350,54 @@ function field_sql_storage_field_storage
+    // We need to require objects arrive in a consistent order for
+    // range() operation to work.

Probably 'ensure that' objects arrive?

+++ modules/simpletest/tests/field_test.module	6 Aug 2009 04:25:26 -0000
@@ -640,4 +640,28 @@ function field_test_memorize($key = NULL
+/**
+ * Memorize calls to hook_field_delete().
+ */
+function field_test_field_delete($obj_type, $object, $field, $instance, $items) {
+  $args = func_get_args();
+  field_test_memorize(__FUNCTION__, $args);
+}

I know these weren't introduced in this patch, but could you remind me what this is again?

This review is powered by Dreditor.

Minor : field_purge_batch() mistakens the function for a batch API callback :-/. Same old problem with APIs / modules stealing an existing, current language word instead of adding lingo...
I don't see much use cases for that function to be called directly, except here in hook_cron, and possibly in field UI for an actual 'batch API' feature ("purge all data *now*" after a field / instance has been deleted in the UI), so I'm not sure the function name matters that much to the outside world; while internally the confusion with batch API is technically unfortunate IMO.
field_purge() was kind of OK with me :-). Maybe field_purge_deleted() if we don't want 2-word functions ?

Excellent work -- yched and bjaspan continue to impress. Committed to CVS.

I wanted to report a potential issue with this commit. My existing D7 install from before this commit (no updates needed) broke:

Visiting /user I get:
Fatal error: Unsupported operand types in /home/rfay/workspace/d7git/modules/field/field.info.inc on line 264

after this commit. I tried to completely debug it but ran out of time - I apologize. I did find that in field.info.inc, _field_info_collate_fields() starting at 216:

    foreach ($definitions['instances'] as $instance) {
      $field = $info['fields'][$instance['field_name']];
      $instance = _field_info_prepare_instance($instance, $field);

$field is null, and that's the issue. I don't understand the data structures well enough to understand *why* it ended up null.

I do have the database that demonstrates the problem and will provide if if you're interested.

I do *not* see the issue if I do a clean install with today's D7.