System info can become inconsistent if hook_system_info_alter() makes a module required

Nice catch

The only nitpick I could have on this patch is maybe trying to loose the round brackets around the module name - they feel like they are a bit redundant, as well as on command line as in the UI.

swenteldoos-2:drupal swentel$ drush pmu datetime
comment is a required module and can't be uninstalled. Reason: Field type(s) in use - see Field list                                                                      
datetime is a required module and can't be uninstalled. Reason: Dependency of required module (comment)     

Yes, RTBC when green - probably tomorrow :)

Nicely found. I can't find any complains so committed/pushed to 8.x, thanks!

Not quite - needs backport to 7.x too.

I think it should be something like this.

Drupal 7 patch attached.

The node module is already required in Drupal 7, So i changed the test, to test it with the list module.

Patch attached.

RTBC, test looks good to me, fix too.

Made changes as in #2410151: _system_rebuild_module_data_ensure_required does not parse dependencies.

Patch attached.

Neither interdiff nor patch make sense to me, patch looks identical. Wrong patch?

Let's start again.

Created straight reroll.

In this patch I addressed the changes as in issue #2410151: _system_rebuild_module_data_ensure_required does not parse dependencies.

RTBC, thats better :). Thanks!

I tried this patch and don't understand why we'd want this.

On a fresh install of Drupal core using the standard install profile, I looked at the "required by" line for the Options module on the module administration page.

Before the patch, it looks like this:

Required by: Taxonomy (enabled), Forum (disabled), List (enabled)

After the patch, it looks like this:

Required by: Drupal (Dependency of required module Taxonomy), Taxonomy (enabled), Forum (disabled), List (enabled)

That is repetitive (taxonomy listed twice) and doesn't really make any sense.

Especially given the way that e.g. field_system_info_alter() uses the concept of a "required" module as a temporary thing rather than a permanent thing, I'm not sure what we gain by marking all these other modules as required too?

I was able to reproduce the bug involving Drush:

$ drush -y dis options
taxonomy is a required module and can't be disabled. Reason: Field type(s) in use - see Field list
The following extensions will be disabled: options, list
Do you really want to continue? (y/n): y
list was disabled successfully.
options was disabled successfully. 

So it shouldn't have let me disable that at all, but it did (and on top of that, it actually wound up disabling the Taxonomy module too, although it never said so on the screen).

But why is that a bug in Drupal core? If module A depends on module B and module A can't be disabled, why isn't Drush able to detect that module B shouldn't be disabled either?

Also a minor issue, if we wind up going ahead with this:

+; Depends on the Node module to test making a module required using
+; hook_system_info_alter() and ensuring that its dependencies also become
+; required.
+dependencies[] = list (>=7.x)

The code comment should say "List" rather than "Node".

Maybe @alexpott can clarify and help us decide what to do with this :)

Reset issue status.