I advised neclimdul to restart from Steven's comments because that issue got derailed soon. Things got a bit complex soon so I took over. Read that issue for more information but now it seems that things work right.

This patch presumes http://drupal.org/node/84546

CommentFileSizeAuthor
#11 dependencies_17.patch19.41 KBasimmonds
#10 dependencies_16.patch19.44 KBasimmonds
#8 dependencies_15.patch19.25 KBasimmonds
#6 dependencies_14.patch19.31 KBasimmonds
#3 dependencies.patch_13.txt19.16 KBneclimdul
#2 dependencies_2a.patch19.85 KBrstamm
#1 dependecies_2.patch18.43 KBchx
dependecies_1.patch18.2 KBchx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

chx CreditAttribution: chx commented
FileSize
dependecies_2.patch18.43 KB

Neclimdul observed that the confirm screen failed to preserve already switched on modules.

rstamm’s picture

rstamm CreditAttribution: rstamm commented
FileSize
dependencies_2a.patch19.85 KB

changed to nicer colors.

fixed handling of required modules in throttle column

based on #1

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
FileSize
dependencies.patch_13.txt19.16 KB

After some discussion with chx, we move the code from _validate to _submit and remove the global variable and use POST instead.

Also, I simplified the throttle code by reusing the disable processing to just disable the throttle on the core modules. I would like some feedback on the useability of this.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

adding to my queue

rstamm’s picture

rstamm CreditAttribution: rstamm commented

throttle column is consistent

looks very good

asimmonds’s picture

asimmonds CreditAttribution: asimmonds commented
FileSize
dependencies_14.patch19.31 KB

Found one little bug, the $current_module_list should be generated *after* the installation of new modules.

asimmonds’s picture

asimmonds CreditAttribution: asimmonds commented

If we have a module with a number of dependant modules, for this example I'll take views.
Views has the views_rss, views_theme_wizard and views_ui modules as dependants.

If views is enabled, then views_rss is enabled separately, the views module checkbox is correctly disabled. But now, if views_ui is enabled we will get a confirm page asking to enable views (but it's already enabled...).

asimmonds’s picture

asimmonds CreditAttribution: asimmonds commented
FileSize
dependencies_15.patch19.25 KB

In system_modules_submit(), should merge in the disabled_modules list before system_module_build_dependencies() is called. I've tested this a few times with different module combinations, hopefully haven't broken anything else.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs review » Reviewed & tested by the community

Wow, I'm very impressed! I was expecting to be able to poke all kinds of holes in this. ;)

1. Dependencies are listed before I even go to enable a module, so there are no "surprises" about needing the foo module.
2. Not only are they listed, it's also indicated whether the required module is enabled, disabled or missing. Colours are used for easy scanning, but the words are there too so we maintain accessibility.
3. Tested disabling comment module and enabling forum module -- the system comes up and prompts me that it will enable comment module for me, which works great. Also allows me to opt out of this.
4. I also renamed comment.module to comment.module2, and Forum module's checkbox gets greyed out, since comment.module is missing.
5. When a module is required by another module, the checkbox is greyed out so I can't cause dependency errors.

So yeah, I can't find anything to keep this from being RTBC, and I really tried, believe me. :)

asimmonds’s picture

asimmonds CreditAttribution: asimmonds commented
Status: Reviewed & tested by the community » Needs review
FileSize
dependencies_16.patch19.44 KB

Deselection of the checkboxes in the throttle column was broken.

asimmonds’s picture

asimmonds CreditAttribution: asimmonds commented
FileSize
dependencies_17.patch19.41 KB

Minor change

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

it is RTBC despite you can throttle a module that has a dependency. However, that's for a different patch because the correct way to do it is to move throttle to another tab IMO. It should not be possible to click for throttling a module which is not enabled therefore the throttle tab should only display the enabled modules. And it should be inverted, it should show which modules are enabled when throttle hits. The current is totally counterintuitive. Next issue or this continued.

But, this is feature complete and even fixes a core bug because the throttle uncheck is b0rken in vanilla HEAD, too.

chx’s picture

chx CreditAttribution: chx commented
Priority: Normal » Critical

Why critical? Because we agreed that 5.0 needs this, Dries made an exception for it and contribution modules can't use the fine functionality until it's in.

Steven’s picture

Steven CreditAttribution: Steven commented
Status: Reviewed & tested by the community » Fixed

Works swell. I tweaked the wording of the confirmation dialog box (the description and title were the same) and changed "Dependencies:" to "Depends on: " (to match "Required by: ").

Committed to 5.

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)