beta 3 breaker: update.module: opt-in, not opt-out

Would:

a) NOT including update.module in the list of modules enabled by default in default.profile
but yet
b) Offering the option to enable update.module -- *checked by default* -- during the installer, along with a detailed description that explains what exactly this means

...be enough to qualify as opt-out? Or do we need this totally disabled by default in order to not raise privacy watchdogs' hackles?

I don't want to belittle the consideration of privacy, however website security is equally an important consideration. Users who don't care about the privacy implications are unlikely to enable the module after the fact, as they tend to be more ignorant and not read instructions, while those who do will likely uncheck the option when given it in the installer.

Thoughts?

I wouldn't mind writing a patch for this since I brought it up, however I can't work on that till this evening.

Regarding Morbus' fourth and fifth points, perhaps the installer should have a 'what is this?' type of link next to the item, pointing back to a d.o node explaining what it does in detail and what the privacy policy is. I think for end users, seeing the explanation on the site the software would be connecting to, would be reassuring, instead of just seeing it in the installer/local only.

@ #1 webchick:

This is where I thought the '(more secure)' and '(more privacy)' labels might work better next to their respective options. That is something the apathetic users will more quickly understand and are more likely to think about.

@ #3 Morbus

I liked your idea of the warning on admin pages, that is not easily disabled (settings.php only) when update.module is not enabled. This also helps with webchick's case of users who click blindly through the install and forget about the option.

I am somewhat concerned about the perpetual support issue and perceived insecurity that such a warning may cause.

Note that system_update_6026() enables this module for all site upgrades. Morbus suggested solving this with a dsm() to the effect of:

"update has been enabled by default to protect your drupal site from security holes, etc., you can read more about it, and its privacy concerns, here."

There's a danger that this message might be lost in amongst the 50 billion other things that might be going on during the update, but at least we will have done our due diligence to inform people. And this text will always be available.. somewhere.. I guess maybe at admin/logs/update.

K, let me see what I can do.

The dsm message could be persistent, like update_status today on 5.x.

We can extend that to not only be on /admin, but also on every page if the uid == 1.

Ok, here's a first attempt. Probably needs work, but needs reviews first to identify what this work is. :)

- Added a new radios selection in the installer to enable/disable update status (defaults to enabled). It contains a "wall of text" that explains the privacy implications of this option. In addition, it's now removed from the list of default installed modules in default.profile.
- Adds an entry to system_requirements that displays an error if the module is not enabled. I'm not sure whether or not this is desirable... we're basically forcing people to enable it or deal with a constant warning on their sites that they train their users to ignore...
- Adds a drupal_set_message() during update.php with type "warning" to hopefully draw more attention to it. I struggled with where in UPGRADE.txt it would make sense to mention this, and the answer really was "nowhere"... we don't put version-specific information in that file, and CHANGELOG.txt is far too long to bury it in there. So this is really our best option.
- Adds a help page which goes into more details about what exactly the data is that's sent to drupal.org.

I tested this patch with a fresh install. It applies cleanly and works as advertised.

I really like the warning. Will definitely get the notice of users, but not harass them.

I would still like to see a link at the end of the text in the installer, to a page on d.o about update.module. Probably here , with a more detailed and longer explanation of how it works, whats done with the data, and so on....

- I like this patch but I suggest that we change the wording a little: the Update status module is technical speak. Better to write: update notifications. My mother doesn't have to know there is a dedicated module to do this; all she cares about is the functionality. ;-)

- The help text is a little wordy for me.

- Replace 'Update module' with 'update module', and try to avoid the use of that string as much as possible. See first bullet item.

very short review...

I think this is not correct and as i remember i changed this bug for project module, too:
'#options' => array(st('Disabled'), st('Enabled')),

... should be:
'#options' => array(0 => st('Disabled'), 1 =>st('Enabled')),

otherwise we will have the "words" in the value... what's not so good practice.

I think "update notifications" is the way to go. :)

@haas:

'#options' => array(st('Disabled'), st('Enabled')),

is just a short-hand way of writing:

'#options' => array(0 => st('Disabled'), 1 =>st('Enabled')),

PHP automatically sets 0-based numerical indexes for each entry. I'm not sure what the project module bug was, but maybe it needed text and was instead getting numbers? We're using this same syntax for Clean URL support, so it works fine.

@everyone else: New version of patch that rewords the installer field and description, and the status report title/description to say "update notifications." I do still use "The update status module" at admin/help/update, because that matches the format for the other help pages ("The $foo module..."), as well as the "To enable the update status module..".

I've taken out a lot of the explanatory text about the data sent, referring people instead to the Drupal.org privacy policy [http://drupal.org/privacy] which I've just created 4 seconds ago and needs to be actually, like, written and stuff. ;) I've created a separate issue for that here http://drupal.org/node/178776 because a) it's documentation-related, not code-related, and b) I don't want to mire down this patch, which is relatively simple (don't enable the update module by default), with another which is going to be complex and need a lot of back and forth.

Please review.

Subscribe. Will try to have a look tonight.

Parse error on line 944 of install.php. Looks like double quotes inside a double-quoted string. :-) Simple to fix, continuing...

"Update notifications periodically check..." "notifications " is an object. The object itself does nothing. Some other actor (update.module) uses notifications to perform some action. The grammar here needs work. The rest of the language feels a bit clunky, too.

Let's see if I'm too tired to come up with something here...

"Drupal can periodically check for new versions of itself and of contributed modules and themes you have installed, and alert you to available updates. These updates may include new features or important security and bug fixes. In order to check for updates, anonymous usage statistics will be sent to drupal.org. It is highly recommended you enable this option to inform you of updates (especially security fixes). This feature may be enabled or disabled at any time by enabling or disabling the "Update status" module. For more information, see drupal.org's privacy policy."

How's that sound? Yes it mentions "update status module" by name, but in the specific context of "here's how you can turn off the phone-home features at any time, so you don't have to make an earth-shattering decision right now". We already mention modules in that paragraph, and anyone installing Drupal will, I certainly hope, either have or will very soon have some concept of what a module is. (We mention it on the default front page.) That it's the update module that actually does the checking is not mentioned explicitly; it just says that "Drupal does it" (which should be proverbial-mom-friendly).

Also, while not listing array keys does work here, I find the code much more readable if it lists out the 0 and 1 keys explicitly. Yay for readable code! :-)

Margh @ parse error. Thanks, that was sloppy. :(

Most of the awkwardness in the wording stems from the fact that I've been trying to avoid using the string "Drupal", since installation profiles could be called anything (CivicSpace, WordSmoosh, etc.) and since the site_name variable isn't set yet here. However, that's getting a little ridiculous, so I'm gonna go ahead and include it. Distros can use the install-time translation features to change it to something else if they want.

New string is:
Drupal can check periodically for important bug fixes and security releases. In order to notify you of updates, your site will send anonymous information on its installed components to drupal.org (see the Drupal.org privacy policy). It is highly recommended that you enable this option for your site\'s security. For more information, view How update checking works page.

Thanks very much to snufkin, bangpound, and Heine for collaborating on the new version. And yes, this means yet another page we need to flesh out: http://drupal.org/update :P I'll make an issue for it later, unless someone beats me to it.

a) This is not a bug. The whole question of opt-in vs. opt-out was debated at great length in the effort to put the module in core, especailly with respect to privacy concerns. The fact that the wordpress fiasco on slashdot happened doesn't turn our thoughtful debate on the matter into a "mistake" that needs to be classified as a "bug". If the community changed its mind on this point in the last few weeks, fine, but let's be honest about what happened here.

b) I'm not opposed to these efforts, though, as always, I wish the people who cared strongly about this paid attention when I was barking for input on this question many weeks ago.

c) I wonder if the privacy-heads would prefer if the anonymous usage reporting could be disabled independently of the phone-home to check for updates. This is how 5.x-2.* update_status works in contrib, and we could easily make update.module in core work this way again. All it would take is re-adding the radio toggle on the settings page, and a few lines of code to check the variable in update.fetch.inc. This, too, was debated at great length in the original update-in-core issue, but if the wordpress thread is reopening old discussions, I wanted to raise this as another possible option to reconsider.

d) I'm worried about http://drupal.org/update and I remain worried about the decision to just call this module just "update". Why doesn't d.o/update talk about update.php? How can we avoid confusion between update status/notifications and DB updates? There's already been confusion in the issue queue over the "update.module" vs. "update system" components, for example. I don't have any particularly good suggestions, other than I wish we started using unambiguous terms for both, and retired "update" on its own from our vocabulary. Of course, this could get clumsy, and with enough context, "update" is often clear enough. I'm just hesitant to see this URL hard-coded into Drupal core for perpetuity, given the long-term potential for confusion given the overloaded, ambiguous word.

To clarify (b) and explain what I mean: I spent (wasted?) quite a few hours getting the core upgrade path working correctly to enable this module on upgrades from 5.x. Now, since all of a sudden people think this is a horrifying assult on Drupal user's privacy, webchick and others are again spending many hours 1) undoing my work and 2) doing new work of their own to make it behave differently (e.g. changing the default install profile, etc). If everyone were getting paid, and it was just a bad management decision that's costing someone the money to pay for this duplication of effort, that'd be fine. However, this is all volunteer work, so the fact that we're undoing it all now makes me feel a little frustrated and disappointed. I need to believe that my efforts on Drupal aren't going to be wasted, which is why I spend so much time rounding up input before I work on things of this magnitude. If it's throw-away work, I'll just stop doing it, since I have far too many other things in my life that are rewarding and which I feel passionately about. Not to say we never make mistakes, change our minds, or otherwise need to alter our direction on something, but in this instance, the *only* reason we're doing so is because not enough people bothered to pay attention when we were having our design discussions. :( This whole thing makes me question the "talk is silver, code is gold" motto, when in practice, it seems like way too much code gets written without enough talk, and it ends up being wasted effort.

Regarding a) and b), I was one of the people who did provide feedback when you asked about the opt-in/opt-out nature of this module. At the time, the "report usage statistics" checkbox was in there, so it was a no-brainer to make it opt-out. As you know, this option was taken out just prior to it being added to core... and I was the one who did that, per Dries's request, so that it would get committed. So realize that this is "undoing" my work as well... I assigned this issue to myself specifically for that reason, as I view this as "fixing" stuff that I "broke" when I did that last push on update.module just prior to it getting into core.

There is no change to the upgrade path apart from a simple dsm() informing people of what just happened and how to turn it off. The main change is at install time, where the module is *not* included in default.profile, but instead there is an option for it (enabled by default) that explains what exactly you're getting yourself into if you enable that option. Among the folks who discussed it, this seemed like a reasonable trade-off between potential privacy concerns and protecting the security of users' sites.

c) We could also do that, but I think the install change is a good one. I'm open to suggestions though.

d) Fair point. This could be renamed to anything, I just threw that in because I was on my way out the door.

I'm really sorry if you feel like this is "wasting" effort, but I do believe that this is important. I guess it wasn't until the /. hazing of WordPress that I started to really "get" *how* important it is. For example, drupal.org has no privacy policy, even though it is currently is and has been gathering anonymous (and not so anonymous, with drupal.module) usage statistics or some time. Fixing that is a critical bug, which was uncovered during the course of investigating this issue.

@webchick: sorry, didn't mean *you* were a source of the trouble here, and I certainly appreciate all the effort you've put in to update.module in core, both then and now. ;)

reviewing the patch...
A) typo with "notifucations" here:

'description' => $t('Update notifucations are not enabled. It is highly recommended that you enable the Update status module from the <a href="@module">module administration page</a> in order to stay up-to-date on new releases.', array('@module' => url('admin/build/modules'))),

Also, "up-to-date" in there seems a little funny, but I don't care strongly. Maybe "... in order to stay informed about new releases."?

B) The first hunk in install.php (regarding $zones) doesn't appear to have anything to do with this issue. Is that included in this patch intentionally? I see no mention of it in the comments here, and generally prefer to keep patches focused on a single change to make them easier to review, verify, test, and commit.

Otherwise, the code visually seems ok. I'd have to apply the patch and play with the interface to comment more closely, but I just got home from a terrible, exhausting week, and I should sleep before I do any real work.

@ #19 by dww

Regarding pint d)

I suggested earlier that we put this page at http://drupal.org/handbook/modules where we should have a page about update module anyway. I think that sort of placement in the handbook should make it clear which component is being discussed. Which functionity is another issue though....

Oh yeah, I noticed that in the patch for the help text, which points to http://drupal.org/handbook/modules/update -- it's not clear how that page is supposed to differ from http://drupal.org/update. I guess in the handbook/modules world, the context is pretty clear, and we can give that page a full title of "Update status module" to be consistent with the title for the module in the .info file (and therefore, at admin/build/modules). Is there any reason we need d.o/update at all, or should we just change everything to point there? Certainly d.o/privacy is worth having, and can itself point to d.o/handbook/modules/update...

I'd love to see this fixed before we roll a beta 2.

I don't think we need a link to a privacy policy in the help text. I'd prefer to keep this simple, and to explain in one or two sentences what this is about. No need for a multi-page privacy policy that is hosted on drupal.org ...

For similar reasons, I wouldn't mind to see the "For more information, view How update checking works page." sentence go. Does Apple or Microsoft tell you how their update system works? No. Do their users demand that they tell us? No.

I too would like to see this patch go in before we release a Drupal 6 beta 2.

I will not fight for this, but

Does Apple or Microsoft tell you how their update system works? No.

Correct.

Does Apple or Microsoft tell you how their update system works? No. Do their users demand that they tell us? No.

Wrong - YES, we'd like to know, but they don't tell us and we'd like to move away...

i'd really like to log the ip that submitted the usage to prevent usage spamming: http://drupal.org/node/168009

I'm indescribably swamped this week, so if this is going to hold up the beta, someone else should likely take it on.

Changes I would make if i could:
- Remove the link to /update and instead change the in-Drupal help page so that it explains whatever we'd want at that page.
- Fix the typo. :P
- Shorten the wall of text... not sure how... guess we need to brainstorm on what's actually 'critical' information.
- Leave in the privacy policy link, imo; I think this is good information (might even somehow be legally required), users can just ignore it if they want, and we really, desperately need a privacy policy on drupal.org in any case.

What about:

Drupal can check periodically for important bug fixes and security releases. To do this, your site will send anonymous information to drupal.org. It is highly recommended that you enable this option for your site's security. For more information, view How update checking works page and the Drupal.org Privacy Policty.

This is a little less wordy, but give links for more info.

Or simply a "Get More Information" link that goes to the page on the Update module which has a link to the Privacy Policy.

Drupal can check periodically for important bug fixes and security releases. To do this, your site will send anonymous information to drupal.org. It is highly recommended that you enable this option for your site's security. Get More Information

Also, if we move the info to the hook_help page, that won't help the user during the install...

@webchick I'd be happy to fix up this patch once we decide what's going in there.

I have a hard time understand what is holding back this issue. Please, can someone summarize it for me?

I think Dries wanted to cut down on the wall of text and simplify it a bit. Webchick and I added a couple of suggestions, and I was waiting to hear feedback on the new text before rolling the final patch.

I think one or two sentence with a summary of the privacy policy / how the process works is better then linking to a multipage document, but I am not sure we should avoid a more deep explanation. I would link to a single document however on drupal.org to simplify getting more information for the user.

Was my note helpful in bringing the patch forward? :)

I'll be back home (finally) on Tuesday; if there's no movement on this patch before then, I'll go ahead and make an attempt at a re-roll. But honestly, since it's just wording at this point, I'd recommend someone take a stab at it so we can get this sucker in.

How about

For more information about how drupal knows how to get the most recent security patches and downloads, click here.

A combination of #31 and #37:

Drupal can check periodically for important bug fixes and security releases. To do this, your site will send anonymous information to drupal.org. It is highly recommended that you enable this option for your site's security. For more information about this process, click here.

OK, I am taking on this patch and will come with an updated version in half an hour or so.

Should never use "click here" for accessibility reasons - some people don't click links depending on how they use the internet.

Here's another try, slightly changed the second sentence

Drupal can check periodically for important bug fixes and security releases and it is highly recommended that you enable this option for your site's security. This process involves your site sending anonymous information to drupal.org. For more information visit http://drupal.org/update.

OK here is an updated patch. The idea is to basically move out any complexity to the drupal.org handbook, and let the handbook explain what happens if that is what desired. Changes include:

- unrelated time zone changes removed (committed already) from the patch
- dot removed from the end of st('Check for updates automatically') for consistency
- the install page now points to the update status handbook page and is shortened:

st('Drupal can check periodically for important bug fixes and security releases. To do this, your site will send anonymous information on its installed components to drupal.org. It is highly recommended that you enable this option for your site\'s security. For more information please read the configuration and customization handbook Update status page.', array('@update' => 'http://drupal.org/handbook/modules/update'))

- the status page warning now links to the update status handbook page (we cannot link to the internal help page because the module is not enabled) I also strong-ed "highly recommended" for consistency.

$t('Update notifications are not enabled. It is highly recommended that you enable the Update status module from the module administration page in order to stay up-to-date on new releases. For more information please read the configuration and customization handbook Update status page.', array('@update' => 'http://drupal.org/handbook/modules/update', '@module' => url('admin/build/modules')))

- The automatic update status turnon note on update is now using the proper module name (previously was "update module"). This links to the module help page as it is turned on at this stage.

t('The Update status module was automatically enabled. This module automatically checks for new versions of modules and themes. For more information, please read the Update status module help page.', array('@update-help' => url('admin/help/update'))

- I shortened the explanation on the update status help page by removing the links to the privacy policy and drupal.org/update. The standard "for more information" link is used as in every other module. People can get more information there:

+ $output = '

'. t("The Update status module periodically checks for new versions of your site's
software (including contributed modules and themes), and alerts you to available updates.") .'

';
+ $output .= '

'. t('The report of available updates will indicate
when new releases are ready for download, and you may configure various options, including frequency of
update checking and notification options, at the Update status module setti
ngs page.', array('@update-report' => url('admin/logs/updates'), '@update-settings' => url('admin/l
ogs/updates/settings'))) .'

';
+ $output .= '

'. t('Please note that in order to provide this information, anonymous usage stat
istics are sent to drupal.org. If desired, you may disable the Update status module from the modules">module administration page.', array('@modules' => url('admin/build/modules'))) .'

';
+ $output .= '

'. t('For more information please read the configuration and customization handbo
ok Update status page.', array('@update' => 'http://drupal.org/handbook/modules/u
pdate')) .'

';

So the general direction is to link to the drupal.org handbook page when the module is not (yet) enabled, and link to the module help page when it is enabled. Then the help page gives some information and points to the drupal.org handbook page. This is a standard approach used with *all* modules in Drupal, so it should fit here. What is different with other modules, is that we are a bit more pushy with linking to the information, so people interested can have an idea.

What should be on the handbook page is not part of this issue, so we can happily leave that to the handbook authors.

Let's review this!

Doh, should have used some code tags. Review the code instead of the post. Sorry.

BTW my patch also changed "log of updates" to "report of updates" which is what the menu item description says.

Looking good. Two suggestions:

For more information please read the configuration and customization handbook Update status page

this is hard to read. Can we drop the handbook title?

For more information please read the Update status handbook page

and:

The report of available updates will indicate when new releases are ready for download, and you may configure various options, including frequency of update checking and notification options, at the Update status module settings page.

Changing "indicate" to "alert" and break it into two separate sentences, reworded the last sentence to avoid using the word "options" twice:

The report of available updates will alert you when new releases are available for download. You may configure options for update checking frequency and notifications at the Update module settings page.

I agree with the second suggestion, and I would in fact agree with the first, *but* the text used is standardized around all modules, so if we shorten it, we lose some consistency. Either we need to shorten it elsewhere too, or do not shorten it on the admin page but only on the other two occasions I added it.

OK, looked at how the shortened version looks on the interface, and I think it makes for an easier to understand text, so implemented all of your suggestions. Modified patch attached.

Still needs review.

OK this my first actual test of the patch although I've followed the various development list discussions and this issue relatively closely.

I installed drupal with the patch applied (bit of fuzz but my tarball is a day old). The option and help text in the install screen both make complete sense, I don't think the text could be any better worded than it currently is and like that it's short, to the point, and links to further information rather than trying to included it in the installer which'd only be a distraction to the job of actually installing.

After disabling the option, I then get the warning in the admin screen and status report. Again the wording is concise and to the point - 100% agree with calling it "Update status handbook page" - the version with "configuration and customisation...." is far too wordy for a short message.

The only tiny, tiny gripe I have is that enable the Update status module from the module administration page doesn't go to an #update id so you just see your list of modules. With many modules installed the user would have to look through a long list to find what they're after. However no such id exists in that table row, and no other help texts do this, and if people think ids for each module on the module admin page are a good idea, that could be a patch for D7 but doesn't belong here ;)

This could maybe do with another review before it goes RTBC, but I hope that helps.

Some minor usability concerns:

A) In the installer, the 2 choices are:
+ '#options' => array(0 => st('Disabled'), 1 => st('Check for updates automatically')),
The wording of the "enabled" choice talks about "automatically", which seems to imply that if you select "disabled", you could still manually check. This isn't the case. You simply cannot check for updates, manually or automatically, unless you're willing to give up a little privacy. Not sure exactly what to do about this, and I'm not sure anyone's going to be confused by it, but I wanted to point this out.

B) system_update_6026()'s message reads:
The Update status module was automatically enabled. This module automatically checks for new versions of modules and themes. For more information please read the <a href="@update-help">Update status module help page</a>.

Do we need "automatically" in both sentences? How about removing it from the first, like so:

The Update status module was enabled. This module automatically checks for new versions of modules and themes. For more information please read the <a href="@update-help">Update status module help page</a>.
??

A) I think of the manual option as "going to the project page yourself and looking at the release nodes". I don't think it will confuse anyone, and can't think of an alternative.

B) Patch attached that removes the first "automatically" - no reason for it to be in there.

webchick tells me you're now planning to remove the DB update that enables this module by default, since in 0.000001% of the cases, some privacy nut's site will happen to have cron.php run between the time they run the db update and the time they have a chance to run kicking and screaming over to admin/build/modules to disable it. Concern for this extremely tiny edge case will potentially cause harm in the other 99.999999% of cases, since people will gloss over the messages and warnings when they run update.php. This seems like a bad move all the way around.

However, re-reading the patch with this potential change in mind, i see that the "solution" to this problem is the new system_requirements runtime check for if the update.module is enabled. That way, if they miss the info about the new module during the update.php phase, they'll still get a warning about it on /admin/* and at the status report page. So, we're trying to make this functionality "opt-in", but if you refuse to opt-in, then Drupal always flags your site as having "Problems", and there's no way to make the warning go away? If anything, that'll be even worse for the privacy nuts. They'll tar and feather us over this behavior. "Sure, they call it opt-in, but if you don't let big brother watch your site, you get this permanent nag-ware error about it...".

Sorry, but this whole approach seems to be losing sight of the supposed point of this issue. Our goals:

A) encourage as many sites to run update.module as possible to improve security for the community (and, coincidentally, make our usage stats more accurate).

B) Appease the privacy nuts by making it possible to disable this functionality if they don't want it.

The current approach doesn't really meet either goal, and the proposed change will make it worse.

If we're going to have this requirements check, we need to have a checkbox that sets a variable that shuts the error off. A "opt-out, and I really mean it" checkbox so that an admin can say "yes, i know, i really meant it when i said i didn't want your big brother module, so shut up already". Many programs use something like "don't warn me about this again" or some such.

If we remove the requirements check completely, then we really *SHOULD* keep the module enabled during update.php. It'd be a huge lost opportunity if a big % of the sites upgrading to D6 didn't run the module since they didn't notice a message about it burried in the update.php output.

Whoops, to clarify: by "current approach" i meant the patches in #46 and #49, and by "proposed approach" i meant the suggestion to remove the db update entirely.

The *existing code* in D6 is actually better than either one, since a) most sites will run it by default, but b) if people don't want it, they can easily turn it off. That said, the changes to the installer are reasonable, and all the help text improvements are nice, so it'd be a good idea to use some of the work that went into this patch.

But, I think the permanent 'error' once you disable the module is almost as bad as (if not worse) than having it be opt-out instead of opt-in...

OK. This patch does the following things:

a) Incorporates catch's changes from #49 (incidentally, I think the suggestion in #47 about adding an ID to each module on the admin/build/modules page is a great usability enhancement that could make it into 6.x in a separate patch).

b) Removes the auto-enable code from update_6026. This is unfortunate, both because dww spent a lot of time getting it just so, and also because this is a lot more convenient for the 99.9999% case. However, in order for this feature to be opt-out, I don't see any other way to do this. Instead, it auto-enables only if update_status.module is enabled, and displays a big honkin' warning if it's not.

c) The "annoying error that never goes away problem" is satisfied by the creation of a "Disable updates" module in contrib (or similar) that's simply a blank update.module that's put insde the sites/all/modules directory. When this is done, it overrides core's offering, so it will remove the warning for those sites that want to *seriously* remove this functionality, while keeping the warning for everyone else, where this is likely an oversight. I'll go and create such a module in a moment.

I think this satisfies everyone's concerns. Marking for review.

@webchick:

a) I agree the usability thing with #anchors on the modules page is a lovely usability idea that could still make it in to D6.

b) I haven't tested your patch, but I know I added code to disable modules during update.php that don't match the requirements in .info. @see update_fix_compatibility(). Namely, if update_status.info from D5 is there, and you run update.php, it'll disable update_status.module automatically, since the .info file doesn't contain core = 6.x. Plus, they're going to have some grief if they're currently running update_status and they upgrade, since there will be some stale stuff left in their DB that's hard to clean up.

We're already telling people that are running D5 update_status that they better disable and uninstall the D5 contrib before they try to upgrade to D6. I think this code in your modified update will never fire because of the fix to update.php I made to enforce the core .info file keyword, and even if it did work, I don't think we'd want to just install update.module automatically if update_status was enabled, due to the DB migration issues and potential table collisions, etc. therefore, CNW, since you should rip that stuff out of the update.

c) that seems like quite a hack, but whatever, it's a hack to appease the people who are going to bitterly complain no matter what we do, so i don't care how ugly the "solution" for them is...

Thanks for the review, dww. I've done as you asked and removed the update_status module checking from the update code. So now only the warning is displayed in 6026.

Thanks for being understanding about the 0.00001% case. I'm really, really sorry it involved undoing some of your hard work. :(

Update Notifications Disable module now available: http://drupal.org/project/update_notifications_disable

I think the Update Notifications Disable module is a sufficiently annoying step to have to take that'll it'll encourage all but the ultra-paranoid to enable update status. Big +1!

I've started an issue for #anchors on admin/build/modules here: http://drupal.org/node/184010

OK I tested this with and without enabling the module at startup, and everything seems to be working fine, so #54 ought to be RTBC.

Okie doke then. Thanks!!

- I still think "Update status handbook page" is somewhat confusing. Why not "update notification information" or something less Drupal-speak? This is not a showstopper for me.

- Enable Update status module ... in code comments we can call the module by its exact name: "update.module" or "update module".

- In user output, I would change using the new Update status module to using the module update notification mechanism, or something. "Update status module" is Drupal-speak that might not be understood by content editors.

- "The Update status module" should be "The update status module", if you insist on using "update status module".

- st('Check for updates automatically') should be st('Enabled') IMO.

- st('Check for updates automatically') should be st('Enabled') IMO.
IMO, the problem isn't this text but the fact that we use radio buttons for that. The text seems okay to me (and more descriptive!), but we should use a checkbox..

Agree with Stefan on this - checkbox would be better, and more standard with online opt-out clauses across websites in general.

Ok:
1. Made the option a checkbox (actually checkboxes, with one option, because without the header it looks like it's grouped with clean URLs, which is confusing).

2. I made "Update status module" into "update status module" everywhere. We generally capitalize our module names, but since the proper name is "Update status" and since "something something Update status module" looks extremely odd, this is the safer option.

3. Changed it to update.module in the comments.

I'm a little torn about how much to "dumb down" the text. Yes, module's a somewhat technical term, but it's also necessary in order to talk about where to turn the functionality on and off. I really don't want us to go down another "node/post" road, where we try and make stuff less confusing by using non-technical terms and only succeed in confusing the heck out of everyone.

We can rename the link to 'update notification information' or similar (I did do this in the installer, since it's reasonable that someone doesn't know what a module is yet at that stage), but once you get there it's talking about "The update status module..." (since this is the convention with Drupal help pages). So I'm not sure if this really adds to usability as much as we'd like. I'm open to suggestions, though.

I'm strongly in favour of keeping the text to as 'module' for the reasons webchick states. If someone's ever going to install and update a module, they're going to need to understand ftp/wget, what gzips are etc. , I don't think it's too much to introduce the word module to them early on.

looks like the update module is enabled even if you untick the checkbox.

lol well that's no good. :P

also, addi mentioned that she was getting a WSOD at the update logs screen. going to investigate that as well.

And I guess this is a beta 3 breaker now. ;)

It looks like the WSOD is a caching issue. If you clear out the cache table, the WSOD at the update logs screen goes away. Tried calling cache_clear_all() right after module_enable(), but no success. Also tried menu_rebuild(), but that didn't work either.

I noticed this behavior the other day as well (#183909), but thought it was just me. For the record, I was getting this exact same behavior at /admin/logs/updates, until I cleared my cache.

This may have some relevance to this issue if you think it is all cache related: http://drupal.org/node/182675

Looks like the install of update status happens because of some weird (at least to me) behavior of the checkbox on the install configure form.

I did a var_dump of the form_state on submission of this page to figure out what was going on.

If the box is checked $form_state['values']['update_status_module'] -> 1,
If the box is unchecked $form_state['values']['update_status_module'] -> boolean true.

This change fixes the problem: (line 989 of install.php)

  if ($form_state['values']['update_status_module'] === '1') {
    drupal_install_modules(array('update'));
  }

I didn't bother rolling a patch, because it seems like this is odd FAPI behavior. I can easily create a patch if desired though...

I think in general the verbage is clear enough.

Not sure, but i think the cause of this is http://drupal.org/node/178581#comment-310934... not tested.

This form is so broken I believed all checkboxes are broken but thanks god, I was wrong and checkboxes are not broken for a change. This form is a) set to #programmed TRUE and with programmed we would expect an array() for an empty checkboxes but as this is not a real programmed we get a big nothing instead b) it checks for the whole checkboxes element, either it should check after an array_keys(array_filter()) or simpler just check for the one checkbox inside. I changed #programmed TRUE to #redirect FALSE and found no problems.

Awesome!! Thank you SO much, chx!!

The checkbox is working now, so the opt-out setting actually works. ;) admin/reports/status successfully shows a big red error when this functionality is not enabled, and http://drupal.org/project/update_notifications_disable successfully removes the warning when it's enabled.

Marking RTBC.

Just tested the latest patch (from #73). I think the interface and the functionality looks good. One problem I noticed here is that the install_tasks() code comments talk about setting the form to programmed which we change here. I wonder what was the actual goal of using a programmed form there (possibly to avoid some Form API functionality to execute which is not yet available). Anyway, the code comments should be reworded with care.

Noone knew why that #programmed was there, I asked those who worked on the issue where it got added and tried to figure out myself. No luck. I tested. No problems. So, it's totally removed with comment and all.

I tested this again, and all seem to be fine for me know, so committed. Thanks to all who participated.

Awesome!! Thanks, Gábor! :D