Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#3 | 3217861-3.patch | 1.15 KB | jhodgdon |
Comments
Comment #2
jhodgdonApparently 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:
Comment #3
jhodgdonHere's a patch.
Comment #4
alexpottThis adds clarity so +1. And it is correct so...
Comment #5
AaronMcHale+1 from me, explains clearly what is otherwise a confusing piece of code at first glance.
Comment #6
catchCommitted/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!
Comment #9
catchCommit didn't go through... now it has.
Comment #10
AaronMcHaleThanks everyone, small but very useful addition :)