Modules have no way of knowing if vocabulary machine names are changed

True. However :

The patch ensures that the $vocabulary object, built from submitted form values and passed from taxonomy_form_vocabulary_submit() to
taxonomy_vocabulary_save() and down to hook_taxonomy_vocabulary_update(), has an ->old_machine_name property.

This only works for form-based updates, and doesn't catch vocabs updated through direct API calls to taxonomy_vocabulary_save().

taxonomy_vocabulary_save() should retrieve the current machine name of the vocab based on the incoming vid (which, unfortunately, is still the official id key for vocabs in D7), and if the current machine name is different than the incoming one, set $vocab->old_machine_name.

Which, on a related note, reveals that the call to field_attach_rename_bundle(), currently made by taxonomy_form_vocabulary_submit(), should be moved out of the form workflow and into the taxonomy_vocabulary_save() API function.

Raising to 'major'. An API call breaking Field API and Pathauto is serious.

Looks good - but do we still need this hunk ?

Powered by Dreditor.

+++ modules/taxonomy/taxonomy.admin.inc	8 Aug 2010 00:02:07 -0000
@@ -163,6 +163,10 @@ function taxonomy_form_vocabulary($form,
+  $form['old_machine_name'] = array(
+    '#type' => 'value',
+    '#value' => $vocabulary->machine_name,
+  );

Powered by Dreditor.

Agreed.

RTBC after the bot comes green.

+++ modules/taxonomy/taxonomy.module	8 Aug 2010 00:02:08 -0000
@@ -372,8 +372,7 @@ function taxonomy_admin_vocabulary_title
-function taxonomy_vocabulary_save($vocabulary) {
-
+function taxonomy_vocabulary_save(stdClass $vocabulary) {

Please revert. We don't use type hinting for objects. There's a very lengthy discussion about this in another Drupal core issue.

Powered by Dreditor.

See #595084: Rollback type hinting for $node

+++ modules/taxonomy/taxonomy.module	29 Sep 2010 02:57:08 -0000
@@ -384,10 +383,17 @@ function taxonomy_vocabulary_save($vocab
+  if (!empty($vocabulary->vid) && !isset($vocabulary->old_machine_name)) {
+    $vocabulary->old_machine_name = db_query("SELECT machine_name FROM {taxonomy_vocabulary} WHERE vid = :vid", array(':vid' => $vocabulary->vid))->fetchField();
+  }
...
   if (!empty($vocabulary->vid) && !empty($vocabulary->name)) {
...
+    if (!empty($vocabulary->old_machine_name) && $vocabulary->old_machine_name != $vocabulary->machine_name) {

The first condition on $vocabulary->old_machine_name seems to be needless, as the code that is added right in front of this already makes sure that $vocabulary->old_machine_name is set.

Powered by Dreditor.

Thanks!

Although badly needed, this is D8 material according to the rules (I had to learn today).

Definitely for D7, of course.
I would also argue that a bug that blocks modules like Pathauto or XML sitemap qualifies as 'critical', but well.

+ crosslinking from #881530: Exportability of vocabularies is ruined by taxonomy field's 'allowed values' setting, since the bug here more generally prevents storing vocab-related data based on machine name rather than vid, which is another important point on the road to vocab exportability.

This issue perfectly demonstrates why we need #651240: Allow modules to react to changes to an entity.

+  if (!empty($vocabulary->vid) && !isset($vocabulary->old_machine_name)) {
+    $vocabulary->old_machine_name = db_query("SELECT machine_name FROM {taxonomy_vocabulary} WHERE vid = :vid", array(':vid' => $vocabulary->vid))->fetchField();
+  }

Ouch. But probably the best we can do without a generic solution now.

That query in taxonomy_vocabulary_save() is ugly has hell, I can see why this is needed though. There are some notes on better entity update APIs in http://groups.drupal.org/node/62633 for anyone who's interested.

But

+    if ($vocabulary->old_machine_name != $vocabulary->machine_name) {
+      field_attach_rename_bundle('node', $vocabulary->old_machine_name, $vocabulary->machine_name);
+    }

Er, no. That also proves we need tests for this.

I think we should move towards not allowing changes to machine names, just as you would never bother to change a node id.

After working for quite a long time with systems that don't allow you to change machine names (Views etc.) I find that it's just not needed.

What's worse, the infrastructure we need as a community to allow for machine names to change is busy and brittle at best: how many D 6 modules out there are not implementing hook_node_type() where they should?

In case it's not obvioius (took me 30 seconds...), what catch is referring to in #23 is :

+      field_attach_rename_bundle('node', $vocabulary->old_machine_name, $vocabulary->machine_name);

'node' should be 'taxonomy_term', of course. So yes, tests needed. Will try give it a shot later today.

Updated patch with test.

+ now that taxo_term $field definitions refer to vocabs by machine_name rather than by vid (after #881530: Exportability of vocabularies is ruined by taxonomy field's 'allowed values' setting), we also need to update them when a machine name is changed. Nothing new here, nodereference fields have done the same for years when a node type name changes.
Also added a test for that other part.

#25: Agreed. This is a bug, image style names and node types can change and have a way of letting other modules know, taxonomy doesn't. This patch follows the general pattern of node. Permanent machine names is a conversation for another issue.

Dug into the failing tests, could not figure out what causes the reported notices:

Undefined index: taxonomy_term           Notice   field.info.inc          677 field_info_instances()	
Invalid argument supplied for foreach()	 Warning	field_test.storage.inc  460 field_test_field_attach_delete_bundle()

It appears that when a machine name changes, calls to the field API from taxonomy_taxonomy_vocabulary_update() don't find the field instances they are looking for field_info_instances() >> _field_info_collate_fields().

Fixes the failures.

For those interested : those failures were actually a non-related bug in the dummy 'field storage backend' implemented in field_test.module - more specifically, its implementation of hook_field_attach_delete_bundle() had an incorrect signature.
It was only triggered by the patch as a side effect of enabling field_test.module in TaxonomyVocabularyUnitTest::setUp(). Some of the tests in this class (not those touched by the patch) delete some vocabs, which calls field_attach_delete_bundle() and its associated hook, which triggers the bug. Did not happen so far because until now those tests ran with field_test.module disabled.
I'm surprised that no existing test has caught this so far. There are definitely existing tests triggering a field_attach_delete_bundle() while field_test.module is on... I'm looking into this, just to be sure.

Anyway - as far as this patch is concerned, this should be ready..

Better fix for the broken field_test_field_attach_delete_bundle().

yched: thanks for detailing the hook_field_attach_delete_bundle() bug.

This is RTBC now.

I am setting the priority critical because without this patch, any changes to a vocabulary machine name will result in taxonomy term reference fields breaking. (Reproduce by adding taxonomy field to content type, create node with terms, change vocab name, edit and save node).

Good catch. We should. Will post shortly.

Use hook_field_attach_rename_bundle() instead of hook_taxonomy_vocabulary_update().

How on earth did that mistake pass tests? Sounds like we need expanded test coverage for renaming machine names?

What I mean is that #34 identifies some blatantly wrong code in the patch. And yet #31 passes testbot. This should not happen.

Ok, nevermind. I had interpreted #34 as saying "you made a typo" when it was instead saying "there is a more optimal hook into which you can place that code."

Carry on. ;)

Fixed according to #41

Shouldn't we be implementing hook_field_attach_rename_bundle() in taxonomy.module rather than hook_taxonomy_vocabulary_update()?

Arguable, I'd say. Both hooks will be fired sequentially anyway, so the difference is mainly esthetic. Personally, I tend to think that the more specific hook_taxonomy_vocabulary_update() is exactly what we want, and that hook_field_attach_rename_bundle() is more obscure here.
+ hook_field_attach_rename_bundle() is invoked more "frequently" (e.g node type change) and means more useless no-op function calls. Rare anyway, so not really important in practice, but makes more sense as a pattern IMO.

So I personnally favor #31 over #43, but either way, both are RTBC.

+++ modules/taxonomy/taxonomy.module	10 Oct 2010 20:17:22 -0000
@@ -434,6 +440,31 @@ function taxonomy_vocabulary_delete($vid
+      if ($field['type'] == 'taxonomy_term_reference') {

Will this actually trigger? In the code about we call it with 'taxonomy_term' instead of 'taxonomy_term_reference'. Need to look at it more but figured I'd report this. I apologize if I overlooked something.

Yes, it will: taxonomy_term_reference is the field type provided by taxonomy module (see taxonomy_field_info()).

Looks fine to me now.

Just so that it doesn't get lost - from my #44 : "I personnally favor #31 over #43, but either way, both are RTBC."
See #44 for explanations.

Upon reviewing this issue, I agree with yched. We know the extra semantics to add to this event (we're updating a *vocabulary* rather than some sort of generic bundle), so let's use it. And looks like Dries's concern was addressed above.

However, #31 doesn't apply anymore. Could we get a quick re-roll?

Yeah in this case the rename bundle hook is the more targeted one, vocabulary update and bundle rename are two distinct events. The only thing that could justify putting this in hook_taxonomy_vocabulary_update() would be if we moved the field_attach_rename_bundle() logic there too, but that would break the current pattern of having field_attach_*() calls directly in CRUD functions. So I think this should go in as is.

Hm. Ok. I can see that perspective.

However, #42 doesn't apply either. :P

Great!

Committed to HEAD. Thanks, everyone. Another critical bites the dust. :)

Sorry for being picky, but I realized the correct reason why I felt that using hook_field_attach_rename_bundle() over hook_taxonomy_vocabulary_update() is off.

A vocab machine name change only triggers hook_field_attach_rename_bundle() because it so happens that taxo terms are fieldable entities, and that the bundles are the vocab machine names.

This has nothing to do with the fact that there exists a 'term reference' field type, and that its $field definitions store a vocab machine name, which need to be updated when the name changes - the fact that this name is also the name of a bundle for fieldable terms is incidental.

That's a job for hook_taxonomy_vocabulary_update(). Doing it in hook_field_attach_rename_bundle() works too, but that's pure luck. X being a fieldable entity and the existence of an 'X reference' field type are two completely decorrelated facts.

Attached patch reverts to hook_taxonomy_vocabulary_update(), as in #31.

That's a good argument, and not a picky one.

Okie doke then.

Committed #56 to HEAD. Thanks!