A taxonomy term synonym clears after re-ordering using drag-and-drop feature

Max_R - April 25, 2008 - 15:30
Project:Drupal
Version:6.x-dev
Component:taxonomy.module
Category:bug report
Priority:critical
Assigned:Unassigned
Status:needs review
Issue tags:backport, Needs tests
Description

A taxonomy term synonym clears after re-ordering using drag-and-drop feature. To reproduce, go to list terms, assign a synonym to a term, save the term and then re-order terms using mouse drag-and-drop.
Press "save" and here you're.

#1

Rowanw - April 26, 2008 - 00:37
Priority:normal» critical

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

#2

Devis - April 26, 2008 - 23:58

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

#3

Robin Monks - August 29, 2008 - 19:30
Title:A taxonomy term synonym clears after re-ordering using darg-and-drop feature» A taxonomy term synonym clears after re-ordering using drag-and-drop feature

#4

techninja - November 24, 2008 - 01:30
Version:6.2» 6.6

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.

#5

catch - December 16, 2008 - 19:50
Version:6.6» 7.x-dev

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.

#6

catch - December 16, 2008 - 20:07

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.

#7

catch - December 16, 2008 - 20:28
Status:active» needs work

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.

AttachmentSizeStatusTest resultOperations
drag_and_drop.patch823 bytesIdleFailed: Failed to apply patch.View details | Re-test

#8

catch - December 16, 2008 - 22:17
Status:needs work» needs review

Re-roll for Drupal 7. No test yet.

AttachmentSizeStatusTest resultOperations
save_synonyms.patch2.06 KBIdleFailed: 9616 passes, 0 fails, 1 exceptionView details | Re-test

#10

System Message - February 9, 2009 - 01:20
Status:needs review» needs work

The last submitted patch failed testing.

#11

gbuske - February 24, 2009 - 10:00

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

AttachmentSizeStatusTest resultOperations
taxonomy.module.2.diff1.13 KBIdleFailed: Failed to apply patch.View details | Re-test

#12

gbuske - February 24, 2009 - 11:27

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

AttachmentSizeStatusTest resultOperations
taxonomy.module.3.diff658 bytesIdleFailed: Failed to apply patch.View details | Re-test

#13

catch - February 26, 2009 - 04:39

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.

#14

Jody Lynn - March 30, 2009 - 03:14

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

AttachmentSizeStatusTest resultOperations
save_synonyms.patch1.23 KBIdleFailed: 14494 passes, 4 fails, 1 exceptionView details | Re-test

#15

Jody Lynn - March 30, 2009 - 03:13
Status:needs work» needs review

#16

Berdir - April 25, 2009 - 20:44
Status:needs review» needs work

+      ->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.

#17

Jody Lynn - July 5, 2009 - 23:09
Status:needs work» needs review

Ok. Fixed the whitespace.

AttachmentSizeStatusTest resultOperations
save_synonymns.patch1.13 KBIdleFailed: 14495 passes, 4 fails, 1 exceptionView details | Re-test

#18

arianek - July 30, 2009 - 22:57

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...

#19

Jody Lynn - August 1, 2009 - 18:22

D7, as it says at the top

#20

rainbreaw - August 16, 2009 - 20:23
Status:needs review» needs work

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.

#21

rainbreaw - August 16, 2009 - 20:30

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

#22

Jody Lynn - August 29, 2009 - 18:39
Status:needs work» needs review

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

#23

roychri - August 31, 2009 - 17:03
Assigned to:Anonymous» roychri
Status:needs review» needs work

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.

#24

roychri - August 31, 2009 - 17:07
Status:needs work» needs review

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

AttachmentSizeStatusTest resultOperations
251255.patch1.05 KBIdlePassed: 14486 passes, 0 fails, 0 exceptionsView details | Re-test

#25

Jody Lynn - August 31, 2009 - 19:59

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

#26

Jody Lynn - August 31, 2009 - 20:16

this should be backported after commit to HEAD.

#27

Jody Lynn - August 31, 2009 - 20:23

Needs to be backported after commit.

#28

Jody Lynn - August 31, 2009 - 20:37

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.

#29

roychri - August 31, 2009 - 20:46
Assigned to:roychri» Anonymous

@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.

#30

Jody Lynn - August 31, 2009 - 22:24
Status:needs review» reviewed & tested by the community

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.

#31

moshe weitzman - October 15, 2009 - 04:10

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

#32

catch - October 15, 2009 - 04:29

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.

#33

Dries - October 15, 2009 - 14:34

#34

Dries - October 15, 2009 - 23:39
Status:reviewed & tested by the community» needs review

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

#35

catch - October 17, 2009 - 05:56
Status:needs review» reviewed & tested by the community

Still works, still needed.

#36

webchick - November 2, 2009 - 04:40
Version:7.x-dev» 6.x-dev
Status:reviewed & tested by the community» needs review

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.

#37

webchick - November 2, 2009 - 04:40
Issue tags:+Needs tests

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

#38

arianek - November 3, 2009 - 03:20
Status:needs review» needs work

marking "needs work" for tests

#39

Jody Lynn - November 3, 2009 - 22:22
Status:needs work» needs review

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)

#40

Weka - November 30, 2009 - 20:34
Title:A taxonomy term synonym clears after re-ordering using drag-and-drop feature» A taxonomy term synonym clears after re-ordering terms
Version:6.x-dev» 6.14

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

#41

VM - November 30, 2009 - 20:44
Version:6.14» 6.x-dev

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.

#42

VM - November 30, 2009 - 20:49
Title:A taxonomy term synonym clears after re-ordering terms» A taxonomy term synonym clears after re-ordering using drag-and-drop feature

and please don't alter the title.

 
 

Drupal is a registered trademark of Dries Buytaert.