Problem/Motivation

The module confirm form does not show all the dependent modules that need to be installed for a requested module if the dependency was already shown for a module.

Steps to reproduce

Uninstall block, block content and navigation ( you'll need to uninstall layout builder etc)
Try installing navigation and block content.
Both depend on block but it only shows up in one list.

Proposed resolution

Show all the dependent modules and sort the list.

Remaining tasks

Review.

User interface changes

Before

Only local images are allowed.

After

Only local images are allowed.

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#3 before.png37.6 KBquietone

Issue fork drupal-3612458

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

quietone created an issue. See original summary.

quietone’s picture

Issue summary: View changes
StatusFileSize
new37.6 KB
quietone’s picture

quietone’s picture

Status: Active » Needs review
akshay kashyap’s picture

Thanks for the patch, the underlying fix looks correct to me. A few small
things before this is RTBC:

1. $checked = array_keys($modules['install']); followed by
in_array($dependency, $checked) does an O(n) scan for every dependency checked. Since $modules['install'] is already keyed by module name, it would be cheaper - and more consistent with the isset() checks used elsewhere in this method - to just copy the array and look up the key directly, e.g.:

  $original_install = $modules['install'];
     ...
     if (!isset($original_install[$dependency]) && ...)

2. It would help to add a short comment explaining why the snapshot needs to be taken before the loop starts. It's not obvious from the code alone, and without an explanation a future refactor could simplify this back to checking $modules['install'] directly and reintroduce the bug this patch is fixing.

3. There's a stray blank line before the closing brace of testModuleDependencyMessages() in DependencyTest.php, phpcs will likely flag it.

quietone’s picture

@akshay kashyap, thanks

1, 2: changes made
3: As far as I know, there isn't a Drupal coding standard about the line before the closing brace.

nicxvan’s picture

Title: Show all dependent modules on module confirm form » Show all dependent modules on module confirm form for each chosen module
Issue summary: View changes

I have a minor comment on grammar, but I'm confused by this, I just compared this branch and main.

I ran drush si then tried installing search node and Json response validator:
Main:

    You must install the following modules to install JSON:API Response Validator:
        JSON:API
        Serialization
    You must install the following module to install Search node:
        Search

Would you like to continue with the above? 

This branch:

    You must install the following modules to install JSON:API Response Validator:
        JSON:API
        Serialization
    You must install the following module to install Search node:
        Search

Would you like to continue with the above? 

What case is this not showing the right list?

Is it about showing dependencies in more than one list? I uninstalled many modules so I could install block content and navigation which both depend on block.
Main:

    You must install the following modules to install Block Content:
        Block
        Text
    You must install the following modules to install Navigation:
        Layout Builder
        Layout Discovery

Would you like to continue with the above? 

This branch:

    You must install the following modules to install Block Content:
        Block
        Text
    You must install the following modules to install Navigation:
        Block
        Layout Builder
        Layout Discovery

Would you like to continue with the above? 

That seems to be the case so I updated the title.

The before and after in the IS don't load, but I have the before and after text here so I think that's ok.

I also can't run the test only, but the test does look good, @quietone, if you could runt the test only that would be great.