Taxonomy term synonyms and relations clear after re-ordering using drag-and-drop feature

Losing data this easily sounds like a critical issue to me.

Same problem, synonyms got deleted after reordering terms with ajax.

Line "353" in "taxonomy.module", inside the function "taxonomy_save_term"

db_query('DELETE FROM {term_synonym} WHERE tid = %d', $form_values['tid']);

runs also on reordering so synonyms get deleted. As a temporary patch I added an "if" to prevent deletion:

if ($_POST["form_id"] != "taxonomy_overview_terms") {
           lines 353..360
}

and seems to work, but that's just a trick

Devis Lucato

Same problem here, in 6.6.

I have over 150 terms in one of my given vocabularies, and I tend to lose all but 25 synonyms when re-ordering using drag and drop.
It seems as though few people seem to use synonyms, so this issue may go unnoticed, as it seems to have for more than half a year :P

If it's still killing me as much as it does, I'm going to look into making a patch, though i hope someone with a little more experience can take a crack at it.

Confirmed that the same bug is present in Drupal 7. And not just synonyms, related terms too.

We'll need to fix it in the development version first then backport.

OK it's pretty obvious why this is happening, but not necessarily easy to fix in a clean way.

The issue is that when re-ordering the taxonomy tree, the terms are loaded via taxonomy_get_tree() - this does a direct query on the term_data tables, which doesn't contain synonyms or related terms. taxonomy_get_term() in 6 or taxonomy_term_load() in Drupal 7 also don't load synonyms into the term object so aren't any use here either. So when these terms are saved, they're incomplete, and taxonomy_term_save() deletes the synonyms and related terms - since they're not a part of the term that's passed in.

For Drupal 7, I think we should add an explicit flag to vocabularies for whether they support synonyms or related terms, then load these into the term object in hook_taxonomy_term_load() - and ensure that's used to get the term objects consistently. That would make the object returned by taxonomy_term_load and saved by taxonomy_term_save actually the same object.

For Drupal 6, we either need to query the synonym and related terms tables directly before saving the term, or we could use a direct db_query to update the parent information instead of calling taxonomy_term_save(). The second option would be more performant I think.

Here's a working patch for Drupal 6 - re-ordering works without data-loss.

Since the changes necessary to do this cleanly are quite a long way off, I'm tempted to re-roll this for Drupal 7 with a test and suggest we commit it there too.

Re-roll for Drupal 7. No test yet.

hi all,
i think this patch works not correctlyand it should be fixed in taxonomy_save_term() (for D6)...
georg

to delete synonyms this patch has to be executed too :-) (I forgotten)

gbuske - that doesn't deal with the fact we don't have full term objects here in the first place - so other fields like related terms could equally be destroyed.

Rerolling patch from #8. Patch passed the test locally for me that it failed on testbed.

+      ->fields(array(
+        'parent' => $term->parent,
+      ))

This is really minor, but according to Crell (#394484-3: DBTNG: Node module) should "single-value fields calls" be on a single line, no need to split them up.

Ok. Fixed the whitespace.

is this most recent patch posted for D7 or D6? it's not too clear whether people are working on the current version and backporting, or just working in D6 from comments...

D7, as it says at the top

In LA codesprint right now - I submitted the last patch for re-testing because when I applied the patch to a fresh d7 install, my manual testing caused the following error when I tried to move the taxonomy term up:

• Warning: include(/Users/rain/Documents/SUNRAIN/sites-in-progress/drupalhead/codesprint/includes/maintenance-page.tpl.php): failed to open stream: No such file or directory in theme_render_template() (line 1145 of /Users/rain/Documents/SUNRAIN/sites-in-progress/drupalhead/codesprint/includes/theme.inc).
• Warning: include(): Failed opening '/Users/rain/Documents/SUNRAIN/sites-in-progress/drupalhead/codesprint/includes/maintenance-page.tpl.php' for inclusion (include_path='.:/Applications/MAMP/bin/php5/lib/php') in theme_render_template() (line 1145 of /Users/rain/Documents/SUNRAIN/sites-in-progress/drupalhead/codesprint/includes/theme.inc).

This error did not show up when I reversed the patch.

I suspected that perhaps with the patches I've already applied today my Drupal install may have had something going on, adding to the problem, so I tried a completely fresh update from Drupal Head and an entirely fresh database (so, 100% clean install). The error still occurred and reversed exactly as it had on my previous test.

I also requested a retest on the patch in #17.

Those last comments seem irrelevant, so this still needs review.

The comment #20 was relevant but incomplete.

This patch no longer work because the tables were renamed from term_data to taxonomy_term_data and from term_hierarchy to taxonomy_term_hierarchy. I will reroll the patch with the new table names.
Because the table names were changed, Drupal tried to show the maintenance page but instead showed a blank page with some warning in watchdog.

There seem to be another problem related to the maintenance page which should not be addressed here.

Here is the rerolled patch which takes into consideration the table name changes.

oh man, that was me that changed those table names... I can test.

this should be backported after commit to HEAD.

Needs to be backported after commit.

I can't reproduce the original book in HEAD now. (yay?)

roychri, could you reproduce the bug? If not, this issue should go back to 6.x.

@Jody Lynn - Yes, I can reproduce the problem.

At first I could not reproduce the problem. But I found a way.

So here is how I reproduce the problem:
1- Installed Drupal7 fresh.
2- Added a new vocabulary with options left as default (tags: off; required: off; multiple select: off)
3- Added 4 terms to that vocabulary
4- Added a synonym to the second term in the middle of the list.
5- Click on "list terms" for that vocabulary.
6- Re-order the second term that I added using drag&drop to the position #3 (was #2).
7- Click "Save".

This should do it.

I think I had trouble with only 2 terms because it would update the other term's weight, not the one with the synonym, even if that was the one I swapped.

Thanks, yes, my problem was I only had 2 terms, seems like you need 3 or more to reproduce.

The patch fixes the problem and code looks good.

fyi, another patch is rtbc that removes synonyms from core.

This patch will still be valid for any module which uses hook_taxonomy_term_*() to maintain it's own data - we should never call taxonomy_term_save() with a fake/incomplete term object.

FYI, #567572: Remove taxonomy synonyms since Field API is better was committed to CVS HEAD.

Retesting the last patch. Let's see if this works.

Still works, still needed.

Committed to HEAD. Thanks!

There's a 6.x equivalent of this patch up there somewhere. Didn't test it, but if you do, mark RTBC.

Hm. And also, we could use some tests on this, come to think of it...

marking "needs work" for tests

I looked into writing a test for this, but now that taxonomy synonyms have been removed, there's nothing left to test for (there's no bug that occurs before or after the patch anymore)

In Drupal 6.14 the synonyms also disappear when using the Reset to aplhabetical button.

This needs to stay at 6.x-dev as that's where it will have to be fixed.

There is also a patch for 6.x per #36 that should be tested per #36.

and please don't alter the title.

#11: taxonomy.module.2.diff queued for re-testing.

In case anyone else is still experiencing this issue with Drupal 6.16:

The patch from #7 didn't work for me "out of the box" -- I had to change
$term->parent to $term['parent']
$term->tid to $term['tid'] and
$term->weight to $term['weight']

So, the updated lines would read:

    db_query('UPDATE {term_hierarchy} SET parent = %d WHERE tid = %d', $term['parent'], $term['tid']);
    db_query('UPDATE {term_data} SET weight = %d WHERE tid = %d', $term['weight'], $term['tid']);

It's unclear to me whether the patch from #12 is relevant -- the if statement case seems to have been deleted.

Lastly, I suspect that a cache_clear_all is needed at the end of taxonomy_overview_terms_submit since the patch uses a direct database query.

I am a bit surprised that no protective measure (e.g. a warning about the data loss, or disabling drag-and-drop reordering) seems to have been taken when this was detected.

I changed this from "needs review" to "needs work" because (according the latest comment #44), the patch for the 6.x tree (in #11) does not work in 6.16.

If someone has the time, please apply #44's changes to the diff and re-submit for review. Thanks!

The attached patch incorporates #11 and #12 for D6.

Tested on a large vocabulary by saving a synonym, re-ordering using Drag and Drop, Reset to Alphabetical and lastly cleared.

Everything seemed to work for me.

The update queries mentioned in #44 based on patch on #7 don’t seem to be relevant.
At first I thought it might be relevant because my query’s kept returning "0 rows affected" but this ended up being related to Weight fields should be int, not tinyint

I just tested this patch very minimally, and it seems to work (I didn't get the error messages that I got before). So, ignore my comment in #44.

I think I might have gotten confused between the D6 and D7 patches. Sorry for any trouble/inconvenience.

#944100: Problem in taxonomy interface, erase taxonomy relation related?

Just hit this bug on 6.16, patched with #46, tested on synonyms & reordering, worked.
Upgraded to 6.19, bug still there. Applied the same patch again, tested on synonyms & reordering, worked.

Also just tested in 6.16 -- was encountering same issue. #46 appears to have worked perfectly. Thanks!

This is the same underlying bug that has given rise to other issues regarding relations. I'm marking those that aren't already as duplicates and changing the title to encompass those as well - (#480806: Term information lost when using drag and drop on list terms page, #748412: synonyms lost when terms are re-weighted, #748426: synonyms lost when terms are re-weighted, #944100: Problem in taxonomy interface, erase taxonomy relation - there could be more...).

Looking through this issue, it looks like the approach taken in #24 (and committed to 7.x-HEAD in #36) is the best way to fix this - noting in particular catch's comments in #5, #6, #7, and #32. I.e. "use a direct db_query to update the parent [and weight] information instead of calling taxonomy_term_save()".

This patch implements jerryplin's D6 adaptations to the D7 patch as detailed in #44.

If this passes testing - please can this be the end of these 2 and 1/2 year old critical bugs!?!?

Rogue period in the patch in #52.

Rogue period fixed in original patch...

Sorry for the mess - but I have no idea why this patch is failing. It's created and tested against a fresh checkout of DRUPAL-6. Help please Robin... anybody...?!

#57: drupal-251255-52.patch queued for re-testing.

D6 testing is broken - #961172: All D6 Core patches are failing.

#55: Index_ modules_taxonomy_taxonomy.admin_.inc_.diff queued for re-testing.

Subscribing - I use synonyms for a lot of stuff on my site and I had a couple heart attacks until I realized what was causing the synonyms to be removed. We're about to hit the 2 year anniversary on this issue :)

subscribing. This issue just wrecked my site as well; I can't wait for it to be fixed.

Patch works for me. RTBC.

It's a shame that we can't use, you know, the term saving function to save a term. But the problem is that taxonomy_save_term() is *not a term saving function*, as its doc header states, it's a helper function for taxonomy_form_term_submit() and it expects form values from a particular form. Hence trying to use it to save a $term array leads to funny things going wrong :/

subscribing. 1798 synonyms gone. poof. Not sure when it happened.

I have a classified site that relies on synonyms as well. I hope this can be solved some day. I wonder what the longest standing bug is? Can we get an award for this?

Please don't change tags set by maintainers, and please don't add tags that are meaningless ('bug'? well, duh) -- review the issue tag guidelines please: http://drupal.org/node/1023102

@joachim - Do you know what that "Needs tests" tag means? Does it mean that until 6.x automated testing is fixed and the patch passes, this issue has no chance of being committed? If so, perhaps it should be made to quietly disappear...

Um, I don't *think* we have a test framework for core D6, only D7. But then this perhaps still needs tests for the D7 patch? Does anyone care to read through all the comments to see?

Yes there is a test framework for D6 but it's broken - #961172: All D6 Core patches are failing. This issue doesn't exist in D7 since there is no support for synonyms or related terms.

Well something was committed to D7 by webchick in #36... and I see the code from the patch in #24 in core.

So it probably still needs tests on 7.

I think the best thing to get this committed is for people who care about it to try and spot a core maintainer in IRC and tell them there's a quick and simple patch that's RTBC. IIRC the D6 maintainer other than Dries is Gabor.

Newbie here just came to the realization that he just wasted more than a week on adding related terms and synonyms before realizing that the disappearing data wasn't just a fluke and that reordering was causing all of that work to magically disappear from existence. Somewhat relieved, somewhat disappointed these form fields haven't just been REMOVED or FIXED by now. Sorry, I'm not athletic enough in Drupal development coding to help fix it but clearly one action or another should be taken. I'm surprised this made it past dev, alpha or beta testing without some action being taken, not realistic to think somone should never reorder or should have to completely redo it all when they have to. ARGHHH said the newbie pirate!

This patch is RTBC and ready to go -- all that needs to happen is for one of the two D6 maintainers to commit it (and then a D6 release to be made in due course). Probably worth pinging them on IRC if you spot them :)

@deanflory: Taxonomy synonyms are not widely used at all -- hence why nobody would have noticed. Also why they've been removed from Core in D7 :)

Joachim, my only reasoning was to offer more relevant results in searches, was the only thing I could think of where those would be useful. Say for example someone searching for Sports would find UNC Athletics without that additional synonym within the node's text.

Is their a separate module such as "Synonyms" for D7 that would provide that functionality? Just wondering. Thanks.

I used Synonyms semi-heavily on a large site. It seems to make perfect sense when you watch to attach information to a taxonomy term itself.

Same here. I use them to prevent too-similar terms via unitag and to generate page titles and extra SEO meta-data on term pages.

Personally I think together with related terms, synonyms are an essential part of the taxonomy sub-system.

@deanflory - For D7 you use a field. The synonyms project can provide one for you.

Reviewed the patch. I think the main problem here is that we don't take taxonomy extension modules into account. While taxonomy_term_save() is just a helper, it does inform all other modules that the term changed (and passes on all data about the term). Looks like to be backwards compatible, we should rather load the term, update the data on it and then pass it on as if it was a regular form based update, so modules implementing taxonomy hooks can react to the change. As the patch stands, we remove features while we fix the bug.

@Gábor - thanks for taking the time to review the patch.

I see your point but there are pros and cons to both approaches and IMO the proposed patch is the most pragmatic:

1. There is no explicit contract that taxonomy hooks are invoked for any update to a term - only when the term is updated via the term edit form. The docs on taxonomy_term_save() state that it is a "Helper function for taxonomy_form_term_submit()" and the docs for hook_taxonomy() state that the term values array contains "for terms, 'insert' and 'update' ops: $form_values from taxonomy_form_term_submit()".

2. From the above it seems that the use of taxonomy_save_term() in the taxonomy_overview_terms_submit() is inconsistent with it's intended use and is more likely to cause problems for contributed modules than the proposed patch.

3. There is precedent for updating term weights directly without hook invocation in taxonomy_vocabulary_confirm_reset_alphabetical_submit().

4. If as you suggest the taxonomy hooks must be invoked, to do so correctly would require executing the term edit form for every affected term. Re-ordering/re-parenting can require many if not all terms in a vocabulary to be updated. For vocabularies with hundreds or thousands of terms it is unlikely that this would be performant and may require a batched operation.

I did not say there is a contract in Drupal to invoke the taxonomy update hook. I don't know how could we say there is one for any other hook invocation either. The simple point here is that it was invoked before the patch and there can be any number of modules among the thousands on drupal.org and any number of modules among the tens of thousand custom modules that people built their sites with. Drupal 6 can fix bugs as long as it keeps being backwards compatible. Removing features to fix a bug is not in the cards. Because reordering already happens with a UI operation, it does not seem to be impossible to do a batch if required. I'm not sure how the taxonomy ordering screen works if there are lots of terms, would it show and save all of them? Sounds like such a page would freeze browsers with the drag and drop controls. (I don't have a site handy with non-tag taxonomies of lots of tags to verify how its displayed and interacted with in practice though).

There is a contract (there has to be) as detailed in the docs, and that shouldn't change without very good reason. The call to taxonomy_term_save() in question is not part of that contract - it's an implementation detail that should be free to change. What's more it's an incorrect, critically flawed implementation that needs change.

If there are contributed/custom modules that depend on that particular taxonomy_term_save() call (not that they should!) then the only case where they would be affected is if they use the taxonomy hook to act when a term's parent has been updated.

BTW - The term overview page is paginated but the submit handler can update every term in the vocabulary.

Let's see, I looked at the documentation of http://api.drupal.org/api/drupal/modules--node--node.module/function/nod... to see which hooks should it invoke as per your suggestion. It is not documented, so I guess as per your understanding we should be free to remove any and all hook invocations from the function given it has no hook invocations documented additionally them being in the implementation? I'm not getting the logic here, sorry.

But changing the order of terms already unreliably invokes hooks -- taxonomy_vocabulary_confirm_reset_alphabetical_submit() reorders but doesn't invoke. So any modules relying on this are already stumped by a broken system.

On the other hand:

> to do so correctly would require executing the term edit form

I don't think so -- we can just load the full term and pass it to taxonomy_save_term(). It's fiddly, as we need to load up both related terms and synonyms into the term object before handing it over to taxonomy_save_term.

Or...

The alternative might be for taxonomy_save_term() to check isset($form_values['relations']) and synonyms before wiping that data -- would that fly as a fix?

@joachim: taxonomy_save_term() uses drupal_write_record() which needs the whole object and will not default with existing values, so not sure how an isset($form_values['relations']) would work there.

It uses drupal_write_record for term_data. But the other stuff is just hacked about like this:

  db_query('DELETE FROM {term_synonym} WHERE tid = %d', $form_values['tid']);
  if (!empty($form_values['synonyms'])) {
    foreach (explode("\n", str_replace("\r", '', $form_values['synonyms'])) as $synonym) {
      if ($synonym) {
        db_query("INSERT INTO {term_synonym} (tid, name) VALUES (%d, '%s')", $form_values['tid'], chop($synonym));
      }
    }
  }

What I am suggesting is that it bypass this bit if $form_values['synonyms'] is not set at all -- and therefore delete nothing.

Basically, fixing this bug can be done in one of three ways:

a) don't call taxonomy_save_term() because it's a hacky POS.
b) call taxonomy_save_term() and give it the data it expects
c) fix taxonomy_save_term() to not clobber data it knows nothing about

Gábor is ruling out a), so we're on to b) or c).

Ok, c) looks to retain the current behavior (minus our bug), without introducing performance issues, so I think it sounds fine. It would also fix this for others who for some reason think that the save function is an API function (despite the docs, yes).

I'd really like to see this fixed but c) will cause problems for contributed modules that alter the term form and then use the taxonomy hook to do stuff expecting all form values to be in the array. Yes, that's the current behaviour but the potential for adverse consequences is much reduced by the proposed patch.

You're saying modules may have done unset($form_values['synonyms']) rather than empty($form_values['synonyms']) with the intention of deleting synonyms?

If they break because of that it's kinda their fault...

No, I'm I saying that a module that adds supplementary information to the term form via form alter, implements the taxonomy hook expecting that information to be in the array. The missing data may cause it to delete the information it holds just as taxonomy_save_term() does with synonyms and related terms.

The problem is that taxonomy_save_term() should be called with all the data from the term form. We should either call it with everything or don't call it at all.

Having said that, it's much more important that this bug gets fixed and c) is a good enough compromise in that as Gábor points out it fixes the bug and retains current behaviour. Any modules currently broken will remain broken - so no surprises.

Subscribing, this has been bugging me for years!

Subscribing since i also lost the synonyms. Issue still exists
If there is no patch i would prefer disabling the reordening feature. Did someone this as well?

There is a workaround for this issue. If you use a free-tagging vocabulary (go to "edit vocabulary" and un-check the "tags" option), then the drag-and-drop reordering feature, and the "Reset to Alphabetical" button disappear. These features are cool, but they are unnecessary for free-tagging vocabularies, as the order of the terms is not displayed to the user at node creation / editing time.

Now, if you want the free-tagging user interface (type in your terms, with autocomplete, rather than scroll thruogh a big list to select) and the lack-of-synonyms-bug, but you do not want to let your users add new taxonomy terms, then you can rely on the awesome unitag module (also mentioned in #78) to make your free-tagging vocabulary read-only.

Now I think that perhaps I am missing something, but this bug seems like almost a non-issue to me. Data loss is really bad, so it is high-impact and should be fixed. However, I would think it would be be rare for this problem to affect anyone on a day-to-day basis, as those who are not using free-tagging vocabularies cannot see or use synonyms -- at least during node creation / editing.

"those who are not using free-tagging vocabularies cannot see or use synonyms" That's not true, maybe not during node creation / editing. This is however a big issue for me as I use taxonomy synonyms a lot for urls, when the urls no longer work because someone re-ordered that, well, sucks.

I cant replicate this in 6.22...

I might be missing something, but if the taxonomy_overview_terms form only allows the weights and hierarchy to be updated then, the patch suggested in 52 should work. If we have concerns about contrib modules losing visibility of the changes to the taxonomy terms then we should be able to replicate what would happen if taxonomy_save_term were called, which would mean that they are no worse off.

Either way I think the first priority should be to fix the bug affecting core, worrying about a number of potential use cases from contrib is justified, but do we even know that there definitely is an issue there? I think we can afford to be a bit pragmatic here.

On my side it work and it solve the problem ! Could we apply the patch for futur update...

Work with D6.24.

Thanks.

This was commited ?
I just update drupal core to 6.24 and problem is solved. Whithout use this patch.

Very reluctant to advise a client to use taxo synonyms because of this. Not only because of reordering through the UI, but also because it's very hard to ensure everyone in the dev team resists the temptation to use taxonomy_save_term() as an API func.
And currently, taxonomy_save_term(taxonomy_get_term($tid)) can do very bad things...

Gabor already stated that he rejects the approach in #52 / #97 for BC reasons, and favors this approach :
"c) fix taxonomy_save_term() to not clobber data it knows nothing about" (see #87 / #88)

That's also the conclusion I came to myself :-), so, patch attached.

I can confirm that the patch in #101 appears to work for me using the latest version of Pressflow Drupal 6. I re-ordered the terms and no relations were lost.

I would like to see this in the next release of 6.x if possible.

yched, in my tests, the patch in #101 has the unfortunate side effect of not re-inserting the term into the hierarchy table if it doesn't have parents -- e.g., top level terms.

When the patch checks for this:
if (is_array($form_values['parent']))

Shouldn't it check for this:
if (is_array($form_values['parent']) && count($form_values['parent']))
?

I've verified that the patch in #97 works on Drupal 6.26. My test site also has taxonomy_image and term_fields, and the additional fields provided by both worked fine. (Neither makes a call to taxonomy_save_term.)

As DeeLay said in #97, the priority should be to fix the bug in core that causes data loss. Any contributed modules that wrongly use taxonomy_save_term() most likely have already been updated, since those modules would have triggered the same bug that reordering triggers. If not, then those modules will continue to function as always.

It has been over four years since this critical bug was first reported. Let's not let paralysis and scope creep keep this bug from getting fixed any longer. Drupal 6 is still a supported version, so we can't just ignore this critical issue.

PS: If you're using related terms, you might also be interested in this issue: http://drupal.org/node/1630980

#101: taxonomy_save_term-251255-101-DC.patch queued for re-testing.

I hit this bug the other day when all the synonyms disappeared from one of my vocabularies, except those on the first term in the list. It's a top level vocabulary with no related terms, but many synonyms.

I can confirm that the patch in #101, together with aasarava's suggestion from #103 fixes this for my use case.

Here's a new patch incorporating aasarava's change.

Confirmed original bug, and tested patch.
Patch posted in #106 applies cleanly and resolved the issue of synonyms being deleted.
Tested on Drupal 6.26, php 5.3.6

Changing status to reviewed and tested.

D6 has so few tests, but let's run at least these. So...

#106: taxonomy_save_term-251255-106.patch queued for re-testing.

Looks like #111 was a testbot glitch, setting back to RTBC.

And again.