Regression: Moving out term children on term listing page is broken

Good catch

Hello

I've been digging in this issue and I think that the problem is related with parent field.

In line 370 of taxonomy.admin.inc file you can find:

  if ($values['term']['parent'] != $term->parent->value) {
    $term->parent->value = $values['term']['parent'];
    $changed_terms[$term->id()] = $term;
  }

Problem is that $term->parent->value is always empty, then when it try to compare, it doesn't update that value when level is root.

However, $term->weight->value is retrieved from DB without any problem.

I hope this can help.

I haven't tested to see if this is still the case, but if it's a regression from D7, that makes it a critical.

Hello

The bug persists.

After more digging, $term->parent->value is NULL by default for all the terms after entity_load(). Not sure if this is the expected behavior or not.

Attaching patch that fixes the bug, maybe someone could find a better approach if the parent value should be loaded.

Regards.

6: term-listing-2049121-6.patch queued for re-testing.

Debugging session after, I think we are next to the root of the problem...

taxonomy_get_tree function doc says:

Each term object is extended to have "depth" and "parents" attributes in addition to its normal ones

but code in OverviewTerms class expects the parents in $term->parent.

It worked until now because $term->parent is always NULL in this environment and comparison works for numeric values, but not for empty value when term is moved to the vocabulary root.

Now, I think there are wo options now:

• Change code in OverviewTerms class to expect $term->parents array instead of $term->parent property.
• Change taxonomy_get_tree logic to return $term->parent instead of $term->parents and modify all the code that currently expects the parents array.

Here, attaching patch modifying OverviewTest class

Here is the patch.
Not including test-only file given that is the same as #1 and #6.

Regards

Yeah, I broke this with the Entity Field API conversion. Fix looks ok to me, there are other issues to improve the parent handling, let's just fix the bug here.

Wondering if we should also check the parents through the API, with the taxonomy_get_parents() function? (or if that is a service now, that)

Hello

Are you suggesting to load parents using taxonomy_get_parents() instead of the parents property returned by taxonomy_get_tree()?

It could imply some db_queries which are maybe not necessary?

Maybe I'm missing something...

Regards.

Only in the test, to make sure that the returned parent information is correct.

You're only assertion right now is a assertNoPattern() on a rather arbitrary html tag, things like that often break when the HTML changes. Explicit assertions are almost always better than something implicit.

Oh sure!

I thought you were talking about the overview terms page logic :)

Here are test improved.

Regards

Thanks, that looks better. Would have been enough to just empty the static cache of that specific function, but it will be emptied anyway in tearDown().

Yes this looks great. Committed/pushed to 8.x, thanks!

The test may have failed but #17th patch worked at least until the fourth level of taxonomy hierarchy.