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.

Files: 
CommentFileSizeAuthor
#5 node_example-fields_uninstall-1015846-5.patch1.5 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 1,945 pass(es). View
#1 node_example-fields_uninstall-1015846-1.patch1.28 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node_example-fields_uninstall-1015846-1.patch. View

Comments

linclark’s picture

Status: Active » Needs review
FileSize
1.28 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node_example-fields_uninstall-1015846-1.patch. View

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.

Dave Reid’s picture

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...

Dave Reid’s picture

Well crap. comment_uninstall does delete its field. Hrm
http://api.drupal.org/api/drupal/modules--comment--comment.install/funct...

Status: Needs review » Needs work

The last submitted patch, node_example-fields_uninstall-1015846-1.patch, failed testing.

linclark’s picture

Status: Needs work » Needs review
FileSize
1.5 KB
PASSED: [[SimpleTest]]: [MySQL] 1,945 pass(es). View

Whoops, used node_example as the root instead of examples.

New patch, with slightly modified comment.

linclark’s picture

Title: Should module uninstall delete fields? » Module should not delete fields on uninstall

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.

Mile23’s picture

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.

rfay’s picture

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.

rfay’s picture

Status: Needs review » Needs work

@linclark, do I understand from the change of title that your patch in #5 is not to be pursued? Making this "needs work" then.

linclark’s picture

Status: Needs work » Needs review

@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_instance
  • the call to field_delete_field

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.

Mile23’s picture

"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.

Dave Reid’s picture

Version: » 7.x-1.x-dev
keyiyek’s picture

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?

rfay’s picture

keyiyek’s picture

thanks

Mile23’s picture

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.

Mile23’s picture

I made a little sandbox project to shed some light on orphaned field data. :-)

http://drupal.org/sandbox/Mile23/1171894

rfay’s picture

Mile23’s picture

Status: Needs review » Postponed

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: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed

bojanz’s picture

Status: Postponed » Needs work

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.

Mile23’s picture

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.

Rory’s picture

@Mile23:

If other modules use fields that you define, they should declare your module as a dependency, so it won't be uninstalled.

I don't know where the intricacies of this issue are concerned, but I completely agree with this.

lsolesen’s picture

Trying to fix this for #976022: field_data_media_gallery_media not removed on uninstall. How should this be handled in a module?

Mile23’s picture

How about if some of the esteemed names who are following this issue chime in, please. :-)

Currently, Drupal won't let you uninstall modules which are responsible for existing field content. Perhaps the best practice is to not delete anything, and require the user to do all the deleting, via the Drupal dependency maze. As tedious as that can be. :-)

Same for Node API. Our module would depend on node, which can deal with orphaned node types from disabled and uninstalled modules, and force the dependency by disallowing disabling, if need be.

So the best practice would be:

  1. Separate modules for each API (node, field) where possible, ideally with a 'core' module that supports both.
  2. Leave content for the user to deal with, since they'll be able to do it in Drupal UI thanks to dependencies.
  3. File core bug reports if Drupal can't deal with the dependencies you've created.

Any disagreement?

Mile23’s picture

Solving two bugs with one patch: #1268112: Disabled Node Example module still permits construction of Node Example content

I'll mark this issue as fixed when the patch gets applied over there.

Mile23’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Strutsagget’s picture

Well think this is the wrong approach as it is impossible to use the code unless you are already an expert now(and guess you don't need it then).

This is the scenario i think most beginners approach.

1 Activate module and see what is does.
2 Make own changes. See what happens.
3 When you got it all figured out how it works, start writing your own module or rename the module to your own module name.

As the second part is impossible to do as the fields and hooks are permanent and only invoked first time the module is installed i guess its not only me that will put down a lot of hours trying to figure out why their code don't work. And when we finally find out there is no other way then to manually remove every post/tables in the database to start testing how to build your module.

And maybe its just because I'm stupid but my codes long from perfect on first try so i have to manually remove all database post (don't even have any nodes or fields in the database), rewrite the code, try again and see if it works, remove everything again, try again and so on and so on.

So at least this module actually kind of makes so it takes me a lot longer to build my module and leave me to find other resources (and now i cant even remember the perfect wording i used on google to find the one page that actually had it as all other resources points here). So it do not feel like it is best practice to leave all fields and bundles intact after uninstalling the module.

But i really do appreciate all the help you give us! Your helps awesome!

AlessMascherpa’s picture

Sharing fileds is not cool. You may think of some use case where is interesting. But generally is a bad practice. It raises problems when creating Features. It raises problems when installing-uninstalling-reinstalling modules. It incorporates complexity to the system (meaning Drupal core). More important, you should always be able to remove your module completely (entities, types, fileds, data...). If you want to keep your data, backup or migrate it. Cloning fileds would be a much more interesting strategy.

Examples is an awesome work BTW. Thanks!

Cheers
Alessandro Mascherpa

joseph.muennich’s picture

I am newly exploring this issue and have a clarifying question:

Is there a reason NOT to have field_delete_field($field) fail/abort deleting the field if there are any instances of that field (for any bundle)?

Note: Following this "garbage collection" approach would necessitate a best practice of always removing your field instances before attempting a field deletion. Alternately, an additional parameter could allow an optional bundle array to trigger instance and field deletion in a single pass ( field_delete_field($field, $bundles='') - again aborting deletion if any instances of the field remain).

Mile23’s picture

@Strutsagget: The Examples project exists so you can have an example of how a given API should/could/might be implemented. You can change things as much as you want... You could have your own node module delete all the content when it gets uninstalled, etc.

There are a few different tools to help developers deal with these APIs. If you find yourself needing to reinstall a module all the time to test its installation behavior, you might check out drush. In fact, just get drush and learn it. :-) For instance: drush pm-uninstall [module] at the command line will uninstall the module, and then drush en [module] will set it back up.

@AlessMascherpa: Fields have two main architectural areas: The field is defined by a module, and the field is instantiated by the user (or the module). As far as Drupal is concerned, the module that defines the field is now a dependency of all content that uses instances of that field, and it won't let you disable or uninstall that module. This is why it's the best practice to separate fields out to their own module if possible.

@joseph.muennich: Look at the source for field_delete_field(): http://api.drupal.org/api/drupal/modules%21field%21field.crud.inc/functi...

It calls field_delete_instance() on all the field's instances, which marks them for deletion. Then you can call field_purge_batch().

Rundown here: http://api.drupal.org/api/drupal/modules%21field%21field.crud.inc/group/...

HTH.

AlessMascherpa’s picture

Hi @Mile23,

Agree that a field declaration has to live in its own module. Like the entityreference filed does. No disagreement about that. Sharing fileds instances between entities bundles still feels like a bad practice (comment #29).

Using an entityreference field instance created by a module will crash (or be inconsistent) on reinstall if you don't delete the field instance or check if it exists before execute its creation, to take in to account this case. That's a way to increase system logic complexity without good reason. It does not make any sense BTW.

A better practice will be IMO to clone filed instancies instead of sharing them: http://drupal.org/project/field_tools This way you don't have to rely in the module developer following good practices. The system takes care about it for you. I understand that this is a system wide behavior, and not Examples module.
EDIT: I correct myself field_tools module referenced in prev. paragraf doesn't really clone a field instance. It eases the sharing of a filed instance in a bulky way :(

Cheers
Alessandro

Strutsagget’s picture

Hi and thanx for your answere!

My problem aint with uninstallng/installing the module in general. That infect would be great id it worked and I do use drush on all our projects. The problem is that uninstalling and reinstalling does nothing as the creation of the field is just done the first time as the field is never uninstalled when you uninstall the module.