Hook field_modules_uninstalled() calls the unimplemented function field_module_delete(), causing some core test failures. I just committed a change so it simply does not call that function which is clearly not the long-term solution.

I assume field_module_delete() is intended to delete any Field API data tied to the module being uninstalled. That includes all fields whose type is defined by the module being uninstalled. What about instances that use widgets or formatters from the uninstalled module?

This is a highly destructive operation, even given the module uninstall confirm form. I'm thinking we should form_alter the confirm form and add some additional information about what is going to get wiped.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Hm, I think this is a straight and stupid remnant of D6's content_notify('uninstall') :

module_load_include('inc', 'content', 'includes/content.crud');
content_module_delete($module);

For the record, content_module_delete() runs content_field_instance_delete() on all instances where *widget*_module = $module - which is, er, surprising...

What about instances that use widgets or formatters from the uninstalled module?

The 'default_formatter' (and the 'display fields' implementation) lets us preserve instances that use a formatter defined by an uninstalled module (D6 hardcoded the use of a formatter named 'default')
Theoretically the 'default_widget' should (at some point) let us preserve instances that use a *widget* defined by an uninstalled (or disabled) module (and let us get rid of widget['active'] stuff)
Actually that's the whole point of this thing that's been bugging me since late dec, and that I still didn't get a chance to raise on g.d.o :
adapting a field/instance definition to the current execution context (enabled modules, current set of instance/widget settings) should IMO happen when we read the field from the db, not 'just in time' like the display code currently does for formatters. Well, that's another story. I *need* to settle and do that writeup...

bjaspan’s picture

I wonder if we've made enough progress since January to either resolve this issue now, decide we don't care, or at least decide it is not critical.

Based on #1, I think we only need to care when a module providing an in-use field type is deleted; instances, formatters, etc. do not matter because we have defaults, which always (?) come from the module providing the field type anyway.

When a module is being deleted, we can field_delete_field() all the fields whose types come from that module. However, once the field type module is gone, the bulk delete process will not be able to invoke its purge hooks. Bulk delete will still "work" because _field_invoke('delete') (which calls hook_field_delete, the field type hook which will be gone) will just do nothing, but obviously any outside-the-db data will never be cleaned up.

So, perhaps when a module is being uninstalled, we should hook the confirm form, use field_attach_query() to determine if any related fields have data (deleted or not), and warn that it will be orphaned. Otherwise, when a module is uninstalled, we field_delete_field() all fields of that module's types, and we're done with it.

Thoughts?

yched’s picture

Fully agreed.

Additional niftyness: we could hook the confirm form and add a batch API purge before the module is actually gone. That part is obviously post-freeze at best - uninstalling a field type module is pretty rare IMO.

bjaspan’s picture

Assigned: Unassigned » bjaspan
Status: Active » Needs work

I've started on this.

bjaspan’s picture

This whole issue is pretty pathological.

field_delete_module() itself is easy to implement; patch attached. However, as described in #2-3, there is basically no valid use case for the function. If we are trying to uninstall a module that defines a field type that is in use, the only safe thing to do is delete all the fields and purge their data first. I guess we could do a batch purge right then as yched suggests in #3, but it seems much simpler to say "you cannot uninstall date.module until the fields foo, bar, and baz are deleted and fully purged; go away until you've done that." Of course, we can't prevent people from just rm'ing the module, so maybe simply refusing to uninstall it isn't a good idea.

Having core form_alter another module's confirm form just does not seem like a good idea to me.

So, we are in the situation in which we know how to implement all the various options, it just isn't clear which one is right, if any.

sun.core’s picture

Status: Needs work » Postponed (maintainer needs more info)

Is this still an issue with the field bulk delete API being in place?

bjaspan’s picture

Status: Postponed (maintainer needs more info) » Active

It is absolutely still an issue with bulk delete API in place, it is just that no one has any decent suggestion for the correct/desirable behavior. See #5.

I am personally leaning towards doing nothing and just documenting, "don't uninstall field type modules with unpurged data."

catch’s picture

Priority: Critical » Normal

For a field module to be uninstallable, it'd have to implement hook_uninstall() or hook_schema() by itself anyway. Presumably if it does that, then it could decide what to do with it's own data on uninstall anyway, so I'm not sure there's anything for field.module to do here at all, and I think I agree pretty much with bjaspan that not doing anything is the best option.

Downgrading from critical, we still need to remove the @todo though if we go this way http://api.drupal.org/api/function/field_modules_uninstalled/7

bjaspan’s picture

Priority: Normal » Critical

For that matter, if we go this route, we should just remove field_modules_uninstalled() completely, since it does not do anything else.

bjaspan’s picture

Priority: Critical » Normal

Did not mean to change back to critical.

Mile23’s picture

Title: field_modules_uninstalled calls unimplemented field_module_delete » field_modules_uninstalled still exists, despite being an empty foreach loop.
Version: 7.x-dev » 8.x-dev
Mile23’s picture

Issue tags: +Needs backport to D7

...

Mile23’s picture

Status: Active » Needs review
FileSize
664 bytes

And why not? :-)

yched’s picture

Status: Needs review » Reviewed & tested by the community

#1264728: Refresh field 'active' state in module_enable() / _disable() (which catch almost RTBC's 3 months ago ;-) ), also removes that useless hook implementation in the middle of other refactoring.

But sure, that hook can go by itself.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Alright let's kill this one from here, means the other issue will need another re-roll, but since yched RTBCed this I assume he's OK with that.

Committed/pushed to 8.x, moving to 7.x for backport.

Mile23’s picture

Status: Patch (to be ported) » Needs review
FileSize
644 bytes

Hey, cool, I have a commit to core by deleting code. :-)

yched’s picture

mgifford’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

This still applies and still seems totally good to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 366152_modules_uninstalled_D7.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
644 bytes

Re-roll. If you can call it that.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @Mile23

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: 366152_20.patch, failed testing.

Status: Needs work » Needs review

mgifford queued 20: 366152_20.patch for re-testing.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: 366152_20.patch, failed testing.

Status: Needs work » Needs review

dcam queued 20: 366152_20.patch for re-testing.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: 366152_20.patch, failed testing.

Mile23’s picture

FileSize
644 bytes

And again. :-)

Mile23’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 29: 366152_29.patch, failed testing.

Mile23’s picture

OK, that's seriously weird, and someone smarter than me can figure it out.

Status: Needs work » Needs review

mgifford queued 29: 366152_29.patch for re-testing.

mgifford’s picture

Ya, you're deleting a function that doesn't seem to do anything.... Must be something with the bot...

EDIT: Same bot problem as https://www.drupal.org/node/935592#comment-8975331

Status: Needs review » Needs work

The last submitted patch, 29: 366152_29.patch, failed testing.

Status: Needs work » Needs review

mgifford queued 29: 366152_29.patch for re-testing.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC then

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.33 release notes

Committed to 7.x - thanks!

Yeah, it's highly unlikely any other code is calling this function, both because it's a hook implementation and because, um, it does nothing at all :) So I think it's safe to remove completely, though it will get a CHANGELOG.txt and release notes mention (just in case).

Status: Fixed » Closed (fixed)

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