Simplify required module hiding in admin/build/modules
Dave Reid - October 10, 2008 - 18:22
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | normal |
| Assigned: | Dave Reid |
| Status: | closed |
Description
After looking into #318984: Unify all core package names to "Core", I was thinking about changing the following in system_modules in system.admin.inc:
<?php
if (in_array($filename, $modules_required)) {
$extra['required'] = TRUE;
}
?>to
<?php
if (in_array($filename, $modules_required)) {
continue;
}
?>which won't show the required core modules, and we wouldn't need to have two different core package names.
#1
Attached is the patch that changes all the core modules' package names to "Drupal core" and will not display the required modules. Removes the $extra['required'].
#2
I would like for #318984: Unify all core package names to "Core" to be committed with this issue, so I don't need to rename all the module packages. Revised patch with just the change to system.admin.inc.
#3
New patch with a *gasp* test to make sure that core required modules cannot be disabled! Yay! It does require #318984: Unify all core package names to "Core" since it checks for the package 'Drupal core' and not 'Core - required'.
#4
A note as why this patch removes the $extra['required'] handling form system_modules and _system_modules_build_row. At one point we were listing the core required modules on the admin/build/modules page, but Drupal is not able to verify required checkboxes are actually checked (#179932: Radio/checkboxes set to 'required' are not validated).
At a later point during the module page cleanup, it was changed to simply hide away the 'Core - required' fieldset (#229129: System module page *seriously* broken). Since we're following along the same path as hiding the required modules so there is no chance they can be disabled, this patch removes the required-checkbox handling code.
Summary so far:
- Removes obsolete required checkbox code in module page.
- Hides the modules specified by drupal_required_modules().
- Includes a test to make sure that required modules do not have a checkbox in the admin/build/modules page, and therefore cannot be disabled.
- Will marginally improve performance on module page since the element building/processing is skipped for the required modules (probably not big enough for me to do full ab testing).
#5
It works like it should. Be advised, don't apply before having applied #318984: Unify all core package names to "Core".
#6
No comment...
#7
Revised patch since we changed the core module package. Still RTBC.
#8
I like that this is removing some special-casing, but it also is retaining some, too.
Just a thought... what if we did this slightly differently?
Add a required = TRUE property to .info files, and let /any/ module declare themselves required and removed from the UI. This would be super handy on sites with a custom SiteName module that causes all hell to break loose if it's ever accidentally turned off.
If we did that, then we wouldn't need special casing for core, we'd just add required = TRUE to Filter, Node, etc. and the logic would treat all such modules the same.
Thoughts?
#9
Hmm... it would require a lot more looking into, based on the references to drupal_required_modules, especially during the install process. It should probably be in a separate patch. I can take that up.
#10
Ok cool. In the meantime, committed this, as it's a nice clean-up and seems to work. Yay!
#11
Well. Crap. I just saw #293223: Hide required core modules and it's effort to hide the required modules if they have HIDDEN = TRUE, which seems like an improvement since we can leverage the code currently being used to hide simpletest modules. Thoughts? Should this be part of a separate issue that removes drupal_required_modules? I'd prefer not to snuff out too many kittens if that page was too involved. I'm not going to mark this from fixed, but I'd like some input if anyone has some.
#12
Hm. Actually, I think drupal_required_modules() might still be needed for install profiles. But agreed! I like this even better. :) For some reason I only associate hidden = TRUE with testing modules that are only supposed to be there temporarily, but it does work equally well for required modules.
So I committed this too. Big yay for making this logic a tiny bit less convoluted. :) Bigger yay for tests that ensure it stays fixed! ;)
#13
What about drupal_required_modules() gathering information about required modules by parsing all info files? If you like it, I'll create a patch for this change.
#14
If we went there, we would need two flags: required and hidden. There are tons of SimpleTest modules that are hidden, but they are definitely NOT required, and in fact should NEVER be enabled outright. That was the whole point of the hidden property.
Might make sense for those to be semantically separate. The logic here would then become:
if ($module->hidden || $module->required) {
// Don't show it.
}
But.. not sure...
#15
I think it's best to use the info files for this. It's good for maintainability and readbility.
#16
No, I know. The question is whether we have TWO info file properties, required AND hidden, since both have the same effect in the UI.
Dave, what do you think?
#17
Filing separate issue in #320024: Replace hardcoded drupal_required_modules with parsing .info files for 'required = TRUE'. We're done here. :)
#18
Automatically closed -- issue fixed for two weeks with no activity.