Problem/Motivation

The class Drupal\Core\Config\Entity\ConfigEntityDependency contains some confusing code in its getDependencies() method:

  /**
   * Gets the configuration entity's dependencies of the supplied type.
   *
   * @param string $type
   *   The type of dependency to return. Either 'module', 'theme', 'config' or
   *   'content'.
   *
   * @return array
   *   The list of dependencies of the supplied type.
   */
  public function getDependencies($type) {
    $dependencies = [];
    if (isset($this->dependencies[$type])) {
      $dependencies = $this->dependencies[$type];
    }
    if ($type == 'module') {
      $dependencies[] = substr($this->name, 0, strpos($this->name, '.'));
    }
    return $dependencies;
  }

The lines of code around if ($type == 'module') need a code comment (see first comment on this issue for more discussion).

Steps to reproduce

Proposed resolution

Add a comment to the code (see comment #1 on this issue).

Remaining tasks

Make a patch.

User interface changes

No

API changes

No

Data model changes

No

Release notes snippet

No

CommentFileSizeAuthor
#3 3217861-3.patch1.15 KBjhodgdon
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon created an issue. See original summary.

jhodgdon’s picture

Title: Logic error in ConfigEntityDependency -- assumes modules are only config providers » Documentation needed in ConfigEntityDependency::getDependencies() to explain what the $type == module code is doing
Issue summary: View changes

Apparently I misunderstood this code. It is adding a dependency on the module that defines the config entity type, not the provider of the configuration entity item. Themes cannot define config entity types (according to alexpott in Slack discussion today). Profiles probably could but they probably don't and it might work to define them as "modules" anyway... the documented structure of config dependencies that is documented on ConfigDependencyManager does not include any space for dependencies on profiles -- which might or might not be correct? no idea).

Anyway, we definitely need a code comment here to explain what this is doing. Which is, quoting alexpott from Slack discussion:

What this is really about is not having to list the dependency of node for node.type.book (for example). It’s implicit because node provides the NodeType config entity. It has to depend on node. So adding

dependencies:
  module:
    - node

To every node type config export would be noisy & more prone to error.

jhodgdon’s picture

Status: Active » Needs review
FileSize
1.15 KB

Here's a patch.

alexpott’s picture

Category: Bug report » Task
Status: Needs review » Reviewed & tested by the community
Issue tags: +Documentation

This adds clarity so +1. And it is correct so...

AaronMcHale’s picture

+1 from me, explains clearly what is otherwise a confusing piece of code at first glance.

catch’s picture

Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!

  • catch committed 846675a on 9.3.x
    Issue #3217861 by jhodgdon: Documentation needed in...

  • catch committed 68e1d3d on 9.2.x
    Issue #3217861 by jhodgdon: Documentation needed in...
catch’s picture

Commit didn't go through... now it has.

AaronMcHale’s picture

Thanks everyone, small but very useful addition :)

Status: Fixed » Closed (fixed)

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