-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Drupal Dependencies commands to Drush core #6060
Conversation
aed0bf2
to
63b4944
Compare
Blocked on #6061 |
Unblocking. Resolving w/o #6061 for now |
8956987
to
2c0ecdf
Compare
b8bda10
to
8956987
Compare
9dff50c
to
3080418
Compare
Ok, let's see what Drush maintainers have to say. In the future we can expand with more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on the tests. I have just nits.
docs/drupal-dependencies.md
Outdated
|
||
Drupal modules are able to define other modules as dependencies, using the module's [metadata info.yml file](https://www.drupal.org/docs/develop/creating-modules/let-drupal-know-about-your-module-with-an-infoyml-file). To get all modules that depend on a given module type: | ||
|
||
drush why:module node --dependent-type=module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just --type? Its briefer, and easier to spell.
private array $dependents = []; | ||
private array $tree = []; | ||
private array $relation = []; | ||
private array $dependencies = [ | ||
'module-module' => [], | ||
'config-module' => [], | ||
'config-config' => [], | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHPDoc for these would be helpful IMO
/** | ||
* @param string $dependency | ||
* @param array $path | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets do a 1 line summary of the method, and omit phpdoc on params if they add nothing above whats in the function signature. Same for methods below.
{ | ||
$entityTypeManager = \Drupal::entityTypeManager(); | ||
$configTypeIds = array_keys( | ||
array_filter($entityTypeManager->getDefinitions(), function (EntityTypeInterface $entityType): bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you improve readability with an arrow function here? https://medium.com/@albertcolom/how-to-use-arrow-function-in-php-c28490ff7fb7
{ | ||
return new self( | ||
$container->get('extension.list.module'), | ||
$container->getParameter('container.modules') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This container param I wanted to autowire, so I've opened #6061
Ready for a 2nd round of review |
This looks great to me. I will merge in a day or two unless we get more discussion here. |
Add https://github.com/claudiu-cristea/drupal-dependencies to Drupal core