path: #overlay=admin/structure/taxonomy/tagname
- click 'Show row weights' after creating some term ( all weights are '0' )
- set the weight of one term
- click save
now the items are not sort as they are supposed to.
Using the normal 'drag to re-order' works fine.
tested in firefox 3.6.10 and chrome 8.0.552.0 dev
issue occurs only when the weights of multiple terms are '0'
Comment | File | Size | Author |
---|---|---|---|
#97 | 941266-97.patch | 4.08 KB | nginex |
#89 | d7_order_of_terms_with-941266-89.patch | 1.33 KB | John Cook |
#86 | after-apply-patch-941266.png | 33.55 KB | amit.drupal |
#86 | before-apply-patch-2-941266.png | 33.85 KB | amit.drupal |
#86 | before-apply-patch-941266.png | 34.01 KB | amit.drupal |
Comments
Comment #1
scuba_flySo as seen in the attached screen shots, i would expect the term 'Bank' to be on top of the list.
Comment #2
scuba_flyComment #3
pfrenssenI can confirm this issue in 7.x-dev. What's more, you don't even need to change any weights. Just click on "Reset to alphabetical" and then again on "Save". The terms are then no longer in alphabetical order.
This happens because taxonomy_overview_terms_submit() does not order the terms the same way as taxonomy_get_tree() does. When the terms are reset to alphabetical, all weights are simply set to 0. This works fine with the standard taxonomy_get_tree() function as this shows terms with the same weight in alphabetical order. However when resubmitting the form taxonomy_overview_terms_submit() is called which only orders by weight, not by alphabet.
Comment #4
pfrenssenpatch
Comment #5
rggoode CreditAttribution: rggoode commentedI'm having a problem with this also. I'm running Drupal 7.8 and have a taxonomy that I want to use in a Jump menu.
I can set the desired order for the hierarchical taxonomy term list manually and that appears to work. But when I try to create a view using that vocabulary, the list order is completely different. When I go to admin/structure/taxonomy/vocabulary_name and change the view there to Show Row Weights, the term weights are totally random.
When I change the term/row weights to the order I'm seeking, the weights are lost as soon as I save the settings. They simply return to the random order (even though they are visually retaining the order I set manually)
The manually-set order is preserved in the taxonomy FIELD that I use to apply terms to content. But NOT in any taxonomy list that I try to create in Views. To make matters worse, when I try to set the sort order to just display by the term Title, in alpha order, it still retains the random order in the term weight.
Comment #6
xjmCross-referencing #815682: Taxonomy term ordering on node view & edit not consistent with taxonomy weight and #394422: Taxonomy terms should be listed in order they are entered, which also relate to term ordering.
Comment #7
hass CreditAttribution: hass commentedComment #8
marcingy CreditAttribution: marcingy commentedThis is not major
Comment #9
xjmLet's get an automated test to demonstrate this bug in 8.x. The original fix. will also need a reroll.
Comment #10
lliss CreditAttribution: lliss commentedThe attached patch creates a test that checks this functionality. The error can be found by resetting a list of terms to alphabetical and then simply saving the list of terms.
Comment #11
tim.plunkettFor the bot
Comment #13
pfrenssenThanks for the test! I rerolled my patch from #4 to the latest 8.x. First one is without test, second one includes the test.
Comment #14
pfrenssenComment #15
tim.plunkettFor future reference, the pattern is to upload just test and then the fix and test combined. But since we already saw it fail, this is fine.
Comment #17
pfrenssenThe test still fails because it is comparing the order in which the terms were initially created with the final alphabetically ordered result, and these are not necessarily the same, unless they were entered in alphabetical order.
What is the intentional behavior in this case? I assume the intention is to have all terms of equal weight to be ordered alphabetically, not in the order in which they were created. At the moment if you create a new vocabulary and enter some terms in random order (eg. apple - strawberry - orange - banana) and then list the terms they are shown in alphabetical order. Only after "resetting" and "saving" the order is back to the original order. This is the way it currently works in both D8 and D7.
Comment #18
tim.plunkettI just discusses this with lliss, and we came to the same realization. I think he plans to revisit this tomorrow.
Comment #19
pfrenssenI rewrote the test according to my assumptions from #17. Also improved the comments in the patch a bit.
Comment #20
tim.plunkett#19: 941266-19-taxonomy_preserve_term_ordering.patch queued for re-testing.
Comment #22
Albert Volkman CreditAttribution: Albert Volkman commentedRe-roll.
Comment #23
jthorson CreditAttribution: jthorson commented#22: order_weighted_terms-941266-22.patch queued for re-testing.
Comment #24
jthorson CreditAttribution: jthorson commentedAdjusting Tags.
Comment #25
dcam CreditAttribution: dcam commentedI tested #22. Prior to applying the patch, taxonomy terms that were given the same weight would not be ordered alphabetically after saving. After applying the patch, terms with the same weight were being properly ordered alphabetically. So it worked for me.
There was a whitespace error when applying the patch, an extra new line at the end of the test file. I'm not sure if that matters.
Comment #26
pfrenssen@dcam: thanks for the review! Whitespace does matter, so setting this to 'needs work'. I will revisit this asap.
Comment #27
tim.plunkettRemoving tag
Comment #28
Albert Volkman CreditAttribution: Albert Volkman commentedThanks for the reviews. The file was missing a newline, which is required per the coding standards-
http://drupal.org/coding-standards#indenting
Comment #29
paranojik CreditAttribution: paranojik commentedgit complained about a whitespace error (1 line adds whitespace errors.) and the test's class name was wrong, so the testing module didn't pick it up. I rerolled the patch with the fixes.
Otherwise the patch fixes the problem.
Comment #30
dcam CreditAttribution: dcam commentedThe patch no longer solve the issue. Equally-weighted terms are still sorted in non-alphabetical order after saving. It also fails its own test now.
Comment #31
Albert Volkman CreditAttribution: Albert Volkman commented@dcam so does the new test "WeightAlphaResetTest" fail for you locally?
Comment #32
dcam CreditAttribution: dcam commentedYes, sorry for not being clear about that. I tested it locally. I'll let Testbot try it again too.
Comment #33
dcam CreditAttribution: dcam commented#29: order_weighted_terms-941266-29.patch queued for re-testing.
Comment #35
David_Rothstein CreditAttribution: David_Rothstein commentedI don't think this should hardcode alphabetical sorting. Don't we actually want to just preserve the current order when the weights are the same?
I needed this for a Drupal 7 site, so here is a patch (temporarily) for Drupal 7 to let the testbot run on it. I'll post a Drupal 8 reroll in a bit.
This still needs tests (we'd need to check if the tests that were already written above can be modified for this), but in manual testing it seems to work.
It also might make sense to move this sorting function to somewhere more general, since "sort by weight but otherwise preserve the current order" could be a pretty common use case.
Comment #36
David_Rothstein CreditAttribution: David_Rothstein commentedOK, tests passed - here's the Drupal 8 patch.
Still needs to incorporate the new tests and possible other improvements discussed above.
Comment #37
dcam CreditAttribution: dcam commented#36: order-weighted-terms-941266-36.patch queued for re-testing.
Comment #39
sidharthapAssigning
Comment #40
sidharthapHere is the new patch. I tested the patch and it is working. simple test also pass for this patch.
Comment #41
sidharthaptagged to IndiaAug31
Comment #43
sidharthapi changed the test file.
Comment #44
leslieg CreditAttribution: leslieg commentedneeds reroll - patch did not apply to drupal 8.0-alpha3
Comment #45
pfrenssenComment #46
pingers CreditAttribution: pingers commentedRemove the whitespace.
Add a space after comment slashes.
More whitespace.
"If the weights are identical" might be better.
Can be simplified to just $a_order = isset($a['current_order'] ? $a['current_order'] : 0;
Too many new lines.
Comment #47
pfrenssenComment #48
pfrenssenStraight reroll.
Comment #49
pfrenssenAddressed the remarks from above and made the following improvements:
Comment #50
pfrenssenForgot the test only patch, here it is. This one should fail.
Comment #52
pfrenssenComment #53
pfrenssenIt's been over a month, let's retest the patch to see if it still green.
Comment #54
pfrenssen49: order-weighted-terms-941266-49.patch queued for re-testing.
Comment #55
leslieg CreditAttribution: leslieg commented48: order-weighted-terms-941266-48.patch queued for re-testing.
Comment #57
jthorson CreditAttribution: jthorson commented49: order-weighted-terms-941266-49.patch queued for re-testing.
Comment #58
leslieg CreditAttribution: leslieg commentedComment #59
pfrenssen49: order-weighted-terms-941266-49.patch queued for re-testing.
Comment #60
YesCT CreditAttribution: YesCT commented#49 failed. setting to needs work since the testbot did not. (could also use more direction as to the next steps here)
unassigning also since it has been a while. feel free to jump back in.
Comment #61
pfrenssen#49 was green actually! #50 is the one that is red, but it's the test-only patch which should fail to prove that it works. So this can go back to needs review :)
Comment #62
BerdirHm, messing with $form_state['values'] directly is a bit weird.
Not sure if there's a better possibility.
Comment #63
jhedstromNeeds a reroll at a minimum, and a response for #62.
Comment #64
Nitesh Sethia CreditAttribution: Nitesh Sethia commentedRerolled the patch
Comment #65
Nitesh Sethia CreditAttribution: Nitesh Sethia commentedComment #67
jhedstromPatch in #64 is the test only.
Comment #68
rpayanmTrying.
Comment #71
rpayanmComment #73
Sharique CreditAttribution: Sharique commentedIt should be
$values = $form_state->getValues('terms');
Comment #74
kerby70 CreditAttribution: kerby70 commentedReroll with change from #73 attached.
Comment #76
pfrenssenMarked #2411301: Alphabetical sort order lost after clicking save as a duplicate of this issue.
Please note that up to #49 this patch included a test, which has been lost in the reroll of #64. When this is rerolled the test should be added back.
Comment #77
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #78
pfrenssenRerolled and added back the test.
I just tested this manually and this use case doesn't work as expected:
Now "B" will not be moved above "A" but it remains in alphabetical order.
I also don't understand why the test changed like this. Reading the code it seems like this shouldn't be needed.
Comment #79
manauwarsheikh CreditAttribution: manauwarsheikh commentedComment #80
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commented@manauwarsheikh, what needs to be improved in this issue. please add your comments in detail which you found to be be implemented.
Comment #81
pfrenssen@RavindraSingh the needed work is described in #78, the issue was just set to 'Needs review' for the test bot.
Comment #84
aerozeppelin CreditAttribution: aerozeppelin at California State University San Bernardino commentedRerolled patch from #78. Added more tests.
Comment #86
amit.drupal CreditAttribution: amit.drupal as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commentedReview Patch #84.
I am little confuse for review patch.
After Apply Patch there is no changes in term Order of terms Weight.
Comment #87
jhedstromFrom the IS:
I cannot manually reproduce this via the UI on the latest 8.3.x. When I set a weight to 0 for more than one item, they automatically get distinct weights.
From
Drupal\taxonom\Form\OverviewTerms::submitForm()
:Comment #89
John Cook CreditAttribution: John Cook at CTI Digital commentedThis issue still exists on Drupal 7.
As such, I've started on a backport. The functionality has been ported, but the tests still need to be written.
Comment #90
John Cook CreditAttribution: John Cook at CTI Digital commentedSetting to needs review so the d7 port can be tested.
Comment #91
Alan D. CreditAttribution: Alan D. commentedPatch #89 helped on D7 but needs work (I think) as D7 needs to support PHP 5.2.... Anonymous functions are 5.3+ I believe, not that we have anyone on these insecure legacy PHP versions
Comment #94
lamp5Patch #89 works well on D7, thanks @John Cook.
Comment #97
nginex CreditAttribution: nginex at AnyforSoft for AnyforSoft commentedRe-roll #84 for 8.7.x
Comment #99
Daniel KulbeAfter I applied #97 to D8.9 I got 2 notices on save:
Notice: Undefined index: tid:21558:0 in Drupal\taxonomy\Form\OverviewTerms->buildForm() (line 257 of core/modules/taxonomy/src/Form/OverviewTerms.php).
Notice: Undefined index: depth in Drupal\taxonomy\Form\OverviewTerms->buildForm() (line 258 of core/modules/taxonomy/src/Form/OverviewTerms.php).
But the issue remains. The updated weights and parents are not applied to the terms.
Comment #100
danflanagan8This one looks related to #2928661: Reordering taxonomy terms after "Reset to alphabetical" yields incorrect results upon save.
Comment #104
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis does appear to be a duplicate of https://www.drupal.org/project/drupal/issues/2928661
Credit needs to be moved over but then this can be closed
Comment #106
quietone CreditAttribution: quietone as a volunteer commentedClosing as a duplicate, and moved credit over to #2928661: Reordering taxonomy terms after "Reset to alphabetical" yields incorrect results upon save. It is a later issue but there has been recent work over there.