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.
Comment | File | Size | Author |
---|---|---|---|
#29 | 366152_29.patch | 644 bytes | Mile23 |
#20 | 366152_20.patch | 644 bytes | Mile23 |
#16 | 366152_modules_uninstalled_D7.patch | 644 bytes | Mile23 |
#13 | 366152_field_modules_uninstalled.patch | 664 bytes | Mile23 |
#5 | field-delete-module-366152-4.patch | 1.59 KB | bjaspan |
Comments
Comment #1
yched CreditAttribution: yched commentedHm, I think this is a straight and stupid remnant of D6's content_notify('uninstall') :
For the record, content_module_delete() runs content_field_instance_delete() on all instances where *widget*_module = $module - which is, er, surprising...
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...
Comment #2
bjaspan CreditAttribution: bjaspan commentedI 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?
Comment #3
yched CreditAttribution: yched commentedFully 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.
Comment #4
bjaspan CreditAttribution: bjaspan commentedI've started on this.
Comment #5
bjaspan CreditAttribution: bjaspan commentedThis 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.
Comment #6
sun.core CreditAttribution: sun.core commentedIs this still an issue with the field bulk delete API being in place?
Comment #7
bjaspan CreditAttribution: bjaspan commentedIt 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."
Comment #8
catchFor 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
Comment #9
bjaspan CreditAttribution: bjaspan commentedFor that matter, if we go this route, we should just remove field_modules_uninstalled() completely, since it does not do anything else.
Comment #10
bjaspan CreditAttribution: bjaspan commentedDid not mean to change back to critical.
Comment #11
Mile23See: #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed
Comment #12
Mile23...
Comment #13
Mile23And why not? :-)
Comment #14
yched CreditAttribution: yched commented#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.
Comment #15
catchAlright 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.
Comment #16
Mile23Hey, cool, I have a commit to core by deleting code. :-)
Comment #17
yched CreditAttribution: yched commentedRerolled #1264728: Refresh field 'active' state in module_enable() / _disable() accordingly.
Comment #18
mgiffordThis still applies and still seems totally good to RTBC.
Comment #20
Mile23Re-roll. If you can call it that.
Comment #21
yched CreditAttribution: yched commentedThanks @Mile23
Comment #24
dcam CreditAttribution: dcam commentedComment #27
dcam CreditAttribution: dcam commentedComment #29
Mile23And again. :-)
Comment #30
Mile23Comment #32
Mile23OK, that's seriously weird, and someone smarter than me can figure it out.
Comment #34
mgiffordYa, 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
Comment #37
yched CreditAttribution: yched commentedBack to RTBC then
Comment #38
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted 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).