Vocabulary hierarchy was always changed to single after term edit. This been fixed in #762604: Taxonomy terms list broken, term add/edit forgets it's parent
But we still has no test for "flatness" and this issue fixes it with more clear/readable condition in code

To reproduce:

  • Create a new vocabulary, on the term list page, you see "flat vocabulary"
  • Add any term, on the term list page, the vocabulary type changed to "single hierarchy"

Expected behavior: the vocabulary type should stay in "flat vocabulary" as long as there is no parent/child relationship in the vocabulary.

CommentFileSizeAuthor
#23 277200-vocab7.patch1.15 KBandypost
#19 vocab-hierarchy-277200-19-test.patch1.24 KBno_commit_credit
#18 277200-detect-hierachy-type-18.patch1.25 KBandypost
#15 277200-detect-hierachy-type.patch1.19 KBandypost
#15 277200-detect-hierachy-type-d7.patch1.15 KBandypost
#8 277200-detect-hierachy-type-d7.patch1.45 KBandypost
#7 277200-detect-hierachy-type-d7.patch1.45 KBandypost
#6 277200-detect-hierachy-type-d7.patch823 bytesandypost
#5 277200-detect-hierachy-type-d7.patch1.45 KBandypost
#1 277200-detect-hierachy-type.patch1.51 KBDamien Tournoud
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
FileSize
277200-detect-hierachy-type.patch1.51 KB

The issue comes from the identification of the root element. Here harmonized for consistency.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Active » Needs review

This is a patch. Could use a test.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Needs work

The last submitted patch failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Is this still an issue?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
FileSize
277200-detect-hierachy-type-d7.patch1.45 KB

Fix with test

EDIT: parent for [root] is not set #762604: Taxonomy terms list broken, term add/edit forgets it's parent

EDIT2: new issue about hierarchy #763380: Do not use \Drupal\taxonomy\TermStorageInterface::loadTree() in \Drupal\taxonomy\Form\OverviewTerms::buildForm()

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
277200-detect-hierachy-type-d7.patch823 bytes

Re-roll last patch suppose no need to wait debatable #763380: Do not use \Drupal\taxonomy\TermStorageInterface::loadTree() in \Drupal\taxonomy\Form\OverviewTerms::buildForm()

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
277200-detect-hierachy-type-d7.patch1.45 KB

taxonomy.module hunk was lost

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
277200-detect-hierachy-type-d7.patch1.45 KB

Re-roll against current HEAD

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Seems this was fixed in #846524: Terms cannot be manually reordered by weight.

So patch still useful
- Adds a test
- isset() a bit faster then array_shift() + compare

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs review » Closed (fixed)
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Closed (fixed) » Needs review

I think this should be commited

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

#8: 277200-detect-hierachy-type-d7.patch queued for re-testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Version: 7.x-dev » 8.x-dev

Sorry, I thought you said in #9 that the issue was fixed. Can we update the title and issue summary to reflect what the current patch is for?

Status: Needs review » Needs work

The last submitted patch, 277200-detect-hierachy-type-d7.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
FileSize
277200-detect-hierachy-type-d7.patch1.15 KB
277200-detect-hierachy-type.patch1.19 KB

re-roll for 7 & 8

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

#15: 277200-detect-hierachy-type.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 277200-detect-hierachy-type-d7.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
FileSize
277200-detect-hierachy-type-18.patch1.25 KB

Re-rolled patch with test

EDIT: summary updated

no_commit_credit’s picture

no_commit_credit CreditAttribution: no_commit_credit commented
FileSize
vocab-hierarchy-277200-19-test.patch1.24 KB

Attached merely removes t() from the assertion message, which is not actually translated. Reference: http://drupal.org/simpletest-tutorial-drupal7#t

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

Ah, I understand now. So the current patch does not include any functional changes. Instead, it includes:

  1. A code cleanup.
  2. An automated test to codify the correct behavior.

Patch looks good to me. Thanks @andypost for pushing this forward. :)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Title: Adding a term change the hierarchy type of the vocabulary to "single hierarchy" » Add tests for vocabulary hierarchy
Version: 8.x-dev » 7.x-dev
Category: bug » task
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks, committed/pushed to 8.x, moving to 7.x for backport.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Novice

Should be a simple backport.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Patch (to be ported) » Needs review
FileSize
277200-vocab7.patch1.15 KB
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs review » Reviewed & tested by the community

Thanks @andypost!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

That's certainly much easier to read. and yay for tests.

Committed and pushed to 7.x. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Novice, -Needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Updated issue summary.

Issue tags: +Novice, +Needs backport to D7

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.