Include modules that aren't enabled

That's a great idea. It's a relatively simple solution to the issue of how to let this module work properly for multi-site installs: http://drupal.org/node/125055.

Basically, all we need is another set of radio buttons on the settings page for which modules to check for (enabled vs. all), a relatively simple change to the logic for finding what modules to query for, and a little bit of visual indication of enabled vs. disabled modules on the update status report.

Subscribing.

I have a site that for various reasons has a few modules that were uploaded but not enabled. It would be great for Update Status to give me an update on all modules, regardless of whether they are enabled, so that I don't inadvertently turn on an out of date module and only find that out after enabling it.

Subscribing.. this is a very much needed feature both in Drupal 5 and in Drupal 6.

+1 on that. managing the modules for dozens of sites gets pretty complicated. eventually, there needs to be something that can show you which sites are currently running which modules, but for now, just being able to see the update status for all modules in a drupal installation would be a great help.

that would need to include /sites/all/modules but also /sites/eachsitename.org/modules

updating version for this request as the feature is not implemented in the latest stable release.

This is still an issue for Drupal 6!

I would even categorize this as a bug, a normal one though. As a feature request it wouldn't be done for D6 anyway.

Please don't abuse categories, just because you hope someone else will work on it. It's not a bug. It's working correctly, exactly as designed. It's just not an ideal design, perhaps. At best you could call this a task, but it's really a new feature. ;)

Almost 5 months ago I laid out what needed to happen here, and no one worked on it. I can't drop everything and work on every new feature request that comes in for the modules I (co)maintain. Yes, this would be nice, but in 5 months if no one else wanted to even take a stab at a patch, I'm not going to go nuts trying to push this into D6 core at this late stage. If I was going to mount a campaign to get something else changed at this point, it'd be this: http://groups.drupal.org/node/7440

Seems like it's way too late to implement this whole feature in D6. However, if we commit the attached patch, it'll be pretty easy to solve this in http://drupal.org/project/update_advanced in D6 contrib.

So this latest patch just makes these two categories available not making them displayed somewhere, right?

dww: While I might have mis-categorized this as bug, that doesn't mean I would abuse categories! That, on the other side, all of us are having this slight bias to mark something as a bug, should be perfectly understandable during a feature freeze.

Your idea to solve this in update_advanced is great, anyway. If Gábor's assumption (in #9) is right, let's do it this way! +1

@Gabor: basically, yes. If you study theme_update_status(), you'll see that each project has a 'project_type' attribute associated with it. theme_update_status() figures out what to display for each project, and groups things by project_type. That's why you see core first, then modules, and finally themes. This patch just adds 2 other types of projects that theme_update_status() knows about grouping together. So, it'll be up to update_advanced in contrib to add the disabled modules and themes to the data structure of project information, but once it does, theme_update_status() will print out the information at the right spot. Notice that a few lines down (still in the context of the patch, in fact), theme_update_status() does this:

So, by default, since there won't be any entries for these two project types in the $data array, nothing will be displayed. However, if update_advanced populates $data with some projects of either type, they'll get printed.

Sound good?

Thanks,
-Derek

p.s. @pancho: Yes, believe me I'm well aware of the temptation to call everything a bug during a code freeze. That's still abusing the category, though. ;) Mind you, it's not malicious, and I'm not angry, but the core committers have enough grief as it is.

The patch looks good to me.

Thanks for the explanation, committed #8.

Thanks! Now we just have to implement the functionality. ;)

So what is the status now? I see that patch of #8 is already incorporated in modules/update.

Is it being used in "Update status advanced settings" ?

No. There's no code in update_advanced to take advantage of #8 yet. Hence, "active" and "unassigned".

So… very… itchy…

… ahhhh.

Here’s the patch.

FYI, this would have been a much simpler patch, but there is no HOOK() to modify the project cache and no HOOK() when fetching project release data.

Definitely would love to have this! Will try to test soon.

Any (at least a snowflake in hell) chance to do a backport for D5? There still are so many of us stuck with D5 and checking for updates of non-enabled modules is so extremely desirable.

@JohnAlbin: I suggest you take a look over at #238950: Reduce RAM resource consumption. It's possible I'll be able to refactor some of this code in D6 to make it more per-project friendly. That would greatly simplify this code, too. /me crosses fingers...

@vacilando: There's a chance, but it depends on someone either doing the work or sponsoring my time. I'm not really interested in putting much more effort into the D5 version of update_status -- I've got far too many other things I need to work on.

Forgot to mention...

A) @JohnAlbin: THANKS! ;)

B) I don't think it's wise to commit this to update_advanced until we know what's going on with #238950: Reduce RAM resource consumption. If the decision over there is "won't fix" for D6 and that gets bumped to D7, we can just commit this (after a little more review/testing). If #238950 is going to fly in D6, I'd rather work on that first (and keep this patch in mind while we do it) to make sure update.module itself will expose the right stuff to solve this problem more easily. Therefore, I think this should really be postponed pending at least a decision about #238950 from the core committers, if not until that's actually done and committed to update.module.

Sound good, John?

Thanks,
-Derek

Sounds good to me!

Oh, man, PLEASE yes.

I have a multisite installation and in order to make sure that all modules are up to date I have to have one of the sites with EVERY MODULE ENABLED. The modules page is very slow to load and the rest of the site is littered with the junk of dozens of rarely enabled modules.

UPDATE: Ran the patch and it works perfectly, THANK YOU!

Is this going to affect the Drupal usage statistics? http://drupal.org/project/usage

It might be necessary to also change how the data is collected so the statistics are not artificially inflated by disabled modules.

It won't be a problem. We can just not include the site_key parameter in the URL for the projects with only disabled modules, and the usage stats won't record it.

Does anyone know if this patch will still work on the 6.x-1.0 release?

re #20 & #21: looks like #238950: Reduce RAM resource consumption is insignificant after all. See comment #51 on that issue.

This module is "so close but yet so far." Frustrating because this could be quite useful.

@Aren: "This module is "so close but yet so far." Frustrating because this could be quite useful."

Feel free to contribute patches or sponsor my time to work on them. This module is *very* low on my priority list, since it doesn't pay the rent (and these days, I can't afford to do too much free labor)...

I took a stab at this, and it is amazingly easy if we can run a drupal_alter() from within update_get_projects() to add the disabled projects. Otherwise we have to re-implement fetching and comparing, as well as the fact that the disabled module update data will not be stored in cache and have to be re-fetched every time. There is almost little to no work for update_advanced but will require us to move this issue back to core once more to add the new hook and make sure that we don't include the site key when requesting info for disabled modules.

I've even attached the screenshot of my admin/reports/updates page...including all the update data on my disabled modules and themes! Yes, it's a long screenshot and you'll have to scroll. :)

Forgot to add the checkbox to turn this into a setting that is disabled by default.

Thought this would get a few comments sooner. Does adding a update_projects_alter() into core seem appropriate and this approach valid? I'd like to be able to move this to D7 soon since it will need to be backported *again* and then we can finally fix this.

Please help go review #445748: Add hook_update_projects_alter()!

#445748: Add hook_update_projects_alter() is now in both D7 and D6. Woot! ;)

So, I took a look at this and I found a few UI issues:

A) If you've already fetched for available update data, turn on the setting, then visit the available updates report itself, none of the disabled modules have any release data and they all show "No available releases found" warnings. We should probably see if this setting is changing in the submit handler for the settings page, and clear the cache of available update data if the setting changes, so that we force a re-fetch in this case...

B) Once you turn on the setting, the "Includes" lines in each project includes every module included in that project, regardless of if they're enabled or not. See attached screenshots for an example. On my test site, I've only got project and project_release turned on. But, once the new setting is enabled, the row for "Project" up in the main "Modules" section (not "Disabled modules") includes all the sub-modules with project, even the ones that are disabled. That seems misleading and weird. Not sure the best approach just yet, but wanted to raise it for feedback.

Otherwise, this is pretty slick. The D7 patch for this feature in core would be even smaller, since we could just patch _update_process_info_list() directly. It'd probably be a 10 line patch (not including the checkbox UI itself in update.settings.inc)...

Fixes (A) and (B). Tested and working. Any other reviews/tests before I commit? Thanks!

Look good, as long as this is still tracked for D7 core.

@Aren: I just asked webchick about getting this in D7 core in IRC earlier today. She said "we'll see"... The first step is to get this done and happy and working in D6 contrib. Then, we'll see how bad the D7 core patch looks. She was concerned about the help text for this checkbox, explaining why you'd want to check for updates on disabled modules and themes, etc, etc. So, we'll have to see. But yeah, either it'll be in D7 core, or it'll be handled as part of #454368: Port update_advanced to D7.

Also, what does "Look good" actually mean? Did you test this? Did you closely review the patch? Did you just skim the patch for obvious code style problems? What? Reviews need details to be helpful...

Thanks,
-Derek

Sorry for the lack of clarity. I just now installed the patch. Here's what I noticed:

• Syntax error on line 176 of update_advanced.settings.inc. Remove the last closing parenthesis character.
• If I select Check for updates of disabled modules and themes and hit Save configuration, the list of modules disappears from admin/reports/updates/settings, and the checkbox appears above Save configuration on page refresh. If I leave the page and navigate bcak, the modules appear again. Are you using the Drupal-approved way to modify the UI?
• admin/reports/updates and admin/reports/updates/settings show no disabled modules.

Yeah, I see "Look good" doesn't mean much, since a syntax error snuck in from when I tested and when I ran the cvs diff to generate patch #34. ;)

New patch for D6, and a fully functional backport for D5 update_status contrib. This is an amazingly small backport, considering that D5 contrib has no notion of $project_type, none of the existing support in D6 core for disable-module, etc, etc. ;)

@Aren: sorry for the cross post:

* syntax error already fixed in #38

* That weirdness is actually due to a separate bug: #237347-14: Table goes hidden on save settings -- yes, I'm using the "Drupal-approved way to modify the UI", the problem is that the form builder is trying to use some cached data to generate that form, and when you turn on this setting, we intentionally clear the cache (see #33 B).

* Once the setting is on, I'm definitely seeing the disabled modules at admin/reports/updates. Are you sure your test site has any disabled modules sitting in sites/all/modules?

Here's a re-roll of #38 now that #237347-15: Table goes hidden on save settings is fixed.

Reroll of both patches so that the settings to ignore certain projects honors the checkbox, too, and lets you ignore individual disabled modules (and in D6, themes).

Untested, but I'm pretty sure this is all we need in D7 core. ;)

YAY!!! I recompiled everything on my laptop, and now have a PDO-enabled version of PHP running, so I could finally install and test HEAD again. ;)

#42 is almost all we need, and mostly works great once I found/fixed #490540: D7 cvs_deploy broken by #380064 (filename vs. filepath). ;) There was just a minor bug with how it was grouping core modules and themes together. Now fixed.

$ patch update_advanced.module 162788-41.update_check_disabled.d6.patch
patching file update_advanced.module
patching file update_advanced.settings.inc
Hunk #1 FAILED at 11.
1 out of 4 hunks FAILED -- saving rejects to file update_advanced.settings.inc.rej

Tried against fresh 6.x-1.0 files.

@Aren Cambre: Re: "Tried against fresh 6.x-1.0 files"

That won't work. ;) I already explained that I had to fix another bug. Please use HEAD of update_advanced to test this patch (hence the 6.x-1.x-dev version of this issue). Thanks.

Sorry! Thanks for your patience.

Rolled the patch and still have no disabled modules showing.

Sorry! Thanks for your patience.

Patch 6.x-dev and still have no disabled modules showing.

@Aren: Repeating myself from #39:

Once the setting is on, I'm definitely seeing the disabled modules at admin/reports/updates. Are you sure your test site has any disabled modules sitting in sites/all/modules?

Also, note that it's not going to show disabled modules that are included with existing enabled modules. For example, if you have views enabled and views_ui disabled, you're not going to see separate update status for views_ui, since you're already warned about new releases of the entire views project. You need to put something else completely separate in a different subdir in sites/all/modules and not enable *any* of the modules included in the project, for that project to show up as a "Disabled module" in the report...

This is an ls in my sites/all/modules directory:

Compare that to the two attachments.

I currently have 9 sites, and I am pretty sure all these modules are used in one or another site.

@Aren: Oh, sorry. #445748: Add hook_update_projects_alter() isn't in 6.12 core. You need 6.x-dev core (the latest from DRUPAL-6--1 branch), or you need to apply the d6 patch manually. Thought that was clear from the history in this issue, but I guess I should be explicit. I'll probably wait to tag another official release of update_advanced with this feature until 6.13 core comes out, just to avoid confusion.

p.s. Maybe I should add something disable this feature and print a warning message if that hook isn't being invoked (e.g. if core is too old)? Maybe that's cruft and overkill, and this can just be solved by good documentation in the release notes.

dww: Thanks. I apologize for missing those details, but I do think this issue is becoming kind of long to catch so many details.

Is 6.13 far off?

6.13 will most likely be released when the next core security hole is discovered and patched.

Re-rolled patch for update_advanced 6.x. Tested with 6.13 and we now have fully functional disabled module update fetching! YAY!

Patch adds update_advanced_form_update_settings_alter but doesn't remove obsolete update_advanced_form_alter then.

webchick wanted to see what this looks like in the UI... very basic example from my current D7 test site.

Even better screenshot with more realistic examples...

Thanks for the screenshot.

I think it makes sense to add this feature to D7.

a) We already have API-level support for it in core (was committed at #13).
b) At #2, Arancaytar outlines a reasonable usability problem. You should not have to choose between enabling a module just to see if it has updates or trawling around on drupal.org.
c) It generally seems to have broad support, esp. from people running multisite installations.

However, we need to make sure the help text is clear about why someone would/would not want to switch this on.

This still doesn't address the UI concerns I raised in #33 about how to display the disabled modules within a project that's otherwise got enabled modules. But, it fixes a bug in #42 where all the core modules weren't getting listed properly.

Also, this adds a #description to the help text on the setting checkbox (and I'm including a screenie of that for easier review).

Moving this to the core queue to get attention before the D7 freeze...

Based on feedback from yoroy in IRC, this fixes the UI problem in #33. Screenies with the feature off and on attached.

The UI parts of this could use a little sanity checking (but let's not go too nuts).

I didn't immediately see any issues with the code but I am also stupidly tired. Will need to review this more tomorrow. Sorry. :(

Actually, here's an even better screenshot demo'ing the feature turned on. the CVS deploy project in here only has 1 (enabled) module, so the "Includes:" line looks like normal. Basically, we only do the split into two lines with (Enabled) vs. (Disabled) if a given project has any disabled modules or themes at all...

It looks fine with short lists. But with long lists, like the core (ubercart comes to mind for contrib) the (enabled) and (disabled) at the end of each line gets buried. Especially when the list gets wrapped over multiple lines. Mind, I proposed this but now seeing it I'm not sure this feature communicates itself clearly enough.

2 options:

1. Move enabled/disabled to the front of the list (ugly with all the colons, but clear)

Includes:
Enabled: Aap, Noot, Mies, Aap, Noot, Mies, Aap, Noot, Mies
Disabled: Ding, Flof, Bips, Ding, Flof, Bips, Ding, Flof, Bips, Ding, Flof, Bips

2. Add a bit more styling to the current solution by muting the color of the disabled ones and/or use bullets for each row.

I agree that (enabled) and (disabled) get lost currently. yoroy's option 1 seems like a reasonable way to go.

#64.2 sounds much less accessible. Here's #64.1.

Agreed. UI review done, approved. removing tag.

In further testing (and playing with the D6 backport), I realized the D7 version of this patch never addressed #33.A. This patch now does so. When the settings page is saved, if the checkbox for checking disabled modules/themes changes, we clear the cache of available update data to force a re-fetch. I'm not sure we need the drupal_set_message() about this, but that's easy to remove if we don't want it. Tested + working patch, and screenshot of the setting page after saving a change attached.

Is there a reason we need to immediately display all the Includes information? Maybe a followup patch could hide that information by default with jQuery and it could be toggled by clicking the module's section.

Based on IRC review from yoroy, we agreed to remove the message when you change the setting. So here it is with the proper behavior but no message to the admin about it.

@Dave Reid: Sure, that'd be a fine usability thing in a separate patch if you wanted to work on it. I'm on my way out of town and have 0 time to do anything until after freeze, but if you wanted to drive that home, sounds great. All the includes info is in <div class="includes"> already, so it'd be easy to also give that a "js-hide" class in update.report.inc, and add some jQuery to show it when the right trigger happens. Knock yourself out. ;)

I did a line-by-line review of the code (only because it's short) and my only real complain is that the unset($includes_items); should be $includes_items = array() which is more obvious and generally more semantically correct. I did not test but dww tests pretty thoroughly so I am not too concerned there.

I just looked at a screen shot on patch #68. It was all shiny and happy and good and then I got to the bottom checkbox. And then something inside me died. What's up with that help text which is LONGER than the checkbox text and says the SAME THING but differently? Please remove the additional text. It is not required. The presence of the new checkbox pleases me though and I approve of it.

On the actual update page I'm finding Includes, Enabled, Disabled intensely brain jarring and slightly confusing. It could probably be solved by better theming and should not prevent this patch from being accepted though.

The silly description on the checkbox went round and round in IRC the other night. My earlier patches never had it, and I think it's a waste of space. I'm very happy to see emmajane's opinion that it should go. Gone. ;) I agree that the "Enabled" and "Disabled" stuff is visually confusing now, but it's just a theme('item_list'). The default admin theme is so helpfully removing all styles on ul and li, which is why it looks kinda crappy here. I suppose we could force the issue in update.css, but I don't think it's worth it for this initial patch.

I also changed the unset to = array() at Earl's request in #71, and generally added some more code comments to clarify a few spots.

I think it's ready to go.

Couple of things:

1) Does this impact the usage statistics on drupal.org? Disabled modules probably shouldn't be counted?

2) When I looked at the screenshots I thought that "Dowload Release notes' was one link and was about to scream: (i) why do you need to download release notes? and (ii) we should lower-case the word 'Release'. Of course, it only took me two seconds for me to realize I was being dumb. Still, there might be room for improvement here. I know it is outside the scope of this issue.

3) Just wondering why we make this an option (versus always enabling it)? From a security point of view, it sounds worthwhile to always have this enabled (and to remove the option). Is enabling this such a usability problem that we want to provide an option?

Let's discuss this a bit more. Thanks!

Hm. #75.3 actually makes a good deal of sense. Then we wouldn't have the checkbox in the first place, and everyone would be happy. :D Is there a good reason NOT to do this?

#75.1 was asked and addressed in #24 and #25, but it wouldn't hurt to re-confirm that this is the case.

#75.2 Can be handled in a separate issue, indeed. Fortunately, it's also something we can fix post-code freeze. :)

Re: #75.1: No, doesn't impact the usage statistics at all. I already thought of that and addressed it at #445748: Add hook_update_projects_alter()

Re: #75.2: Right, outside the scope of this issue. This happens to be another thing that the default styles from the "seven" admin theme makes worse. That said, one very good place to start changing those links would be #555362: Support multiple download links in the available updates report (e.g. tar.gz + zip packages on drupal.org) ;)

Re: #75.3: Because (IMHO) it's information overload in many cases. Plus, when you disable a module, I believe users expect to see it "disappear" from their site. The fact it's still sitting around in their filesystem doesn't really occur to them unless they deeply understand how Drupal works, and I think it'd be jarring for them to see those things showing up on the Available updates report unless they specifically asked for them. My $0.02 at least.

If you really want to think about it more, you can, but this morning is my very last chance to do anything with this issue before 9/8, so if you think about it too long, either a) someone else has to drive it home, b) you need to be flexible about the code freeze, or c) this is going to wither and die and I'm going to be pretty pissed. Let's try to avoid (c), shall we? ;)

Back to RTBC since 75.1 was a potential show-stopper that's already fixed...

(To clarify, since webchick was concerned in IRC -- I'm not at all currently "pissed", and my comment was meant as a joke, not a threat).

As always, dww did an awesome job in documenting the code. Most of this is in line with the (rather critical) issue #535680: Prevent removing permissions of disabled modules, but I have some issues to raise for this patch:

I don't quite understand why

a) the $project_type argument isn't suffixed (instead of prefixed) with "disabled"

b) why the $project_type argument is a string and not a Boolean. At least, it's unclear to me why the following code cannot equally work using a Boolean of TRUE or FALSE.

Why not empty() here?

"Only record include" ?

This description talks about a certain trigger that is not explained elsewhere - how does one check only for enabled and how does one check for enabled and disabled modules/themes?

I'm on crack. Are you, too?

Thanks for the review, sun!

re (a): The answers lie in theme_update_report():

That's just how those strings have lived in core since D6 when my patch (from comment #8 in this very issue) was committed (see comment #13). I guess I went with those keys since they more closely matched the corresponding English labels, but whatever, if you think a suffix is more natural, we can go with that for D7.

re (b): _update_process_info_list() is shared by both modules and themes, though it has to know if it's processing theme or module data so that it can save the data in the right place in the resulting array so that themes and modules show up separately in the Available updates report. We could in principle change the API of this function and add a $status boolean, but that'd be an API change and would require a bigger patch. Maybe for comparison I'll one anyway, so core committers can decide which they prefer.

Why not empty() here?

Because the status column from {system} contains either 0 or 1, not TRUE, FALSE, nor NULL. We have a value, and it's either 0 or 1. I personally think it's slightly more self documenting this way. We're not wondering if the status is set -- we know it's there, we're just wondering exactly what the value is. But, I really don't care.

"Only record include" ?

It's "Only record include data". ;) Only record data about what's included... I can see how that's confusing, so I'll reword that.

how does one check only for enabled and how does one check for enabled and disabled modules/themes?

Uhh, by the checkbox we're adding here. ;) I'm not sure what you mean. This is a helper function. The test for the feature happens at the call site. If you want the comment to be more clear about something, can you be clear about what you want it to be clearer about? ;)

Ok, so here's a reroll that splits out $status into a separate argument, adds/cleans up yet more of the code comments, adds PHPDoc for _update_process_info_list(), and moves the "disabled" part to a suffix, not a prefix.

p.s. @emmajane: If you're still reading, here's a screenshot of the Available updates report with garland as the admin theme, so that you see:

Includes:
* Enabled: foo, bar
* Disabled: baz

as nature intended...

I think its obvious that this patch will clutter the page somewhat. As we are adding a whole list of possible modules (I have like 20 disabled modules) to this page. We could indeed try to put up an option, I am not sure if anyone will actually bother to click that - so lets not do that. I'd rather rethink the whole design of this page (sorry dww), then trying to find an awkward solution to hide this data - but this is better kept for a new issue when we find the time to do so.

We could probably place it in a fieldset, and expose > Disabled modules - 21 Security updates - but then again I think it would be visually somewhat confusing to have a listing inside a fieldset. In this case it might be a tradeoff worth taking.

Please always show admin facing stuff with "Seven" not Garland.

Well, a redesign of this entire page is certainly out of scope (as you said, new issue). It's not quite as simple as an on/off toggle on this page, because each time you change your mind you need to rebuild the project data and this takes quite awhile.

Unless of course, we remove the optionalness of this functionality altogether (as Dries suggested above) and always pull in disabled modules' data, and then we could indeed have some flexibility for how/where/if this extra information was presented.

In fact, the more I think of it, the more I think it makes sense to make this module behave the same way on all D7 sites, and the way that seems like it makes the most sense given the security implications is for both enabled/disabled modules to always be checked. So let's remove the option, and make this the default. Final answer (I hope :P).

If this extra data is seen as confusing to people in user testing, we could probably put something like an on/off JS hide toggle as a minor usability enhancement after freeze IMO. But honestly, I don't think it's going to come up. They can see on the modules page they have disabled modules, and if anything it'd be more confusing to not see them mirrored here.

Got time for one last re-roll dww? Else I can probably find someone in IRC to do it.

@Bojhan: I've been posting screenshots of seven the whole time, this final screenshot is simply an FYI about how it'd look with a different theme.

Seems a bit late in the dev cycle to want to massively redesign the UI for this page, and I certainly don't have time to do so in the next 30 minutes when I leave town for 2 weeks. ;) If that can be "usability enhancements" post freeze, we can consider it.

The new option is generally an advanced option. Not everyone is going to want it, and it defaults to off. Not only is it potentially adding "clutter" to the report and making something already dense with information even denser, it has another implication as well...

If your filesystem just happens to have dozens (hundreds) of contribs lying around but disabled, but we always fetch data for all of them, we're doing a lot more http traffic than we need (each project requires a separate request to updates.d.o), we're downloading a lot more data than we need, we're parsing a lot more data than we need, and we're lugging it around in RAM (and cached in the DB) than we need. Saying "Let's just always fetch everything no matter what and then figure out if/when to display it" is IMHO just going to bloat core, make it even more likely people run out of RAM, and force the issue of #238950: Reduce RAM resource consumption onto the table (see also #353524: Drupal update from 5.12 to 5.14 Update_status module responsible for MYSQL GONE AWAY warnings from D5 contrib, which is basically the same bug). People are going to get WSOD from lack of RAM, the thing we're trying to cache into the DB being larger than MySQL's max packet size, etc, etc, and there's going to be no way to turn it off, short of disabling update.module entirely. Instead, if we leave this setting (and continue to default to off), then we're much less likely to run into any of those problems. If someone turns on the checkbox and then runs out of RAM, and least they see that they changed a setting and that's what broke it, and they can revert the change.

I'd love to work on #238950 but I simply ran out of time, and webchick tells me that such refactoring (which will make testing easier) is probably off the table post 9/1. Unless that changes, D7's update.module is going to have the same RAM footprint that D6's does, and that makes me worried if we always fetch/parse/cache data for every module/theme on your filesystem, no matter what.

If I thought it was a good idea, I'd make time for a quick re-roll now. But, then you're likely to commit that, and I think that'd be a step backwards. C'mon people, it's a trivial checkbox with a self-evident name that defaults to off. If #D7UX can't handle that, we're in deep trouble. ;) Please re-consider #80. If you're unwilling to commit that, then please find someone else to re-roll this, and please be prepared for some "we had to change this API to fix bugs" style patches over at #238950 some-time post freeze (again, this is FYI so you can make an informed decision here, not a threat).

Thanks,
-Derek

Awesome - the code is much more comprehensible and cleaner now! :)

I think there is a common misunderstanding of disabled and uninstalled modules. If you have 20 disabled modules, because you disabled, but did not uninstall them, and you do not suppose to use (re-enable) them again, then you should uninstall them.

The purpose of disabled modules is to allow to keep their data and speed up your site, because the modules are just not loaded. For example, Administration menu allows you to quickly disable/re-enable all developer and UI-only modules (e.g. Views UI, ImageCache UI, etc.) on your production site until you need to do further structural changes, so you can quickly re-enable them again.

Disabled modules should be checked for updates, because disabled modules are supposed to be used. They are, just currently, not enabled.

We are not talking about not yet installed or uninstalled modules here.

This patch is ready to fly for me.

Ok. Those arguments are pretty convincing. I'm not sure what else there is left to discuss that's still within scope of this issue, so committed to HEAD. Thanks.

Is there already an issue to give this page a nice visual refresh in D7?

Unless, I missed the commit comment, this still needs backporting to Update Advanced, right? :-)

Side note: woo-hoo! Awesome work, everyone.

Subscribing

I can roll a patch for this tomorrow, unless dww wants help with a co-maintainer in which I could commit it directly. ;)

@Dave Reid: I'd love help with a co-maintainer. ;) However, everything I maintain with co-maintainers, I still do everything via patches and peer review... So, I'd gladly grant you commit access if you agree not to commit directly. ;)

No problem at all. I was planning on rolling the patch back here again anyway.

@Dave Reid: Reviewed and tested the patch -- looks great. Granted you CVS commit access. Please commit this to update_advanced HEAD (and give yourself, JohnAlbin, and me credit when you do). ;)

Thanks!
-Derek

p.s. If you wanted to reroll http://drupal.org/files/issues/162788-41.update_check_disabled.d5.patch from #41 for the D5 update_status backport, that'd be nice, too, but certainly not urgent. ;)

Touch a D5 patch? No thank you. I don't feel like putting a gun to my head. :)

I'm on this...

Committed to the module! YAY! WOO-HOO! Moving back down to D5 Update Status module.

Re-rolled for D5 contrib update_status.module. Tested and working great.

@Dave Reid: Thanks for the re-roll. However, it failed to apply after #499828-15: Wrong release date on multi-module projects went in. Also, it's not clear why #94 undid the new hook invocation from #445748: Add hook_update_projects_alter(). So, I took that out of the patch (i.e. left the hook invocation in place).

Revised patch correcting the double-hook invocation. :) This patch also removes the asort() from update_get_projects() because we should handle the sorting during display (will require patch from #319033-14: Weird order of projects listed on updates page to be committed to this as well in D5).

Thanks, sorry for the bad reroll in #95 -- it's been a crazy few days! Tested again, looks great. I just ripped out the dsm() about "attempted to re-fetch update data" when the setting is changed (as we did in core). Committed to DRUPAL-5--2 of update_status contrib. Yay! Moving back to core for posterity.

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