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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yhahn’s picture

I'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.

joachim’s picture

Brainwave: 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.

yhahn’s picture

Assigned: Unassigned » yhahn
Category: feature » task

After using this a couple times with your notes in mind I'm convinced. On my list.

q0rban’s picture

I'd love to know what the Sprocket guys think as they were the ones who initially requested this feature.

Yeah, 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. :(

longwave’s picture

+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.

joachim’s picture

> 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.

AaronBauman’s picture

+1 to removing this entirely.
I was about to enter a ticket about always getting false negatives.

hefox’s picture

Title: make Cleanup page UI match system UI » Remove orphan module detection
Category: task » feature

+1

- Confuses users
- Doesn't work well
- Extra code to maintain

hefox’s picture

Xen’s picture

OK, 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.

redndahead’s picture

I 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.

Xen’s picture

Well, 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.

vinmassaro’s picture

Hoping 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.

vinmassaro’s picture

Version: 6.x-1.x-dev » 7.x-2.x-dev
Priority: Normal » Major

Bumping 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.

justindodge’s picture

+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.

ohthehugemanatee’s picture

+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.

vinmassaro’s picture

Status: Active » Needs review
FileSize
4.32 KB

Here'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.

mpotter’s picture

Status: Needs review » Fixed

I'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.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

donquixote’s picture

While 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.

donquixote’s picture

For 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.

donquixote’s picture

This 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.

donquixote’s picture

Issue summary: View changes
donquixote’s picture

Issue summary: View changes