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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

Status: Active » Needs review
FileSize
9.85 KB

So, the attached patch does the following:

rfay’s picture

For background: #858722: Cannot reinstall Commerce modules after uninstall due to field deletion failure, which has a pretty significant explanation of the options.

amateescu’s picture

I think we also need a hook_requirements for the update, but I'm leaving that for a re-roll after some reviews.

bojanz’s picture

We 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:

  // Clear bundle display settings.
  $settings = variable_get('field_bundle_settings', array());
  if (isset($settings[$entity_type][$bundle])) {
    unset($settings[$entity_type][$bundle]);
    variable_set('field_bundle_settings', $settings);
  }

  // Let other modules act on deleting the bundle.
  module_invoke_all('field_attach_delete_bundle', $entity_type, $bundle, $instances);

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.

bojanz’s picture

FileSize
5.01 KB

Here'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.)

bojanz’s picture

FileSize
7.32 KB

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

bojanz’s picture

Title: Clean-up uninstall mess » Make uninstallation possible again.

Updated the issue summary. Updating issue title.

bojanz’s picture

Title: Make uninstallation possible again. » Make uninstallation possible again

I have a strange fondness for dots.

bojanz’s picture

@@ -178,6 +205,11 @@ function commerce_delete_fields($type) {
  * work on disabled fields. As a workaround, this function first activates the
  * fields of the specified type and then deletes them.
  *
+ * Deleting reactivated fields doesn't call the hook_field_delete()
+ * implementations of their parent modules (since they are already disabled),
+ * making this approach infeasible for core, but perfect for Commerce,
+ * since it doesn't implement that hook for any of its field types.
+ *
  * @param $field_name
  *   The name of the field to enable and delete.
  */

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

bojanz’s picture

FileSize
6.76 KB

Per #9.

rszrama’s picture

Small 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?

bojanz’s picture

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

rszrama’s picture

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

rszrama’s picture

Priority: Major » Normal
Status: Needs review » Fixed

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

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

Anonymous’s picture

Issue summary: View changes

Update issue summary.