Problem/Motivation
A bit after Commerce got its uninstall code (with the "reactivate field" approach), Drupal core got its own fix, which consists of disallowing the disabling of modules that provide field types, if fields / instances of those types exist. Those dependencies are calculated on the fly, and if you have a "product_reference" field on module X, it won't allow you to disable the product_reference module until module X gets disabled and the offending field removed.
The problem begins when you know that X = Line item, because the Product Reference module already depends on Line items, and then Drupal creates a circular dependency, making it impossible for Commerce to be disabled / uninstalled through the UI.
People on the queues figured out a way around that: use "drush pm-disable commerce", which disables all modules without making a fuss.
However, you still can't uninstall correctly, since various Commerce submodules call functions from commerce.module (commerce_delete_instances / commerce_delete_fields) without including the commerce.module file, assuming it is already loaded. However, if you've just disabled everything through Drush, that is not true.
Proposed resolution
The current patch is in #6. It does four things:
1) Documents the situation described in the summary.
2) Updates the commerce.module weight to be bigger than the weight of field.module, then using a hook_system_info_alter() to unrequire the field type providing modules that Commerce ships with (customer, line_item, price, product_reference). This reverses the damage done by core, allowing all modules to be disabled through the UI.
3) Adds the module_load_include() lines needed to always load commerce.module, making uninstall possible when all modules are disabled.
4) Adds code to commerce_delete_instances() that removes field display settings for the entity type / bundle being uninstalled, so that cruft doesn't remain.
This code was copied from field_attach_delete_bundle(), the function that Core recommends for deleting fields and field instances on module uninstall, unusable for Commerce because it doesn't contain the "reactivate" hack invented for Commerce.
Problems with the current patch
Deleted data doesn't get purged by cron after uninstall, you get stuck with rows that have deleted = 1 in field_config and field_config_instance, as well as field_deleted_* tables. This is because when cron runs, the fields are inactive again, entity types are gone, and the purge can't happen.
It's just dead data though, Commerce can be installed again without a problem.
There could be several fixes to this:
1) Provide a script that deletes the rows with deleted = 1 and drops the field_deleted_* tables.
2) Do it on commerce_uninstall(). Panic if the tables are big.
3) Bring back the "required" state for customer_reference and line_item_reference fields, so that at least that data gets purged.
I tried adding the manual call to field_purge that #1 had, but it doesn't really work when "required" is removed from all modules.
Comment | File | Size | Author |
---|---|---|---|
#10 | 1403972-fix-uninstall.patch | 6.76 KB | bojanz |
#6 | 1403972-fix-uninstall.patch | 7.32 KB | bojanz |
#5 | 1403972-fix-uninstall.patch | 5.01 KB | bojanz |
#1 | 1403972-fix_commerce_uninstall.patch | 9.85 KB | amateescu |
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedSo, the attached patch does the following:
field_attach_delete_bundle()
. See #1115510: Entity providing modules must call field_attach_delete_bundle() in hook_uninstall() for Drupal core and #1204820: Entity providing modules must call field_attach_delete_bundle() in hook_uninstall() for Entity APIcommerce_delete_instances()
andcommerce_delete_instance()
in favor offield_attach_delete_bundle()
hook_system_info_alter()
to remove a dependency added by Field.module in #943772: field_delete_field() and others fail for inactive fieldsComment #2
rfayFor background: #858722: Cannot reinstall Commerce modules after uninstall due to field deletion failure, which has a pretty significant explanation of the options.
Comment #3
amateescu CreditAttribution: amateescu commentedI think we also need a hook_requirements for the update, but I'm leaving that for a re-roll after some reviews.
Comment #4
bojanz CreditAttribution: bojanz commentedWe are very close. And this is awesome.
My proposal:
1) Forget about field_attach_delete_bundle(), undeprecate commerce_delete_instances() and use that in it's place, the same way.
Why: when all modules are disabled at once, it doesn't work (field_info_field() fails for fields that are not active), and all the work is actually done by commerce_delete_field(). So it's useless. This part is not handled by commerce_delete_instances(), but we can add it there:
2) Apply the same alter trick to customer and line_item modules. That way if the user didn't add any custom fields (or did, but removed them) he can disable all Commerce modules at once, and proceed to uninstall. This basically undos the core approach that doesn't work for us.
I can reroll tomorrow if we agree on the approach.
Comment #5
bojanz CreditAttribution: bojanz commentedHere's a reroll, not yet fully tested.
It basically does the alter trick from #1, undoing the core approach, adds docs, and a bit of cleanup code copied from field_attach_delete_bundle() to commerce_delete_instances (which feels slightly icky, but it's the only place where I can put it.)
Comment #6
bojanz CreditAttribution: bojanz commentedForgot to include the "module_load_include" fixes from #1.
So, this works. I disable the fields, then do the uninstall.
The only problem is that the deleted data doesn't get purged, you get stuck with rows that have deleted = 1 in field_config and field_config_instance, as well as field_deleted_* tables. This is because when cron runs, the fields are inactive again, entity types are gone, and the purge can't happen.
It's just dead data though, Commerce can be installed again without a problem.
There could be several fixes to this:
1) Provide a script that deletes the rows with deleted = 1 and drops the field_deleted_* tables.
2) Do it on commerce_uninstall(). Panic if the tables are big.
3) Bring back the "required" state for customer_reference and line_item_reference fields, so that at least that data gets purged.
I tried adding the manual call to field_purge that #1 had, but it doesn't really work when "required" is removed from all modules.
Comment #7
bojanz CreditAttribution: bojanz commentedUpdated the issue summary. Updating issue title.
Comment #8
bojanz CreditAttribution: bojanz commentedI have a strange fondness for dots.
Comment #9
bojanz CreditAttribution: bojanz commentedThis can be dropped. When I wrote the comment I assumed that delete would succeed anyway, which is not the case since on cron the field statuses are synced, the fields deactivated. Even if active, it doesn't matter, since the entity type no longer exists, so purging still fails.
Comment #10
bojanz CreditAttribution: bojanz commentedPer #9.
Comment #11
rszrama CreditAttribution: rszrama commentedSmall quibble, but since I vetoed it before, I want to see if there's a compelling new reason to use module_load_include() in these uninstallation files. My understanding was that the uninstallation function should not be including code from disabled modules, and it wouldn't be good to leave the door open for the future execution of unexpected code if we add some hooks to commerce.module that get executed when a module is deleted. Are you adding it for the UX benefit weighed against the potential DX head-scratcher?
Comment #12
bojanz CreditAttribution: bojanz commentedhook_uninstall() should never assume the existence of any function outside of its own .install file. For functions outside the .install file, the appropriate file is loaded through module_load_include(). That is official Drupal policy, so the current code is wrong even if it works.
If you add some hooks to commerce.module that get executed when a module is deleted, then that is their business. I don't see how it relates to hook_uninstall at all.
Keep in mind that until now commerce.module was always loaded and present for other module's hook_uninstalls, so everything it had was always executed. We are only now changing that.
Furthermore, all hooks that use module_implements() (which should be almost all in core) won't call hooks of disabled modules even if they are loaded.
There is no DX head-scratcher.
And even if we disregard all that, there is no other way for this to work. We need it to get uninstall working again.
Comment #13
rszrama CreditAttribution: rszrama commentedOooh, this is the money quote. "Furthermore, all hooks that use module_implements() (which should be almost all in core) won't call hooks of disabled modules even if they are loaded." My fears were unfounded. Carry on.
Comment #14
rszrama CreditAttribution: rszrama commentedAlrighty, committing this bad boy.
I'm actually removing the .info specification of Drupal core 7.4+. There have been security releases since then and several other point releases; sites should really be up to date. Additionally, since we're not using the new API function any more, it shouldn't even matter. This is such a fringe feature that it doesn't bear further testing. I'll just make some notes in the uninstallation documentation about this and the persistence of "deleted" tables and rows in the database / field config tables.
Comment #15.0
(not verified) CreditAttribution: commentedUpdate issue summary.