Problem/Motivation

Currently it is not possible to disallow terms to have children. It would be really interesting, though.

Proposed resolution

Allow to set "Maximum ancestor depth" to 0 and act accordingly.

Comments

DuaelFr created an issue. See original summary.

duaelfr’s picture

Status: Active » Needs review
StatusFileSize
new6.18 KB

Here is the patch.
You might want to extend test coverage :)

duaelfr’s picture

StatusFileSize
new601 bytes
new6.28 KB

I accidentally reversed a condition and I forgot to hide the parent field in the term edit form.

dmitriy.trt’s picture

Status: Needs review » Needs work

Thanks a lot for working on this feature! It would be awesome to add this into the first stable release! A few points that should be improved on the patch are below.

1) Tests for existing code fail with the patch and they should be fixed. Functional tests should be written for this feature as well, probably it is possible to just add them to existing ones.

2) It would be very nice to keep the same interfaces and meaning of the values they return. For example:

  /**
   * Returns max ancestor depth set for the vocabulary.
   *
   * @param string|\Drupal\taxonomy\VocabularyInterface $vocabulary
   *   The vocabulary or its ID.
   *
   * @return int|null
   *   Max ancestor depth or NULL if it's not set on the vocabulary.
   */
  public function getMaxAncestorDepth($vocabulary);

With the patch applied, we're gonna break this interface, as meaning of the return value changes. Could we instead keep it as it is now? NULL could mean no limit and 0 could mean that hierarchy must be completely disabled. This way you don't need constant for a special unlimited value and still could distinguish them using isset($max_depth) and $max_depth === 0. We could use empty string as unlimited value on the vocabulary form where user edits the limit.

3) When you need to reference a class constant from a different class, it's nice to define a constant in the class that's gonna use it, reference the original one and then use it as static::CONSTANT_NAME. This way it's really easy for a child class to change the constant value. But if you reference it directly as OtherClass::CONSTANT_NAME, then it's not possible in a child class to override it.

For example, this one doesn't allow us to change the constant value in a child class:

class TermOverviewFormAlterer extends FormAltererBase {
   ...

   if ($max_depth === VocabularySettingsReader::MAX_DEPTH_UNLIMITED) {
     return;
   }
}

But this one does:

class TermOverviewFormAlterer extends FormAltererBase {
  
  const MAX_DEPTH_UNLIMITED = VocabularySettingsReader::MAX_DEPTH_UNLIMITED;

   ...

   if ($max_depth === static::MAX_DEPTH_UNLIMITED) {
     return;
   }
}
dmitriy.trt’s picture

Issue tags: +Needs tests

  • Dmitriy.trt committed 1476536 on 8.x-1.x authored by DuaelFr
    Issue #3092689 by DuaelFr, Dmitriy.trt: Allow to disable hierarchy
    
dmitriy.trt’s picture

Status: Needs work » Fixed
Issue tags: -Needs tests

Committed with changes.

duaelfr’s picture

Thanks!
I'm sorry I wasn't able to follow up on this. I'm glad you had more time than me.

Status: Fixed » Closed (fixed)

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