TermStorage::loadChildren returned elements are not sorted by weight, name

let's try newer Drupal to gain attention .. then if needed backport to 9.

Create new MR from existing patches
+
- Add also name for sorting
- Add some tests for specific children weight and name.

Tests coverage looks good so removing that tag.

Can the issue summary be updated though with exactly what's being fixed and how? Current IS is mentioning D8 is everything still the same from then?

Also may want to update the MR for 11.x

Thanks!

- update summary
- move to 11.x for creating new MR.

new MR for 11.x available ... same changes/commits ...

Issue summary looks good.

Cleaning up tags and hiding patches.

Also verified tests fail without fix

Failed asserting that two strings are equal.
Expected :'000'
Actual :'eVWsOZrj'

Change looks good.

Triaging the RTBC queue.

I read the issue summary and the comments. Thank you @vasike for updating the IS, it really does make review easier.

I applied the diff and ran the test, looking at the output pages. The test is working as expected. I do wish there were some comments in the Test where the weights are added. It would make it just a bit easier for someone to see what is going on when they look at in sometime in the future. But then, this is no overly complex either.

I found two lines of unused core being removed. That is out of scope and should be done in a separate issue. I did go and look at the initial commit for it and I saw nothing there that indicates these lines were intended to further testing. (See the comments in the MR) Of course, it is in a for loop are well and constantly overwritten. I do think it is safe to remove them. The question remains if it should be done in this issue or a separate one. In this case, I think it is low risk of perpetuating an error and this is a bug report so I'd rather get this fix in without further work.

@quietone updates the comments ... i hope it's better ... at least

About removing/cleanup ... i see ... why not ... as the changes are in that place.

Otherwise ... i think we need a more generic issue for cleaning up the entire code ... maybe something about not allowing to have unused variables for "passing" MRs/patches.

Could we run the query on a real dataset, and check EXPLAIN before and after - would be good to know whether existing indexes handle this or not.

@catch i'm not sure what are the worries here
However those are the "explain results"

Before MR

EXPLAIN SELECT
    base_table.revision_id AS revision_id,
    base_table.tid AS tid
FROM
    taxonomy_term_data base_table
INNER JOIN taxonomy_term__parent taxonomy_term__parent ON
    taxonomy_term__parent.entity_id = base_table.tid
WHERE
    taxonomy_term__parent.parent_target_id = '1';

After MR

EXPLAIN SELECT
    base_table.tid AS tid
FROM
    taxonomy_term_data base_table
INNER JOIN taxonomy_term__parent taxonomy_term__parent ON
    taxonomy_term__parent.entity_id = base_table.tid
LEFT JOIN taxonomy_term_field_data taxonomy_term_field_data ON
    taxonomy_term_field_data.tid = base_table.tid
WHERE
    taxonomy_term__parent.parent_target_id = '1'
ORDER BY
    taxonomy_term_field_data.weight ASC,
    taxonomy_term_field_data.name ASC;

i hope it helps

So the worry is that the first EXPLAIN shows 'using index' and the second explain shows 'using index, using temporary, using filesort', with a large number of taxonomy terms this could cause a slow query.

So I think we should look at improving the indexes if possible, needs work for that.

@catch we won't get rid of those extra "using" if we need sorting on "SQL side"
However, maybe we could use a custom query ... instead of EntityQuery .... i think there is something already used on Views ...

Or we could do sorting at PHP level ... but would we have benefits?

Or am i missing something ...?

The MR below comment #25 fixed the issue for our use case.

Providing a patch for that

Just to note that there is a similar query in the TaxonomyIndexTid::valueForm(), see:

$query = \Drupal::entityQuery('taxonomy_term')
  ->accessCheck(TRUE)
  // @todo Sorting on vocabulary properties -
  //   https://www.drupal.org/node/1821274.
  ->sort('weight')
  ->sort('name')
  ->addTag('taxonomy_term_access');

It is without the parent condition, but there are other conditions used as well.