I was facing the following problem today. I was informed that uninstalling commerce_opp (which I'm maintaining) is failing. I've investigated the problem and saw that error occurred, when Drupal tried to drop a field table, which already was deleted. The module is shipping with two commerce_payment types (bundles). A payment type plugin is implementing Entity API's BundlePluginInterface, allowing to define bundle fields. The first plugin class, Opp, is defining a field called "opp_checkout_id", which is necessary for all kinds of payments of this module, no matter if it's a credit card, direct banking, PayPal or whatever else type of payment. The second plugin implementation is needed for a specific domestic payment type in Portugal (SIBS Multibanco), where you need to store a transaction reference, for which the plugin also defines a bundle field. But of course these payments also need the checkout ID, and of course every Multibanco payment is also a specific case of an Open Payment Platform payment, meaning that is semantically correct to subclassing the Opp plugin. So long, so good. This subclassing, going alongside with re-using the opp_checkout_id field for both payment types, works great, without any problems... as long as you don't try to uninstall the module.

The BundlePluginInterface though does explicitly say in the interface comment:

Important:
   * Field names must be unique across all bundles.
   * It is recommended to prefix them with the bundle name (plugin ID).

So that means, that the current behaviour is somehow intended. But I was having a chat with @mglaman in Slack today, and he stated that this feels like a bug, and he doesn't know, why this was forbidden to do. The uninstall problem arises only because the uninstaller just deleted the field definition and immediately after the field storage as well. It wouldn't harm to check further usages of the field, before uninstalling the field storage too. We both agreed that this would make the process of uninstalling a bundle more bulletproof and won't harm in any way

CommentFileSizeAuthor
#2 3122980-2.patch1.48 KBagoradesign

Comments

agoradesign created an issue. See original summary.

agoradesign’s picture

agoradesign commented
StatusFileSize
new 3122980-2.patch1.48 KB

The attached patch fixes the problem for re-use of a field within the same module, as it uninstalls first only the field definitions of the given bundles, and finally the field storages.

What it doesn't do, is to do a global check, if the field is used somewhere else. I wanted to write a patch variant that asks the key value storage, such as the entity field manager does, for further usages, and only delete the field storage definition, if there's no more usage. Somehow my checks failed and the storage got deleted immediately, until I gave up... maybe the week's already too long :D

Also, tests are missing.. but it's a good starting point, and I can confirm that the attached patch works and allows to uninstall commerce_opp cleanly.

  • dawehner committed 5680540 on 8.x-1.x authored by agoradesign 
    Issue #3122980 by agoradesign: BundlePluginInstaller uninstalls field...
dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Fixed

This makes absolutely sense. Thank you for adding the helpful inline comment.

What it doesn't do, is to do a global check, if the field is used somewhere else. I wanted to write a patch variant that asks the key value storage, such as the entity field manager does, for further usages, and only delete the field storage definition, if there's no more usage. Somehow my checks failed and the storage got deleted immediately, until I gave up... maybe the week's already too long :D

Wouldn't this be better handled in an uninstall validation handler?

Commited and pushed to 8.x-1.x

agoradesign’s picture

agoradesign commented

thanks for committing :) and yes, you're absolutely right. uninstall validation handler sounds the right way for adding validation step

Status: Fixed » Closed (fixed)

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

paul121’s picture

paul121 commented

I opened a new issue to support re-using field storage definitions across bundles: #3203352: Allow field storage definitions to be reused across bundles

Because our field names are exposed via the API this is a pretty important feature for us. Leaving a note here because I couldn't find any other issues relating to this problem and I'm not 100% sure uninstalling was the only complication/issue with allowing field storage definitions to be reused. Appreciate any feedback or ideas!

The fix committed with this issue still seems relevant for the case that multiple bundles with the same field storage definition are uninstalled at the same time. I included a test for uninstalling a single bundle, but not multiple. Perhaps that could be added in the process.