Original issue: "make Cleanup page UI match system UI"
On the clean up page shown after disabling a module, the admin user is invited to disable possible orphaned dependencies or leave them.
This is done by checking modules and clicking 'Disable selected modules.'
This feels really counter-intuitive, as every else in Drupal core or features you check things you want to keep, not things you want to remove.
This page should match that -- keep the checkboxes but label the column 'Enabled' like on module admin, and change the button to 'save settings'. To make it clear what is going to happen, the boxes should probably be initially selected -- add a tableselect checkbox at the top to allow quickly deselecting all the modules listed.
New framing: "Remove orphan module detection"
It was decided that the functionality is fragile and buggy and not worth saving.
So instead of changing the UI, the functionality was removed.
Resolution, follow-up, alternative
The main functionality was removed.
However there is still a leftover function features_get_orphans()
, which is now unused (and also kinda defunct).
This will be dealt with in #3075533: Document features_get_orphans() as unused/deprecated, defunct and poisonous.
If you still need some tool to help you find out which modules are no longer needed:
Try https://github.com/donquixote/drupal-drux
Comment | File | Size | Author |
---|---|---|---|
#17 | features-remove_orphan_dependencies-719308-17.patch | 4.32 KB | vinmassaro |
Comments
Comment #1
yhahn CreditAttribution: yhahn commentedI'm not sure I agree with this - the action being taken here is "Turning off modules", and it's not just a page that "saves settings". It's actively taking a further step, and therefore I think the checkboxes should positively reflect that action.
Thoughts from anyone else? I'd love to know what the Sprocket guys think as they were the ones who initially requested this feature.
Comment #2
joachim CreditAttribution: joachim commentedBrainwave: change the button to 'Keep selected modules'.
It is an action, but having the UI visibly mean the reverse of what a checkbox + module name normally means results in the user having to do brain contortion to figure it out -- it's like negative statements on checkbox labels.
Comment #3
yhahn CreditAttribution: yhahn commentedAfter using this a couple times with your notes in mind I'm convinced. On my list.
Comment #4
q0rban CreditAttribution: q0rban commentedYeah, I'm not too happy with the way this works currently. I'm almost tempted to feature request that this feature get removed, now that I've seen it in action. :(
Comment #5
longwave+1 to either changing this to "Keep selected modules" or removing this entirely.
This "feature" of Features is still confusing even after having seen the page several times now.
Comment #6
joachim CreditAttribution: joachim commented> removing this entirely.
I think this is the way to go. This page keeps throwing up false negatives, as it seems to list *any* module that has no dependencies, even completely unrelated ones.
Comment #7
AaronBauman+1 to removing this entirely.
I was about to enter a ticket about always getting false negatives.
Comment #8
hefox CreditAttribution: hefox commented+1
- Confuses users
- Doesn't work well
- Extra code to maintain
Comment #9
hefox CreditAttribution: hefox commentedMarked #1047702: Cleaup proposes erroneous modules to disable as duplicate
Comment #10
Xen CreditAttribution: Xen commentedOK, I'll play the Devils Advocate here...
Features will enable needed modules for me, but by staying quiet when they're disabled, it'll just leave cruft behind. Shouldn't it at least tell me about it? And if it can tell me, it might as well offer to do it too (I don't want to have to visit the modules page afterwards).
It has an issue with false positives, granted, but shouldn't it be fixable by only taking the now un-depended modules that was actually dependencies of the feature just uninstalled? I thought it was supposed to do that.
The flipping of the checkboxes makes some sense.
Comment #11
redndahead CreditAttribution: redndahead commentedI don't see how features can ever make an accurate decision on what those modules are. For install profiles some of those modules may have been enabled from the profile and not from a feature. Some may have been manually enabled. I currently get offered to disable modules when enabling a feature. I even was offered to disable the feature I was enabling. While it has merits in theory in practice I don't think it quite works and is often more confusing than helpful.
I think at most it should just state that these modules were part of the feature you disabled and you may want to disable them also, but even that should be an optional message.
Comment #12
Xen CreditAttribution: Xen commentedWell, if it can't offer the right modules to disable, it can't tell you what they are either, as the code seems quite broken.
Anyway, I've taken a look now, and the major issue is that it doesn't take the features disabled into account at all. This seems fixable.
Comment #13
vinmassaro CreditAttribution: vinmassaro commentedHoping to revive this since some of my features are claiming there are orphaned dependencies when enabled, suggesting to disable one of my important base features which should NEVER be disabled.
Comment #14
vinmassaro CreditAttribution: vinmassaro commentedBumping this again because it is prompting users in our university installation to disable a base feature that MUST be enabled (contains CAS, LDAP and Varnish configurations). Thanks.
Comment #15
justindodge CreditAttribution: justindodge commented+1 to removing this feature entirely.
Even if you could get a perfect list of modules that have no current dependencies, that still doesn't mean a user will want to disable them - I just got prompted to disable date and entity api among many other modules that are fairly fundamentally important to my site. Even if I disabled the last feature module using a date field, that has no bearing on whether or not I want to continue using the date module.
In other words, I don't think your future usage of other modules is really much related to features to begin with.
Comment #16
ohthehugemanatee CreditAttribution: ohthehugemanatee commented+1 to removing this feature. Lots of other modules come with example Features (Flag and Rules come to mind), and leaving these features disabled means that you'll always get "orphaned dependencies" for those modules.
We want to encourage people to provide Features integration with their modules. We want to encourage people to provide example code. The current behavior punishes module creators for their "good" practices, by insisting on a poor user experience for their install base.
Alternatively, we could do a proper implementation: record when a Feature *which has previously been enabled* is disabled, and offer a tab in the Features interface that lists Orphaned Dependencies based on that list of disabled modules. Then we can display an admin message sitewide that prompts the administrator to review this page. We could also have a way for users to mark an orphaned dependency as "ignored" so it doesn't produce these messages in future.
But that sounds like a lot of work for something that is peripheral to Features at best.
Comment #17
vinmassaro CreditAttribution: vinmassaro commentedHere's a go at removing this. I'm not sure if the 'admin/structure/features/cleanup' item and callback are still necessary since it's really just flushing cache now.
Comment #18
mpotter CreditAttribution: mpotter commentedI'm all for this also. Anybody who wants this functionality added back to Features will need to submit a patch with a much better way of handling orphan detection. This was definitely causing more trouble than it was solving. Committed this removal to f77f1d8.
Comment #20
donquixote CreditAttribution: donquixote commentedWhile the functionality was removed, there is still the leftover defunct features_get_orphans().
See #1776738-33: Notice: Array to string conversion in features_get_orphans for some notes.
For BC reasons I will not remove this function. But I will mark it as deprecated and defunct.
Comment #21
donquixote CreditAttribution: donquixote commentedFor the record:
Anyone who wants to know which modules can be safely disabled:
https://github.com/donquixote/drupal-drux
Anything that is not directly or indirectly required by one of the "seed" modules (specified as cli parameters) can be safely disabled.
It is not specific to features, but also doesn't have to be.
Comment #22
donquixote CreditAttribution: donquixote commentedThis said:
Nowadays I usually have one main site feature with a list of dependencies (and a lot of other features of course).
To enable new modules on all environments, we run "drush en OUR_MAIN_FEATURE".
To disable modules on all environments, we use hook_update_N().
So we don't really use the drux thingie mentioned in the previous comment.
Comment #23
donquixote CreditAttribution: donquixote commentedComment #24
donquixote CreditAttribution: donquixote commented