Hi,

I wanted to reinstall commerce tax module. It was impossible because uninstall fails.
It can not be done because the tax_number field in customer profile field can not be deleted.
The tax_number field in the customer profile was created, when installing the commerce tax module.

Issue fork commerce-3118588

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

dercheffe created an issue. See original summary.

kecinzer’s picture

Same issue here.

nanak’s picture

You can't uninstall a module that defines a field type while having at least an instance of that field type on your site. The field must be deleted prior to uninstalling the module. Try to uninstall the "text" module, you'll have to same error.

The hiccup in here is that commerce_tax_field_storage_config_access purposely prevent access to the field config delete form:

/**
 * Implements hook_ENTITY_TYPE_access().
 *
 * Forbids the profile "tax_number" field from being deletable.
 * This is an alternative to locking the field which still leaves
 * the field editable.
 */
function commerce_tax_field_storage_config_access(FieldStorageConfigInterface $field_storage, $operation) {
  if ($field_storage->id() == 'profile.tax_number' && $operation == 'delete') {
    return AccessResult::forbidden();
  }
  return AccessResult::neutral();
}

The temporary solution is to comment this while you delete the field in order to uninstall, but it would be good to make this more straightforward.
Maybe by adding an "uninstall" tab on the field config that would delete the field config, and redirect to the module uninstall form?

martijn de wit’s picture

Category: Bug report » Support request
ben.hamelin’s picture

Category: Support request » Bug report
Status: Active » Needs review
StatusFileSize
new951 bytes

Attached patch checks for the profile module, and then gets a count of all profiles. The assumption here is that if there are no profiles then it is safe to allow the deletion of this field. This was quick and dirty in the hopes that it will help someone. Happy for refinement/feedback as needed.
Just a note that I do feel this is a bug - if we aren't allowing site maintainers to uninstall a module that's not very useful or helpful software.

Additionally a check on orders might make sense as follows, but this isn't included in the patch:

$orders = \Drupal::entityTypeManager()
      ->getStorage('commerce_order')
      ->loadMultiple();

    \Drupal::messenger()->addMessage(print_r(count($orders), TRUE), 'info');
jhnnsbstnbch’s picture

#5 works brilliantly. Thank you so much ben.hamelin.

SushiPrice’s picture

Could someone please give me instructions on how to use this patch. Please provide specific step by step instructions assuming that I have little to no knowledge. - Thanks so much

ben.hamelin’s picture

This is a start: https://www.drupal.org/patch/apply

Also you can manually make this update by editing the file: /modules/tax/commerce_tax.module b/modules/tax/commerce_tax.module
Find this line in the file "function commerce_tax_field_storage_config_access"
Add add the following on the next line:

// If the profile module exists, but there are no profiles, allow delete.
  // @see https://www.drupal.org/project/commerce/issues/3118588
  if (\Drupal::service('module_handler')->moduleExists('profile')) {
    $list = \Drupal::entityTypeManager()
      ->getStorage('profile')
      ->loadMultiple();

    if (count($list) === 0) {
      return AccessResult::allowed();
    }
  }
spidersilk’s picture

I've tried adding the code in #8, editing the existing code to make it return "AccessResult::allowed()" no matter what, and even removing that function entirely, and nothing helps — it will not let me uninstall the Commerce Tax module, no matter what I do.

And of course that blocks all the rest of Commerce from being uninstalled...

ben.hamelin’s picture

@spidersilk
It's possible there is something else blocking this. Make sure you have deleted all orders, profiles, products, tax types, stores etc. Try to remove everything you can from a commerce content/configuration perspective. For example, here is the admin URL for tax types: /admin/commerce/config/tax-types

Hope this helps!

SushiPrice’s picture

So what exactly am I supposed to do after editing the file as per your instructions? I have copied and pasted the code as per your instructions and then saved it. What do I do to trigger it to run that file again and fix the problem?

ben.hamelin’s picture

@SushiPrice
Make sure you have deleted all commerce content (products, orders, stores). Then make sure you have deleted all profiles as well: /admin/people/profiles

The code we added will only work if all the profiles are deleted.
Once they are gone, you should then be able to delete the "Tax Number" field for your profile type, found here:
/admin/config/people/profile-types/manage/customer/fields

Once the "Tax Number" field is deleted, you can uninstall the Commerce Tax module.

londova’s picture

@ben.hamelin

Why should we delete all the orders? Is that the only way possible?
Let's imagine the following case - a company being previously registered for VAT purpose, after a period of time decided to deregister because of the threshold (sales decreased). In this case the Tax module is not required anymore.
Should we keep it because there references from orders to Tax module?

bojanz’s picture

@Londova
The only case in which you should be uninstalling a Commerce submodule is because you're destroying Commerce and all of its data. A company no longer collecting tax is not a reason to touch the tax module. Your orders still have historical tax data attached. And even if they don't, the cost of an installed module is negligible.

londova’s picture

@bojanz
Glad to hear you are back. My comment is out of this issue - is just and idea.

I had a similar case, when tried to delete a store. I suspect this happen because orders keep REFERENCES to entities like Tax, Stores, Product, etc.

It may be a good idea to add an alternative settings option, when the Orders will COPY data (from Stories, Taxes, products, product variations) without keeping references to these entities. And developers may select which option is more appropriate for them.

bisonbleu’s picture

Status: Needs review » Reviewed & tested by the community

Just ran into this issue. Patch in #5 applies cleanly to Commerce 2.21. After that I was able to delete the tax_number field from the customer profile and uninstall the remaining Commerce modules.

Many thanks @ben.hamelin

spidersilk’s picture

I tried to apply the patch, but nothing happened - no changes were made to the file at all. I tried adding "-v" to the git apply command to see if I could find out why, but the only output it gave me was "Skipped patch 'modules/tax/commerce_tax.module'." No indication of why it skipped it.

I tried applying the code manually again, and cleared caches afterward in case that was needed for it take effect, but it's still not allowing me to delete the tax number field or profile type, or uninstall Commerce Tax.

I don't think anything else is blocking it. There are no tax types defined, no profiles, and no orders, and the store and products had already previously been deleted.

Next to Commerce Tax on the Uninstall page it specifically says:

The following reason prevents Commerce Tax from being uninstalled:
The Tax number field type is used in the following field: profile.tax_number

So that's the only reason it's giving, and I'm really stuck. I even tried commenting out the latter part of that function the path alters, the part that gives a "forbidden" result, and that didn't help either. No matter what I do, it still says the profile.tax_number field is blocking it from being uninstalled.

And this is on a site that's going to launch very soon, so I'd really like to get rid of any unused modules before it does, particularly Commerce given how big it is!

ben.hamelin’s picture

It sounds to me like the code you are editing isn't being executed. Is it possible that you have another modules folder?
I would expect to find this in /modules/contrib/commerce/modules/tax/commerce_tax.module

Can you search your project folder for "function commerce_tax_field_storage_config_access" and see if there is another instance of it somewhere?

Based on your description above it sounds like the code you are patching/editing isn't being run.

spidersilk’s picture

@ben.hamelin — to my surprise, that did turn out to be the case! There was a copy of Commerce in /modules AND one in /modules/contrib, probably because I had initially tried to install various modules via FTP, Drupal-7-style, before realizing that wasn't workable for all of them and finding a way to do it via Composer. I had not realized Composer put them in a different location!

Unfortunately — that didn't seem to solve the problem! When I deleted the redundant copy of Commerce (which was the one where I'd been making changes to that file) and applied the changes to the commerce_tax.module file in the contrib folder... It still didn't work.

And this time I know it is executing the code, because when I at one point accidentally left out a closing bracket, it took the whole site down! So it must be reading the file... But despite that, making the changes to the file does not make Commerce Tax deletable, or get rid of the message that "The Tax number field type is used in the following field: profile.tax_number".

I'm not really sure what else to try at this point — thinking of possibly seeing if I can somehow delete either that field or the customer profile type from the database using phpMyAdmin or something, but that may be a terrible idea...

ben.hamelin’s picture

@spidersilk - see comment #12 and make sure you've manually deleted the Tax field from the profile tpye, which has to happen first. Then you should be able to uninstall Commerce Tax.

spidersilk’s picture

@ben.hamelin - Ah, OK! I had missed a step... It worked this time, thank you!

ben.hamelin’s picture

Alright!! You're welcome, glad this finally worked for you.

wickwood’s picture

Code in #8 worked for me. It was easier for what I was doing to just edit directly than to apply the patch but I see the code is the same as patch in #5.

I was running into other issues with configuring Commerce Modules for a new site and wanted to start over, which is why I was trying to cleanly uninstall all of them.

nwom’s picture

I applied #5 successfully, but can't seem to delete the tax field or the customer profile, since there is no delete option. Am I missing something? Thanks in advance!

ben.hamelin’s picture

@NWOM - make sure you've deleted all orders and customer profiles first. You want to remove all content associated with customers. Then you should have a "Delete" option for the tax field. You can also try directly loading this URL:
"/admin/config/people/profile-types/manage/customer/fields/profile.customer.tax_number/delete"

But you should be seeing the delete option if the patch applied, and all customer content is removed.

nwom’s picture

@ben.hamelin: Ah I was trying to delete the profile type the entire time, rather than the profiles. Now it works. Thank you so much for both the help and the patch!

igonzalez’s picture

#5 works for me. Thank you so much

jsacksick’s picture

Status: Reviewed & tested by the community » Needs work

The patch in #5 looks incorrect to me as you're not even testing the field name and the operation, which means you're essentially granting access to all operations for all fields.

jsacksick’s picture

Why not putting the code in the existing condition (since you're targeting the delete operation)? Also... You're loading all profiles of the site... Which means you can potentially load millions of profile entities, which is bad... :p.

jsacksick’s picture

And.. Note that you're not even targeting "customer" profiles only (where the tax number field is attached)... Which means, if you have profiles of a different type, the check will fail.

ben.hamelin’s picture

@jsacksick Good catch(es) and thanks for the feedback. I will spin up a local env and work to replace the loadMultiple() with an entityStorageQuery. The MR approach I started introduces a guard clause for this field, let me know if that makes sense to you, if you think we should add the delete op to that clause as well, or if you'd prefer to just keep everything in that original conditional.

I'll also do some testing on the multiple profile types.

jsacksick’s picture

The MR still loads all profiles, additionally, the code is not targeting only the "delete" operation but all.

So perhaps we should return early for non delete operations?

From

  if ($field_storage->id() !== 'profile.tax_number') {

to

  if ($field_storage->id() !== 'profile.tax_number' || $operation !== 'delete') {

Also... What happens if the field is deleted, but not in a context of a module uninstall? This would leave the installation in a broken state no?

bojanz’s picture

That's why I added code to forbid the deletion in the first place. Back in the old days (e.g. 1.x) you could freely delete any Commerce field, and people broke their sites all the time. We never really optimized for the uninstall use case, cause if you need to uninstall a module set the size of Commerce, your best bet is usually to reinstall the site.

ben.hamelin’s picture

@jsacksick Updated the conditional per your recommendation. I haven't had a chance to update the entity query yet but it's on my list.

RE: the field deletion, I think that's a valid point to consider. It could be manually added back, correct?

@bojanz - I first ran into this when attempting to install Commerce on an existing D8 site. The site had been online for about a year using a 3rd party ecomm solution. For a number of reasons we backed off from using Drupal Commerce after some initial testing and integration work, but due to this issue I was unable to completely remove Commerce until the patch. A complete reinstall of the site was definitely not an option.

Thanks to you both for the feedback and suggestions. If this seems like a dead end issue let me know and I'm happy to leave this as is, and keep the patch here available for those who may need it. Seeing comments here where it's helped some folks makes me feel like it may still have some value to introduce formally into the module.

bobburns’s picture

The best way to remove some data is to "empty" the field tables with phpmyadmin. Do nt delete the tables - just empty them - the uninstall will delete the tables.. I got several WSOD where once it could not find the default order - and then for this profile deletion there were hundreds of profiles, so I went to the profile tables in phpmyadmin, checked them all, and then selected "empty".

Only because I have phpmyadmin to copy the database before the required deletions do I not mind this "feature" - because there are plenty of times when a company might have a multi domain database with hundreds of users they do not have the luxury of loosing all that data required just to delete Commerce.

I can simply re-import the data - but this is NOT a good feature of Commerce

z3cka made their first commit to this issue’s fork.

z3cka’s picture

I ran into this issue doing some maintenance on some old sites. I rebased !45 to the latest on `8.x-2.x` and it works like a charm. Please have a look at MR!115. This would be a nice to have for folks trying Commerce on an existing site and decide to uninstall.

kopeboy’s picture

Status: Needs work » Needs review

#38 worked for me, thanks!

alabandit’s picture

#12 still required to uninstall with drush or web interface.

arunkumark’s picture

Version: 8.x-2.17 » 8.x-2.38
arunkumark’s picture

Status: Needs review » Needs work

I verified the Merge Request #!115 not solving the issue of uninstallation. The patch applies successfully. It was verified on the Commerce core 8.x-2.38.

ben.hamelin’s picture

@arunkumark make sure you have followed manual steps outlined in #12. There is manual data removal likely needed before you can uninstall.

arunkumark’s picture

@ben.hamelin
The deleted data, But the uninstallation is not smooth. After removing the tax field.storage.yml removal I can uninstall. The field associated with the customer Profile.

kopeboy’s picture

Title: Unable to reinstall/uninstall commerce tax module » Unable to delete profile.tax_number & uninstall commerce_tax module
Version: 8.x-2.38 » 8.x-2.39
Status: Needs work » Reviewed & tested by the community

MR!115 from #38 still applies cleanly to commerce 2.39.0

Without it you don't have the option to delete the tax_number field from the default Customer profile type (the delete path is access denied), even on a fresh install of Drupal & Commerce (no content or orders added), hence trying to uninstall commerce_tax module would fail with commerce_tax: The <em class="placeholder">Tax number</em> field type is used in the following field: profile.tax_number.

After applying the patch you can instead visit /admin/config/people/profile-types/manage/customer/fields/profile.customer.tax_number/delete and uninstall commerce_tax right after.

Please include this old bug fix before it needs to be re-rolled!

🙏🏻

anybody’s picture

Version: 8.x-2.39 » 3.0.x-dev
Status: Reviewed & tested by the community » Needs work

Fixes should go into 3.0.x now. Can someone do that please?

arunkumark changed the visibility of the branch commerce-3118588-3118588-unable-to-_rb to hidden.

arunkumark changed the visibility of the branch 3118588-unable-to- to hidden.

arunkumark changed the visibility of the branch commerce-3118588-3118588-unable-to-_rb to active.

arunkumark changed the visibility of the branch commerce-3118588-3118588-unable-to-_rb to hidden.

arunkumark’s picture

Status: Needs work » Needs review

Created a new MR against the 3.0.x commerce branch.

orkutmuratyilmaz’s picture

Status: Needs review » Reviewed & tested by the community

I've applied the @arunkumark's MR as a patch and I confirm that I can delete the related field now.

jsacksick’s picture

Status: Reviewed & tested by the community » Needs work

Definitely not the right fix... The patch in the MR just loads all profiles... This would crash on a site with a lot of profiles, I don't believe we should do that, we should do a count query instead. Also, you could have profiles of other types too.

orkutmuratyilmaz’s picture

any updates on this?

jsacksick’s picture

So... We can fix the query and even refine it further to check if there is any tax number field with values, but I think the problem is by allowing deletion we open the door for broken installs (if the tax number field is accidentally deleted and the tax module isn't being uninstalled).

jsacksick’s picture

Version: 3.0.x-dev » 3.x-dev

jsacksick changed the visibility of the branch 3.x to hidden.

jsacksick changed the visibility of the branch 8.x-2.x to hidden.

jsacksick changed the visibility of the branch 3118588-allow-tax-number-field-deletion to hidden.

jsacksick changed the visibility of the branch 3118588-allow-tax-number-field-deletion to active.

jsacksick’s picture

Status: Needs work » Needs review

So perhaps we can go with the approach from https://git.drupalcode.org/project/commerce/-/merge_requests/465. We basically allow deletion of the field if there are no profiles with a tax number set in DB.

This still opens the door for a broken install, but... not sure how we could allow this only in the context of uninstalling the tax module.

jsacksick’s picture

@orkut murat yılmaz: Can you confirm the patch resolves the issue?

  • jsacksick committed 59353830 on 3.x
    Issue #3118588 by ben.hamelin, jsacksick, arunkumark, z3cka: Unable to...
jsacksick’s picture

Status: Needs review » Fixed

Decided to go ahead and merge the MR!

orkutmuratyilmaz’s picture

thank you @jsacksick :)

ben.hamelin’s picture

🙌 Whoo-hoo! Thanks all!

Status: Fixed » Closed (fixed)

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