uc_catalog should not delete Vocabulary

Hello,
Maybe it is not a bug report but a feature request.

lphm.

Yep, this is part of the hook_uninstall process of uc_catalog. You're right, it really shouldn't do this. We still use the Catalog vocabulary for categorizing our content, despite the fact that we've disabled the UC Catalog module (we're using Views for our catalog pages, so we don't need it).

Attached is a patch which removes this part of the process.

Yes, and if someone using your module has selected their catalog taxonomy, and they decide to uninstall the uc_catalog module, there goes one whole vocabulary. The patch I posted prevents that from happening.

RTBC, and bumping priority as this can cause data loss - I have seen this first hand when upgrading an old D5/UC1 site to D6. Catalog had previously been installed and unbeknownst to me the vocabulary was in use as the main taxonomy on the site. I uninstalled catalog as I had no use for it on the new site, only to find all the categories had been unexpectedly deleted.

Here's my concern: I think the majority of sites are just using the catalog vocabulary created by uc_catalog, not replacing it with their own. In those cases, if you don't remove the vocabulary when you uninstall uc_catalog, it will remain to clutter up the database. So I think the patch needs to be a little smarter about deciding what to not delete.

TR: Understood; but in our case, I want to keep the uc_catalog vocab (which was the default from Ubercart). Looking back I wish I had never installed the uc_catalog module; that would let me avoid this problem.

What if we drupal_set_message() after the uninstall process a link that points to the taxonomy admin area, with a message stating that if they want to delete the vocab they have to do it manually? I think there's really no way to be 'smart' about it because as it is now, this is a data loss issue that someone could stumble into accidentally.

(Edited for some typos)

Exactly my point, most sites will use the vocab set up by uc_catalog to categorise their products - but my uninstall case was that I was replacing uc_catalog with Views, and I didn't expect the vocab and its terms to be deleted when I uninstalled uc_catalog after I no longer needed it; it's not obvious that the vocab is "owned" by uc_catalog as such.

I don't see how we can determine from code whether the admin intends for the vocab to be deleted at uninstall time, so I think it best to err on the side of caution - the vocab can easily be deleted manually, but recreating it after an unintended deletion is much more difficult.

Here's a revised patch that looks to see if a uc_catalog vocab exists, and if so, instead of deleting it provides the user with a message notifying them that it has not been deleted. Provides them instead with a link to the actual vocabulary's admin page.

Patch works as expected.

Oh, good. This change should have been made years ago.

One thing I changed before committing the patch: the link text wasn't translatable. When displaying links within copy text, the best practice is to replace only the URL in the href attribute.

    drupal_set_message(t('The Ubercart Catalog vocabulary has not been deleted. If you need to delete it, <a href="!url">please do so manually</a>.',
      array('!url' => url('admin/content/taxonomy/edit/vocabulary/'. $vid))), 'notice');

This lets translators work on the sentence as a whole and gives them the freedom to change word order, or even the link's position as necessary.

Good catch. Thanks, Lyle!

Needs porting to 7.x.

Don't forget to give control of this vocab back to taxonomy module. I.e. set the module field in the vocab table.

db_query("UPDATE {vocabulary} set module='taxonomy' where module like 'uc_catalog'");

@sean-gnu: What is that field even used for, except hook_term_path, which Ubercart doesn't implement anyway? Perhaps "taxonomy" should always be the owner? #425984: Employ a hook_term_path() for catalog is relevant though I see you already posted there!

In fact, looking at the source for taxonomy_term_path(), it looks like we don't need to worry about setting the owner back to "taxonomy" as it will fall back to the correct value anyway?

@longwave - yes - it's used for hook_term_path. uc_catalog alters the term_path to no longer be 'taxonomy/term/%', and replaces it with 'catalog/%'. Thereby giving control to uc_catalog's menu system, and hence bypassing the standard taxonomy menu's system.

Perhaps module_invoke will not call a module that is uninstalled (haven't checked) - but the line of code I suggested is part of proper cleanup of uc_catalog uninstall.

Just a suggestion :) It has its place in my patches and thought I'd share !

@longwave - my patch here : http://drupal.org/node/425984 includes an attempt to get the issue mentioned here sorted as well. Sort of a bonus ! It did not include any message though.

My reasoning was - back then - that deleting uc_catalog should remove the catalog "handler", not the data itself and since uc_catalog can attach itself to an existing vocab, it should most definetly not delete it on uninstall.

So, thumbs up for this patch.

Since uc_catalog stopped using hook_term_path(), the only reason I can think of for that module field is just to have a record that uc_catalog did indeed create that vocabulary. If an existing vocabulary becomes the catalog, I don't think it's module is changed to uc_catalog. In fact, that may have been the reason hook_term_path() was dropped. At any rate, I don't see a need to change that field when uc_catalog is uninstalled.

Patch for 3.x.

Committed, with a fix to the vocabulary edit path.