Posted by linclark on January 5, 2011 at 3:18pm
10 followers
| Project: | Examples for Developers |
| Version: | 7.x-1.x-dev |
| Component: | Node Example |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Issue Summary
In node_example_uninstall, the module removes all the fields it defined for this content type and then removes all instances.
// Loop over each of the fields defined by this module and delete
// all instances of the field, their data, and the field itself.
// http://api.drupal.org/api/function/field_delete_field/7
foreach (array_keys(_node_example_installed_fields()) as $field) {
field_delete_field($field);
}
// Loop over any remaining field instances attached to the node_example
// content type (such as the body field) and delete them individually.
// http://api.drupal.org/api/function/field_delete_field/7
$instances = field_info_instances('node', 'node_example');
foreach ($instances as $instance_name => $instance) {
field_delete_instance($instance);
}Since fields defined in any module can be added to any node type, I'm not sure whether the example should demonstrate deleting the field itself (the first chunk of the code above), but rather should just delete the instances associated with the node type that it defines.
Basically, a user could have installed a module and then added one of the fields the module defines to another bundle using Field UI. That user would then get a nasty shock when the field disappears on that other node type.
Comments
#1
Also, field_delete_instance includes a call to field_delete_field if there are no bundles using the field, which I think is the appropriate action.
// Delete the field itself if we just deleted its last instance.if ($field_cleanup && count($field['bundles']) == 0) {
field_delete_field($field['field_name']);
}
I'm attaching a patch that removes the chunk that deletes the fields and also fixes a minor issue with a comment.
#2
I would follow the example set by core. Don't do anything on uninstall. It looks like we have a todo that field.module will handle it for us automatically, but until then, we still shouldn't do it.
http://api.drupal.org/api/drupal/modules--field--field.module/function/f...
#3
Well crap. comment_uninstall does delete its field. Hrm
http://api.drupal.org/api/drupal/modules--comment--comment.install/funct...
#4
The last submitted patch, node_example-fields_uninstall-1015846-1.patch, failed testing.
#5
Whoops, used node_example as the root instead of examples.
New patch, with slightly modified comment.
#6
I'm changing the title of this, since it is clear that the module should not delete fields when it is uninstalled.
The question now is what it should do.
#7
Disabling the module is just turning it off. Uninstalling means wiping it from existence. If I installed a module and then decided to uninstall it, I'd rather it didn't leave its fields clogging up my database. I think that's generally understood as the best practice. If other modules use fields that you define, they should declare your module as a dependency, so it won't be uninstalled.
#8
The problem here is that this has never been resolved authoritatively by either CCK or core, even in D7.
So the very least we can do is pick a solution and give good comments for it, explaining the alternative.
#9
@linclark, do I understand from the change of title that your patch in #5 is not to be pursued? Making this "needs work" then.
#10
@Mile23, the code I included in comment #1 deletes the field if it hasn't been used elsewhere. So if the user hasn't reused the field, then it will be deleted by the call to field_delete_instance. I don't think that deleting a field that the user may have attached to other bundles is an appropriate action to take. The logic that already in field_delete_instance makes sense, I'm guessing that they had this type of use case in mind when it was coded.
There are two issues here.
The call to field_delete_field is the big problem right now. That is the one that deletes the field regardless of whether or not the user has attached it to another bundle using Field UI.
Dave Reid is saying that both calls should be removed because there is a todo to have all of this handled by field.module itself. He is saying that we should remove both calls right now.
I think that my patch includes the appropriate fix for the state of Drupal right now. If the @todo in field.module is completed and it becomes unnecessary to call field_delete_instance, then another patch can remove the call to field_delete_instance. As far as I'm concerned, the manual call to field_delete_field should be removed asap, because that can have significant repercussions for a user who has attached a module-defined field to a bundle.
#11
"The call to field_delete_field is the big problem right now. That is the one that deletes the field regardless of whether or not the user has attached it to another bundle using Field UI."
Yes, exactly. :-)
If you uninstall your module, you see this warning:
"The following modules will be completely uninstalled from your site, and all data from these modules will be lost!"
This leads me to believe that if I'm implementing hook_uninstall(), I can't let field_delete_instance() make the decision for me about fields I implemented, for the reason you just gave. Without cleaning up all the instances, there's a bunch of zombie data in the database with no module to look after it.
This problem exists for field_example, too. You can uninstall it but the data will live forever in your db.
An ideal core solution would be to have Field API flip a switch on module dependencies, disallowing the uninstall (or deactivation, for that matter) of any module that has instances in use. A quick test tells me this hasn't been implemented. And a best practice would be to disallow as many fields from being attached through Field UI as possible, but I'm coming up short finding out how to do that.
#12
#13
I have been fighting with the uninstall process of a module I'm writing.
I can't eliminate the instances from the database
it seems field_info_instances() doesn't return nothing after I disable the module.
I tried in the "execute php code" page from the devel module, I give
echo field_info_instances('entity type', 'entity');
getting "Array" in return, until I disable the module then nothing.
Is someone else having the same problem?
#14
You may be interested in #943772: field_delete_field() and others fail for inactive fields.
#15
thanks
#16
With the devel PHP page, try print_r() instead of echo, like this:
print_r(field_info_instances('node', 'article'));
and/or:
print_r(field_info_instance('node', 'body', 'article'));
Echo just gives you 'Array' which isn't very helpful.
#17
I made a little sandbox project to shed some light on orphaned field data. :-)
http://drupal.org/sandbox/Mile23/1171894
#18
Wondering if #943772: field_delete_field() and others fail for inactive fields has relevance for this issue.
#19
webchick sez it's fixed: http://drupal.org/node/943772#comment-4924298
But really that's just to start a new discussion in a new issue queue. I wonder which one.... Helpful list and meta-issue of what to do about disabled modules: #1199946: Ensure that disabled modules maintain data integrity
#20
See #1204820: Entity providing modules must call field_attach_delete_bundle() in hook_uninstall().
In hook_uninstall(), you call field_attach_delete_bundle() for the bundle the module provided, and all the fields attached will get dropped.
This is the new consensus in core and contrib.
#21
That issue is in EntityAPI issue queue, which isn't core, but yah. :-) The core issue is: #1115510: Entity providing modules must call field_attach_delete_bundle() in hook_uninstall().
It seems obvious to me that Drupal should do this instead of requiring it in hook_uninstall(), especially since all implementations will be less than trivial, exactly the same, and there's a high risk if someone does it wrong.
#22
@Mile23:
I don't know where the intricacies of this issue are concerned, but I completely agree with this.