Reordering taxonomy terms after "Reset to alphabetical" yields incorrect results upon save

Hi daveferrara1,

I have tested the patch #2 manually and it seems to be working fine.

But a warning message occurs when we click the save button.
Warning Message: Warning: uasort() expects parameter 1 to be array, null given in taxonomy_overview_terms_submit() (line 449 of /var/www/html/drupal-contribution/modules/taxonomy/taxonomy.admin.inc).

Test case :
Created a new taxonomy term and tried to drag and sort some taxonomy terms and saved it. Then listed the terms in alphabetical order. After clicking save button previous sort appeared. After applying the patch it works fine. But warning message appears. When ever we are clicking the save button the warning message appears.

Please refer the attached screenshots.

I have debugged this issue, and found out what is causing the problem.

When the terms are reset to alphabetically, what taxonomy module does is, it resets all their weights to '0'. The actual alphabetical order is hardcoded into to taxonomy module.

Since all of the terms contain "0" as their weight, when uasort is used with drupal_sort_weight, the order returned by uasort is not predictable (but consistent it seems).

Attaching a patch that would use a custom sort function. This will use drupal_sort_weight when weight is supplied, otherwise will try to use the taxonomy "name" to sort.

Patch is applied to 7.x-dev.

After applying the patch, you may review this patch as follows,

1. Create a new vocabulary called TEST
2. Add 5 terms called A, B, C, D, and E.
3. In the Taxonomy overview interface, i.e. the place where the terms are listed in a table, show the row weights too. This will help a lot in understanding what is going on behind the scenes.
4. Follow the procedure that is listed in the main issue description and you can confirm the order is maintained after last save operation.

Hi D34dMan,

I have tested the patch #9 manually. There is an issue after clicking the save button.

I have created the taxonomy terms with upper and lower case.
When clicking Reset to alphabetical button, the terms are set to alphabetical order but again after clicking the Save button alphabetical order changes like upper cases are sorted first and then the lower cases.

Please refer the attached screenshots.

Nice catch @sarguna raj.

I have modified the taxonomy_sort_weight function to use case insensitive comparison, and if the strings are equal, then use tid to sort them. Hopefully this is as close to the database OrderBy statement.

Changing the status to Needs review.

@D34dMan

I have tested the patch #13 and everything works fine related to the taxonomy.
Tested the above issues and also the taxonomy terms with integer value.

Please refer the attached screenshots.

Kindly find the updated attached screenshot.

There seems to be a very similar issue in Drupal 8.6.0-rc1. After doing a reset to alphabetical, it's very difficult to manually sort the terms in a list. They keep reordering in a seemingly random fashion for multiple submits of the form. The only way I've found to clearly fix it through the UI is to use the the Show Row Weights button and adjust them manually to the sort order that you're trying to accomplish.

Probably would be logical to detect a reset and skip sorting. Please review a proposed patch that we are using to address the issue on our systems.

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -510,7 +510,10 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
+    if (!$this->isOrderReset($terms)) {

What is this detecting?

If it's trying to figure out if the alphabetical sort button has just been pressed to submit the form, then we should check the form state.

If this is for a later form submission, then why does alphabetical sorting set all the weights to zero?

Ok, so weights at 0 is the default when new terms are created.

So everything in the DB is correct, and this is a form data problem.

On Drupal 8.9.x, only security issues are fixed.

The tests are really lacking for the term re-ordering form. The test TermTest:testTermReorder() incorrectly assumes that the three test terms are named term1, term2, and term3.

// Fetch the created terms in the default alphabetical order, i.e. term1
// precedes term2 alphabetically, and term2 precedes term3.

The three test terms in fact have randomly generated theme names. So none of the assertions in that test related to alphabetizing are especially meaningful. The test terms should have hardcoded names to make alphabetization tested more clearly. (Update: I think I understand that comment now. term1 does not necessarily correspond to tid = 1. But the clarity is lacking unless there are hardcoded names that can clearly be alphabetized. We also shouldn't depend on the fetch returning terms in alphabetical if we don't have to.)

And according to @attheshow in #17,

The only way I've found to clearly fix it through the UI is to use the the Show Row Weights button and adjust them manually to the sort order that you're trying to accomplish.

The TermTest:testTermReorder() test doesn't drag terms; it submits explicit weights. I think that whatever test we write for this should actually drag the terms. That might require a FunctionalJavascript test.

Here's an attempt add adding clearer test coverage for dragging terms around the overview page. The test does this:

1. Make a vocab
2. Make some terms
3. Drag the terms around and save
4. Reset to alphabetical order and confirm
5. Click save without dragging anything

And then for some reason the terms are out of order!

I think it would be worth adding additional steps that confirm the form can do nesting and then reorder after nesting and stuff like that, but that may beyond the scope of this issue. I'm not sure what the process is for adding missing test coverage in cases where there isn't a bug directly associated with the missing coverage. But I am of the mind that TermTest:testTermReorder() as it stands is insufficient coverage for the overview form. Even the comment is a copy-paste typo from the preceding test!

/**
  * Save, edit and delete a term using the user interface.
  */

The overview form testing needs some love.

I realized that I mixed up expected/actual values in that test. I'm posting a new patch where I fix that error. I also added a test class that nests terms by dragging and moves them around and alphabetizes.

Once again, I don't know if adding these tests is overkill for this particular issue, but I think we need to get tests like this into the codebase somehow.

Ok, so weights at 0 is the default when new terms are created.
So everything in the DB is correct, and this is a form data problem.

Agreed, it is a form data problem. A form data problem, where the form doesn't render the terms based on how taxonomy_get_tree/TermStorage::loadTree method defines the order to be like.

More info:

Point to be noted is that, the user's intend is respected. Which means, even thought the taxonomy form shows the term in broken order, the taxonomy_get_tree/TermStorage::loadTree will load the terms in correct order.

The taxonomy_get_tree and TermStorage::loadTree methods uses the following query to sort by weight,

      ->orderBy('t.weight')
      ->orderBy('t.name')
      ->execute();

From the above SQL query, it is clear that, if all the terms have same weight, then they are ordered by "name" column (adhering to SQL ordering rules in case multiple terms of same name exists).

This is interesting, because, reliance on render array, which uses "#weight" drupal_sort_weight or SortArray::sortByWeightProperty alone is not sufficient in determining the order. And hence we have a buggy experience when terms have same weight.

I think I have a fix for the issue. The taxonomy storage cache does not get reset after alphabetizing. The TermStorage::resetWeights() does a straight-up db operation so there's no cache invalidation of any kind from that. I just added a cache reset to the alphabetize form. Maybe it should be part of the TermStorage::resetWeights() function, but I don't think that's called anywhere else so it doesn't matter much.

I also bulked up the fail test once a gain to demonstrate that after alphabetizing, the form reloads in the right order visually but with the wrong weights set.

Removing "Needs tests"

The issue summary should be updated. The description of the bug and the steps to reproduce it can be improved.

Patch in #30 applies to Drupal 9.3.x and resolves the issue as described. Tests have been added and enhanced. The summary is up-to-date and accurate. Marking RTBC.

Setting back to RTBC. Looks like test failures are unrelated.

Unrelated random fail. Resetting status.

Putting back to RTBC seems random failures

This is retesting on a 9.2. Can someone get it testing on 9.5 and/or 10?

Thanks

@quietone the patch #30 still applies to 9.5

As the test passed for 9.5.x, verified patch #30 on 9.5.x.
The patch was applied successfully.

Steps followed:
1. Create vocab
2. Create terms (with upper/lowercase)
3. Drag to the terms > Save
4. Reset to alphabetical order and confirm
5. Save without dragging any term
6. Again drag the term to top > Save
7. Ensure the dragged term is getting saved

It is working fine.
Please find attached the videos.
Can be moved to RTBC

I think we need to ensure that the same cache tags are clears as would occur on a taxonomy term save happen and I think to be consistent with term saving this needs to occur inside the \Drupal\taxonomy\TermStorageInterface::resetWeights() implementation - so it doesn't matter where it is called from. The duplicate issue does some of this but not all of this - see #2956594: Taxonomy "Reset to alphabetical" doesn't clear cache of entities. I'm going to close that one as dupe and credit people on that issue here.

Crediting wengerk, Mschudders, gido, Vj, kriboogh, Asacolips, tonifisler, alexrayu, borisson_, Krzysztof Domański, catch from the dupe - #2956594: Taxonomy "Reset to alphabetical" doesn't clear cache of entities

I can reroll this patch, but I want to be sure I understand the last comment.

This needs to occur inside the \Drupal\taxonomy\TermStorageInterface::resetWeights() implementation...
The duplicate issue does some of this but not all of this.

Does that mean that cache tag invalidation should go in /taxonomy/src/TermStorage.php instead of VocabularyResetForm.php? Are there are other pieces that are still missing?

And, should the tests from both issues be combined? Or is one of them better than the other?

Here's a patch in the following comment #64 based on assumptions from my previous comment. (Ignore the patch in this comment.)

I moved the code from the duplicate issue, comment #52 into TermStorage as noted above in this issue, comment #49.
I included tests from both this issue (#30) and the duplicate (#52).

I tested the patch on a fresh 10.0.0 install, using the steps to reproduce in the Issue Summary.
I also tested the patch on 9.5.1-dev using DrupalPod.

Corrected the path in the patch file.

+++ b/core/modules/taxonomy/src/TermStorage.php
@@ -334,6 +335,20 @@ public function resetWeights($vid) {
+    $termIds = $entityQuery->execute();

Code style: both these should be snake_case

+++ b/core/modules/taxonomy/src/TermStorage.php
@@ -334,6 +335,20 @@ public function resetWeights($vid) {
+      ->accessCheck(TRUE)

Access check should be FALSE.

Fixed command failures of #64.
Please review.

No, please fix #66 before asking for a review.

Patch #68 updated with requirements in #66.
Thanks

My bad, ignore #70.
Here is the correct one.

Fixed CCF in #71

I was able to reproduce this issue on 10.1.x using the steps in the summary, the patch provided in #72 worked for me and did fix the issue with the cache.

I have tried to fix the CI issues with the above patch

+++ b/core/modules/taxonomy/src/TermStorage.php
@@ -338,6 +339,22 @@ public function resetWeights($vid) {
+    $entityTypeKey = $this->getEntityType()->getKey('bundle');

Another variable with the wrong case.

Also, should we also invalidate the ENTITY_TYPE_list and ENTITY_TYPE_list:BUNDLE cache tags?

I opened a merge request to address the snake case comment as noted in #76.

Regarding the question in #76, I don't have the answer or an example of when this would be necessary--if this should also invalidate ENTITY_TYPE_list and ENTITY_TYPE_list:BUNDLE cache tags. Can anyone think of a use case where we'd need this?

I updated the IS to include Steps that demonstrate what Cache::invalidateTags($term_ids); fixes. I also updated the Remaining tasks to include some of the questions in the issue queue that are pending answers.

I'm not sure what the most appropriate status is now. I updated the patch (MR), but should this issue remain Needs Work until the questions are answered?

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

Removing credit from myself for the rebase, as I was making sure the tests pass. Will let committers decide to add me back or not.

Tested the MR following the steps from the issue summary in scenario 1 and this does seem to solve the problem.

Moving back to NW for the remaining tasks in the issue summary update. If they've been completed could you mark them please.

Thanks and good work!

Closed #941266: Order of terms with same weight messed up after saving as a duplicate and moving credit.

MR !3209 is for 10.1
MR !9044 is for 11.x

MR !9067 is for 10.3

Patches added

Left comments on MR

FYI would recommend hiding all non 11.x branches

Made updates per MR comments.

(Sorry for all the branch visibility toggling.)

Thanks for the quick response. Still appears to be a few dozen out of scope spacing changes. Nitpicky stuff but should be reverted.

I removed the rest of the extra spacing.

https://git.drupalcode.org/issue/drupal-2928661/-/jobs/2418794 shows the test coverage

Applied 1 missing return void.

Believe rest of feedback has been addressed.

There are several questions in the 'remaining tasks' that I can't find a clear answer to. Can someone check that and either answer them or provide a link to the answer?

I read the MR and mostly concentrated on the comments and noticed two things, one that many are wrapped incorrectly and usages if "Helper to" in a method summary line. The phrase 'Helper to' (which is here and there in core) does not add anything useful to the summary line and it would be better to simply state what the function does. I have changed the MR to wrap the comments correctly and remove use of 'helper ..', plus a few other tweaks in the comments. It was simpler to do that than make many suggestions in the MR.

So, we just need answers to those questions to be sure that everything is covered here.

Our editorial team has a new search view that works with the taxonomy term list order and facets. This issue was making it impossible for them to predictably order the items and have them reflected in the order of the view’s facet filters.

We are running 11.3 and the current state of the patch does not apply due to the tests, so I made a smaller patch with the fix. The fix does to solve our issue.

I am noticing that after the patch is applied, while resetting the terms to alphabetical order works, unless the page is saved, all the term weights are zero. Maybe this is expected behavior, but it is not ideal.

Posting the patch here, in case anyone else experiencing this issue needs a solution before the merge happens.