Deleting node type with only instance of a field leaves the field in a strange zombie state

This may be by design but for now i will just demote priority. see http://drupal.org/node/45111

Couple questions:
What happens when deleting a node that uses a field shared with other content types?
How does D6 handle this now?

Deleting a content type can leave thousands of nodes in "a strange zombie state". It has been so since always. IMO this still is not Major.

+++ modules/field/field.crud.inc
@@ -569,22 +569,13 @@ function field_read_fields($params = array(), $include_additional = array()) {
+function _field_delete_field($field_name) {

@@ -602,6 +593,28 @@ function field_delete_field($field_name) {
+function field_delete_field($field_name) {

@@ -914,7 +927,7 @@ function field_read_instances($params = array(), $include_additional = array())
-function field_delete_instance($instance) {
+function _field_delete_instance($instance) {

No need for those separate private functions. Let's incorporate that code into the primary API functions, please.

Powered by Dreditor.

Agreed with sun. That was the approach identified in #838022: Deleting a content type leaves orphaned fields (older, but there's a patch here, so marking the other one as duplicate).

_field_delete_field() / field_delete_field() is utterly confusing.

I don't see why the attached patch wouldn't do the job ?

Subscribe, also running into this problem.

we need clear the field_info cache before fetching the $field from field_info_field().

looks perfectly sane.

I'll get back at this after RC1

patch failures in #17 were caused by field_delete_field() first calling field_delete_instance() on each instance, which in turn calls field_delete_field() when deleting the last instance.

jbrown's #21 solves that by splitting field_delete_field() and field_delete_instance() in two functions each, introducing field_delete_field_record() and field_delete_instance_record() functions. Fixes the tests, but obfuscates the code flow, and we really don't want to clutter the API namespace with functions that, in addition, do not correspond to any action we want to promote to the API.

I'll post an updated patch asap.

So, issue summary :
- Contrary to CCK, Field API cleanly separates $field and $instance - and that is great. The CRUD APIs for fields and instances are separated and (to a large extent) independent. API wise, you can have a field without instances (yet, or anymore). field_delete_instance() currently doesn't clean orphaned fields, and that was a conscious design choice.

- To keep UX reasonable, Field UI, though, doesn't present the two separate notions. You manipulate (create, reuse, delete) *instances*, and fields are silently created and deleted accordingly. This is all for the best IMO, and won't change now anyway.
Consequence : as far as Field UI is concerned, a field without instances doesn't exist - it's not visible anywhere in the UI, you can't do anything with it (delete it completely, or create a new instance). It only bites you when you want to create a new field with the same name : 'a field with the same name already exists'. --> so-called 'zombie' field.
It's only a problem if the field was created by Field UI originally and thus lies in the 'field_' namespace. Other fields just don't bug you, you can't clash with them.

- Field UI makes sure none of its UI action leads to a lone field without instances : when deleting the last instance, it deletes the field as well.
Problem : there's another way to delete instances in the UI : deleting a node type - or a more generally a bundle. Field UI doesn't control that, this just triggers an API call to field_attach_delete_bundle(), which does a series of field_delete_instance(). If a field wasn't used anywhere else, it stays around with no instance.

Solution : field_delete_instance() needs to be able to garbage collect the corresponding field if it gets orphaned. We need this behavior to be optional, because :
- field_delete_field() already calls field_delete_instance(), and we need to avoid infinite loops
- it makes sense to keep the ability to programmatically delete and create instances without caring about order (i.e having to take care of not deleting the last instance before trying to add new ones). I'm notably thinking of Features having to perform complex create / delete instances when doing a revert.

Attached patch just does :

-function field_delete_instance($instance) {
+function field_delete_instance($instance, $field_cleanup = TRUE) {

As a side effect, the changes in field_purge_batch() fix #566048: Attempt to purge a field that still has instances.

Looks straightforward. Well done.

+ * @param $field_cleanup
+ *   If FALSE, no cleanup is performed to delete orphaned fields without
+ *   non-deleted instances. Defaults to TRUE.

Sorry, but I am completely turned around by all the negatives in that sentence. Can we explain what *true* does instead?

Also, are the tests here sufficient, or should they be expanded? #23 describes two separate code paths: one for programmatic fields and one for fields created and managed through field UI. I would've expected more hunks to be added in this patch (I'm only reading the diff, though...)

Reworded the PHPdoc :

 * @param $field_cleanup
 *   If TRUE, the field will be deleted as well if its last instance is being
 *   deleted. If FALSE, it is the caller's responsability to handle the case of
 *   orphaned fields without non-deleted instances.Defaults to TRUE.

About tests :
Field deletion through Field UI is already tested in FieldUIManageFieldsTestCase::testDeleteField(), and that test stays the same (thus doesn't appear in the diff).
The 'garbage collect orphan fields' behavior in field_delete_field() is tested in FieldInstanceCrudTestCase::testDeleteFieldInstance(), and as a side effect in FieldBulkDeleteTestCase::testPurgeField().

Oops, error while rolling the patch.

Fix double full stop at the end of a comment.

The approach looks really sane. RTBC from me if the test bot comes back green.

I still found the "FALSE" part of the comment hard to read. I stumbled over "without non-deleted instances".

Nit-picks: Double space after "TRUE,", no space before "Defaults", "responsability" should be "responsibility".

How about:

If the last instance is being deleted and TRUE, then the field will be 
deleted as well. If FALSE, then it is the caller's responsibility to handle
the orphaned field. Defaults to TRUE.

*   If TRUE, the field will be deleted as well if its last instance is being
*   deleted. If FALSE, it is the caller's responsibility to handle the case of
*   fields left without instances. Defaults to TRUE.

#33: field_delete_instance-915906-33.patch queued for re-testing.

Still green. Assuming wording has been fixed, bumping back to webchick.

#33: field_delete_instance-915906-33.patch queued for re-testing.

Ok, so before:

Create a content type called 'boom' with a field called 'bang'. Delete content type 'boom' which, presumably, takes field 'bang' with it. Create another content type called 'pow' with field called 'bang'. Get an error that the 'bang' field already exists.

Now:

Create a content type called 'yowza' with a field called 'shazam'. Delete content type 'yowza' which, presumably, takes field 'shazam' with it. Create another content type called 'magic' with a field called 'shazam'. Field UI lets you go on your merry way.

Very nice. That property description makes a lot more sense now, too.

Committed to HEAD. Thanks!

Still getting this error with the patch applied:

    * Notice: Undefined variable: node in eval() (line 3 of /var/www/vhosts/domain.be/httpdocs/modules/php/php.module(75) : eval()'d code).
    * Notice: Trying to get property of non-object in eval() (line 3 of /var/www/vhosts/domain.be/httpdocs/modules/php/php.module(75) : eval()'d code).

Btw: when applying the patch I got this message:

patching file field.crud.inc
Reversed (or previously applied) patch detected!  Assume -R? [n] 

I left it there and re-uploaded the field-folder to (root)/modules, with no result.

Your error message seems unrelated to the patch. Also, the patch has already been committed, so if you're using a recent version (later than December 15, 2010) you do not need to apply the patch.

I'm using Drupal 7.17 and still have the original problem as state in this issue. Is the issue only corrected in 7.x-dev version of Drupal? I would have thought that since the patch was committed by WebChick in #37 in December of 2010 that this would be in the stable release by now.

I don't know how to check to see if it is in the stable Drupal 7.17 release so that I might discover if this is an issue being caused by a some contrib module I'm using.

Any advice on how I might track this down would be greatly appreciated!

Thanks for your help in Advance!
Steve

P.S. I assume if this is still a problem in the stable releases of Drupal than the issue should also be reopened, but I'll leave that to someone else to decide.

I think I figured out what was going wrong for me as posted in #41. I was trying to remove a Term Reference but the Taxonomy still existed. Once I deleted the Taxonomy, the field also was removed.

This particular Taxonomy, if I remember correctly, was created by the WP Blog Module which I had disabled and uninstalled.

So most likely my issue was related to poor clean up by the contributed module.

Steve