Taxonomy term weight is not translatable, meaning you can't order term translations differently

Very valid point!

tagging for some upcoming collab days

I did a rough attempt to see how feasible this would be.
See patch in attachment as a starter. Currently no tests added, some existing might fail..

We are currently testing this locally on our multilingual project but at first sight this seems to work. I'm not so familiar with core entities & translations so review definitely needed.

Currently the adapted query in loadTree (TermStorage) will only be applied on multilingual sites. Additionally we want to make this configurable by adding a config option on the level of the taxonomy to allow/disallow weights per language. This is not yet included in the patch.

The reason why this query was adapted with a subquery and alternative condition is because we want to have a fallback in case a term was not (yet) translated. In this case we just fall back to the 'default'. The subquery prevents doubles (meaning: giving back as well the default_langcode term and it's translations - which is undesirable of course).

Rerolled for 9.3.x

> This makes it impossible, for example, to have the terms alphabetically sorted in each language.

That surely makes this a bug.

ran phpcbf --standard=Drupal core/modules/taxonomy/src/TermStorage.php this time, removed an empty line that the test runner complained about

+++ b/core/modules/taxonomy/src/TermStorage.php
@@ -230,11 +231,37 @@ public function loadTree($vid, $parent = 0, $max_depth = NULL, $load_entities =
+          // Subquery to prevent doubles in default_langcode fallback situation:

What's the impact of the subquery on performance?

Small change, but I needed to update the hook number to reflect the new D9 core version. We had another patch with a 9200 update hook so the update number didn't run. since we are on 9.3, i set it to 9301.

#17, performance cost of subquery:
good question. In what situations is the term storage loadTree method called? is this only for taxonomy term editing, browsing a vocabulary? or are there other common tasks that would invoke this method?

There was an issue with patch #18 when a site only has one language. The condition caused an infinite recursion. Patch #20 solves this along with a rewrite of the query replacing the subquery with a left join.

> good question. In what situations is the term storage loadTree method called?

Everywhere, by the looks of it! In forms, in the ForumManager, and also the TaxonomyIndexTid views filter.

Subqueries can usually be rewritten as a self-join, which has a much lesser performance cost. Setting to needs work based on that.

> Subqueries can usually be rewritten as a self-join, which has a much lesser performance cost. Setting to needs work based on that.

Uhu, that's what I did in #20... so needs review.

@kriboogh oops, sorry!

Reroll of patch 2931680-20 for version 9.1.4

Reroll for D9.3.0-beta2

I think this is RTBC.

After making a quick look into the issue summary and patch, there 3 points that core committers usually ask. Let's save their time and take care of that beforehand:
1) Although the issue summary is clear, it would be really great to use the default issue template to outline some important sections: https://www.drupal.org/community/contributor-guide/reference-information...
2) It looks to me that this is rather a new feature instead of a bug. I think we should also draft a change record, what do you think? If you insist it is a bug, let's describe why :)
3) Finally, we should add a test coverage including a test for the update hook as well.

I'm adding appropriate tags and moving it back to "Needs Work".
Thanks!

> If you insist it is a bug, let's describe why :)

It is a bug because on a multilingual site, taxonomy terms can't be put into alphabetical order in all languages.

> 3) Finally, we should add a test coverage including a test for the update hook as well.

I'm not sure it needs test coverage for the functionality. It's just a multilingual field. We don't have tests for the translatability of all base fields on core entities, but tests of the entity translation system in general.

I don't know about tests for the update hook though.

I tend to agree with @joachim

not sure what difference that makes though whether it's a bug or not other than a change request, I don't think this needs one though, it should just be fixed plain and simple.

An automated test should be fairly easy to put in place ,just take the current weight test, but with translation enabled
test translated term weights , change both translated and default and make sure that weights are different for the translated term weights than the default term weights

Added a test cases to validate the weight field translatibility.
Initially I tried to write kernel test, however I was not able to properly work it out.
Took to functional test approach for now.

Added a test to validate the database update hook as well. Both are passing on local

Oops, incorrect interdiff. Uploading a fresh one.
Please ignore this patch.

Added a test cases to validate the weight field translatibility.
Initially I tried to write kernel test, however I was not able to properly work it out.
Took to functional test approach for now.

Updated the correct interdiff.
Diff is taken from comment #25 only.

Removing the needs tests tag as they appear to have been added.

Still could use the IS update and change record so leaving the tags for those. Moving to NW for them.

Not super clear the issue

In Drupal 10 I added some terms to the tags vocabulary.
I'm able to have different orders per language.

uploading a tests_only patch to prove that the patch 37 is needed.

sorry this one is better

stright up reroll for 10.1.x

Is this issue summary clear enough?

We need now a change record? or can this be skipped because it's 'optional'?

I have able to Reproduce the issues in 11.x .
1) I have added dutch language (nl)
2) added same taxonomy terms as mentioned in IS.in english
3) added translation for two taxonomy term.
4) Changed weight in English and save . it is changing for both language.

applied this patch #37 , its fixed the issues and weight is changing according to language selected.
please review and let me know if you need MR for this.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

The patch and the automated tests have been reviewed however the core maintainers will likely have their say.

Please create a change record.

Other than the change record, I believe this is ready.

Added some nitpicky stuff, but the update hook version should be updated.

Appears to have a phpcs issue.

Issue summary was missing standard template, got it started but left TBD for sections I can't answer not being familiar with issue.

Agree on the need for a CR

Also agree this appears close though!

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Going out on a limb here, saying it's good.

There's a draft change request
There's passing tests
There's tests only fails

Looks good to me. Slam dunk?

We've been using a version of this solution for several years in harmony.

Left some questions/comments on the MR

Head of 11.x/10.2.x is broken right now so we'll have to re-trigger tests once that's fixed. I imagine that'll be fixed soon.
https://www.drupal.org/node/3060/qa
***EDIT***
nvm, look at gitlab ci instead:
https://git.drupalcode.org/issue/drupal-2931680/-/pipelines/87571

Looks like performance profiling tests show degraded performance when testing against Postgres 14.1 . Funny that the other DB systems tested do not have any performance regression.

    There were 3 failures:
    
    1)
    Drupal\Tests\standard\FunctionalJavascript\StandardPerformanceTest::testAnonymous
    Failed asserting that 67 is identical to 66.
    
    /builds/issue/drupal-2931680/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
    /builds/issue/drupal-2931680/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
    /builds/issue/drupal-2931680/core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php:58
    /builds/issue/drupal-2931680/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
    
    2)
    Drupal\Tests\standard\FunctionalJavascript\StandardPerformanceTest::testLogin
    Failed asserting that 41 is identical to 38.
    
    /builds/issue/drupal-2931680/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
    /builds/issue/drupal-2931680/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
    /builds/issue/drupal-2931680/core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php:108
    /builds/issue/drupal-2931680/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
    
    3)
    Drupal\Tests\standard\FunctionalJavascript\StandardPerformanceTest::testLoginBlock
    Failed asserting that 50 is identical to 47.
    
    /builds/issue/drupal-2931680/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
    /builds/issue/drupal-2931680/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
    /builds/issue/drupal-2931680/core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php:138
    /builds/issue/drupal-2931680/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
    
    FAILURES!
    Tests: 3, Assertions: 13, Failures: 3.