update.module in core (formerly known as update_status)

First, we're going to need a way to *identify* the module. The .info file is going to need to contain either the project URL or the project name that can be reliably aliased to the project URL.

xmlrpc, at that point, is actually super simple to respond to, I believe. You implement hook_xmlrpc() which
returns a structure like this (from blogapi):

Basically it's an identifier, a callback, an argument structure and a description.

It's very very simple.

all of this would only work for modules that live at d.o, which not necessarily *all* drupal modules in the wild are... so, we'd have to deal with this in a way that doesn't screw over other people's modules. this needs more thought. for this reason, i'm thinking a full URL (e.g. "http://drupal.org/project/signup") would be better than just having the project name or even the nid of the project node. but, i'm really not sure yet, which is why this is 6.x material. ;)

I don't believe it has to be 6.x material. Ideally I'd like to see a contrib module that can check module version numbers; and that's why I'd like to see something like this put in place before it gets too too late. The longer we go, the more .info files will exist without enough information.

While Drupal itself doesn't need to take advantage of it, if the packaging scripts put the information in there, then we can make use of it; if it doesn't, we cannot use it in contrib.

If we want to support modules that don't live on drupal.org we could do something like this:

Project home = drupal.org
Project name = views

From that, we can construct the xmlrpc URL, and have all the information we need to query. Then all that's needed is a simple xmlrpc implementation that responds. That part can come later too; but getting the information into the .info files is, IMO, something that should happen early, if possible, which is why I'm pressing this before the patches to the packaging system get committed. =)

Recall that we already have as of 4.7 a way in drupal.module for sites to connect with drupal.org (or another server) and send data on what modules they have installed, implemented with xml-rpc. This was originally conceived as a first step toward fetching data on updates etc. Presumably drupal.org is currently collecting such data, though I don't know if anyone has actually checked. A quick "SELECT count(*) as count FROM {client_system}" would tell. But is this useful?

yes, we're collecting the data. there are currently 40008 records in that table on d.o. is this info useful in what way, nedjo? it's certainly useful, e.g. to show project usage data, browse-by-popularity stuff, etc. i'm just not sure what this has to do with my proposal here. instead of drupal.module sending an RPC to tell drupal.org what modules are installed (which doesn't include any version info right now, anyway), i was imagining that the clients would query for updates on drupal.org (via check_updates.module, not drupal.module). we want the client installation to immediately tell the admin they're out of date, not to tell that to drupal.org (then we have to periodically inform sites they're stale or something)...

i think i'm misunderstanding your comment. ;) or, is that your point: "this stuff exists but it's not useful for this discussion"?

thanks,
-derek

a patch for the packaging script now lives here: http://drupal.org/node/98277

we should move discussion about the details of what data to include in the .info files and how it should look into that issue, and leave this one as the feature request for putting the module-page functionality into 6.x core.

thanks,
-derek

or, is that your point: "this stuff exists but it's not useful for this discussion"?

I guess my point is that the first step in fetching update information is a basic mechanism for posting data on what a site has installed, and this we have partially in place.

To fetch update data, a client site has to initiate a connection with a central server and post information on what it has installed. Then the server needs to respond with relevant data and the client needs to know what to do with those data (e.g., display them in some way to a site admin).

Early iterations of the patch I did for drupal.module included rough versions of all this. Rather than just passively log the site data, drupal.org would respond with relevant data, which would display on on the client site. I also coded a hook through which other modules (e.g., project.module) could respond to contacts from client sites.

That patch was late in the 4.7 cycle and there were concerns about potential security issues arising from these data exchanges, so we removed all that before applying the patch.

Now, as we return to this issue, we have the option of building on this existing exchange mechanism, which has some of what we need. Or we could strip all this out of drupal.module and instead move it to a new module (check_updates). In any case, we probably don't need or want to have two different XML-RPC implementations of ways for a client site to post data on its installed modules to drupal.org.

cool, thanks for the clarifications. there's no way we can build on drupal.module for 5.x, so i think whatever we're going to do in the short term will be a stand-alone contrib that does its own thing. we can certainly look more closely at what drupal.module is doing, since i now see more of what you mean about how this is related.

personally, the implementation details aren't as important to me right now as the data we need in the .info files to make it possible. of course, what data we need (partially) depends on the implementation details, but i think what's laid out in http://drupal.org/node/98277 should cover everything we'd want. then, the .info files will say what host the module lives at (so we only check for updates at drupal.org, etc), the project short name and the release version (so we know exactly what we have). seems like that's all we could possibly need, right?

whether we do seperate XML-RPC calls to drupal.org/project/release/version-check/[project-name] for each contrib we need to query, or if we do a single call to drupal.org/project/release/version-check and post an array and get back another array, or if we subscribe to a series of drupal.org/project/release/[project-name]/feed RSS feeds, in all cases, we have the data we need to construct the query we want.

Just a few thoughts on this.

I personally would prefer a more anonymous system where drupal.org would have a full list of all modules and the latest version number that the client pulls, rather than having to send information about my system to drupal.org. I know, I'm a little paranoid, but I like my anonymity some times. ;-)

Also, it would be good to have a way to mark a module as locally modified (for instance if you applied a patch that would be erased if you updated) and get warned about that when you are notified of an updated module.

Nedjo is overstating things when he says 'have to' register anything. All that 'has' to happen is for drupal.org to be able to answer requests for information about a given project. It doesn't need to record anything about that incoming request.

All that 'has' to happen is for drupal.org to be able to answer requests for information about a given project.

Yes, any logging on the drupal.org server is separate from the main interest here which is fetching available update information. The two processes just happen to involve the same sort of transaction, posting of data on installed modules and themes.

(As Steven mentions on the dev list, we should build in support for themes and theme engines too--they also get updated, they also may involve security issues.)

So, yes, a contrib mod sounds like the way to go for 5.0. Drupal.module probably offers some code we could crib from. The main needed addition would be version data. And then, with that in place, we could return to consider what makes best sense for the next release version of Drupal.

RSS and XMLRPC are not really competitors and choosing between them doesn't matter a whole lot anyway. One simple alternative is to use a regular old menu callback in project module. Or several callbacks. The callbacks just should just return a small XML page that the caller can parse. The XML contains project summary info, release info, announcements, etc. something like (square backets help avoid formatting problem):

As for how to define the phone home info, lets just use single URL like http://?project=x&release=y. thats the simplest, for drupal.org projects, it will look something like http://drupal.org/project/summary?project=views&release=x. Note that this URL isn't as pretty as it could be but lets not make any unneeded demands on the server side for this. In my example, 'summary' is a new menu callback unrelated to any project.

In the end, whomever implements this should just use whatever he is comfortable using. It makes little difference.

whether we do seperate XML-RPC calls to drupal.org/project/release/version-check/[project-name] for each contrib we need to query, or if we do a single call to drupal.org/project/release/version-check and post an array and get back another array, or if we subscribe to a series of drupal.org/project/release/[project-name]/feed RSS feeds, in all cases, we have the data we need to construct the query we want.

I agree with moonray about a "pull-only" method. What about a simple XML list of (module name, latest version) for all modules? This is the method used by apt, PEAR, PECL, and RHN (for Red Hat Linux). RHN in fact uses XML-RPC to retrieve the list. The process is:

1. Client contacts drupal.org with installed version of Drupal (4.6.10, 4.7.4, 5.1, whatever)
2. project.module on drupal.org returns a simple XML list of the latest version of modules (eg. 5.1-1.2) compatible with the user's installation. (The complete release info suggested in #12 isn't really useful to the user.)
3. Client processes results and displays potential updates to the user.

As a result, drupal.org serves one (cached?) XML document per update check, instead of generating short XML replies to dozens of XML-RPC requests. The single XML reply also requires less processing overhead than an RSS feed; it's a simply-formatted dump of a single query against project_releases.

Another suggestion: instead of having the modules page automatically perform this query, provide a prominent "check for updated modules" link. This will drastically reduce the number of requests for the lists, and also prevent the page from being cluttered with information the user may not care about.

With that sort of process, Drupal is one step from automatically generating URLs like eg. "http://drupal.org/files/projects/og-5.x-1.x-dev.tar.gz", downloading and installing new module releases—but that's beyond the scope of this issue.

Thoughts?

Subscribing to this thread.
Agree with Paul's comments here.
Just having a client pull a full XML based list periodically (maybe on every x-th cron run or something like that and parsing it - or click here to check for updates) seems straightforward enough.

Nedjo has a patch for the server side XML: http://drupal.org/node/48580

#48580 is now committed and installed on d.o. the update status module does this in 5.x contrib. we basically just have to turn that module (quite small) into a patch against system.module.

Hmm... I got to this thread from the "cram-a-thon" link, where it says "Update status in core."

So here's my two cents worth (keep te change if you think that's too much).

I like Update_status and use it on my sites. However, I think it's still changing too much to move it all into core. We don't even have themes using it yet. I would prefer to see it remain as contrib until it is far more stable (unchanging). It is easy for almost anyone to update a changed contrib, but few Drupallers want to attempt updating a core module.

nancyw: Your logic in that argument is generally spurious.

"However, I think it's still changing too much to move it all into core."

Many of the changes it's going through right now are part of meeting the requirements for going into core.
Many of the changes it's going through right now are due to re-evaluating our needs in the protocol we were using.

"We don't even have themes using it yet."

Of course we don't. We've said -- in threads I know you've read, that it can't support themes until themes have .info files. THemes don't have .info files in Drupal 5. Therefore, update_status.module, as it is now, can't support themes. This is a 'duh' moment. By your argument, we basically can't move forward. That's stupid.

"It is easy for almost anyone to update a changed contrib, but few Drupallers want to attempt updating a core module. "

What does this mean? The update status that is released with core will not go through lots of small updates. It'll go through the same test/review process, the same as core, and likely be stable. It's not as though we're going to stick update_status in core and then go "OH update this, update that".

Frankly, as a developer of update stauts, I find your comment in generally really insulting, as though you feel we're doing a bad job of it, even though you don't really understand what's going on here.

The fact is, update status is going into core in Drupal 6, unless something big happens to make it just not work out.

If someone wants to help out with getting update_status in 6.x, should we be testing the development snapshot of 5.x-2.x-dev over on Release for Update status page? Or getting the HEAD version from CVS?

Or should we just wait until a patch shows up on this issue?

right. the best way to help this is by helping with the 5.x-2.0 release of update_status by testing 5.x-2.x-dev and any patches in the issue queue. getting this feature into core is temporarily postponed, blocked on the official 5.x-2.0 release of update_status.

thanks for the offer to help! as soon as the 5.x-2.0 release is out, i'll shift my attention to converting the code into a patch against D6 core, and when that's ready, i'll change ths status of this issue to "patch (code needs review)".

I feel the privacy-paranoid pull-only method will result in a MUCH larger file for all clients to fetch all the time. We have hundreds of modules and themes to choose from. The caching on the drupal.org end is certainly a bonus, but my gut instinct is the bandwidth for 'everything, all the time' will be an order of magnitude greater - multiplied by the number of requests.

Soo... simply offer both both methods via the client UI. Let them that care choose the slow option, but have the lightweight eclectic version the default.
I think it'd be cool to get some popularity statistics for modules.

and +1 on making it opt-in via client admin, not auto. Possibly with a reminder to do so popping up on a '1 month since last checked' basis.

Also, how can we opt-out individual modules/themes if we've branched/customized a contrib beyond recognition?

Note: the download should be ".gz", but I uploaded as ".tgz" due to d.o restrictions. It is gzip'd; not tar'd & gzip'd.

I realize this issue is postponed, but here is some data for informational purposes. The following script downloads the project-release XML information for all "stable" modules (with a "1.0" or better release), then concatenates, gzips and serves the data as a download.

You can run it for yourself (hopefully!), but I've attached what resulted for me—a 68.4-kB archive containing a 749.9 kB text file with information on 419 packages that have 5.x-series release. The drastic compression should be expected, since the XML data contains many repeating strings. @dman, I'm not sure what the going definition of a much larger file is, but 68.4 kB would seem to be smaller than most actual packages. I concede that 750 kB is a lot of data to process on the client end, but this isn't something that really needs to be done more than daily, IMHO. It could also be processed incrementally.

Also, the module list would be much smaller if (as in APT and the other package management systems I'm trying to ape here) it included only information on the most recent release. The premise is "if your installed module is out of date, you should update to the newest release, and that means you don't care about intermediate releases." The current file contains, for example, Aggregation releases 1.0 through 1.3, 2.0 through 2.15, 3.0 and 3.1... 22 releases in total! With information on only 1.3, 2.15 and 3.1, users of any major version of Aggregation would know whether their installation is out of date within a major version, or in general.

To filter these out would require a bit of DOM manipulation but wouldn't be difficult. If anyone is curious how slim the module list would then become, I'd be happy to work it out.

@Paul Kishimoto: no offense, but what are you doing?! ;)

You clearly haven't read any of the issues I linked to. And you've written and posted a script to pound/screen-scrape one of the most expensive pages on drupal.org: http://drupal.org/project/Modules/name. Please don't do this. Please see http://drupal.org/project/update_status, its issue queue, and other related discussions.

Thanks,
-Derek

After a discussion about if this should be required or an optional core module, and what it should be called, Dries and many others have decided this will be a stand-alone module called "update.module", enabled by default, but not required.

So here it is, the Update status module from D5 contrib, ported to D6, enhanced to support themes and a bunch of other improvements based on the work in my sandbox.

Sadly, there are only 4 modules ported to D6 right now with available releases, and 0 themes. :( That makes testing the full glory of the UI rather difficult in D6. You can cheat by editing the .info files a little bit, playing with the timestamps, etc, but you won't get to see any of the security update warning stuff, since there are no security releases for D6 yet. Other than the split for modules vs. themes in a few key places, this is mostly the identical code from the 5.x-2.0-rc release from contrib. Playing with the UI in D5 will give you a much better sense of what this module does and how it behaves, since it fundamentally depends on having available releases to tell you anything interesting. Note that the CVS deploy 6.x-1.x-dev works fine with HEAD, and should interact nicely with this patch if you're deploying things from CVS.

If you're wondering why anything looks or behaves the way it does, chances are very high there's an issue I recently fixed about it, so before you comment here with "Why does it do foo?!", please look at the Update status issue queue from D5 contrib.

I consider this the culmination of my efforts writing the new[sic] release system, so I'd really love to see this in core for D6. Also, this provides optional anonymous usage reporting to d.o, which will really help with project quality metrics and such. Therefore, I'm marking this critical.

Thanks primarily to merlin and nedjo for the fundamentals, and also to the many others who have helped me over the last few frantic weeks getting 5.x-2.0 to be a thing of beauty. ;)

"update-rtl.css" is missing :-)

I have no good way to test this, but based on my (limited) understanding of the issue, here's a new patch with update-rtl.css and a properly /* LTR */ commented version of update.css.

I made some dummy modules and themes on my laptop for demo'ing the UI differences with theme support. Obviously, there's no release history on d.o about these, so update.module can't tell you much, but this should at least give you a sense of how it looks with themes in the picture. Here's a screenie of the report page. The settings tab will follow next.

1. I started by looking at the screenshot and I'm a bit confused by the "Includes". It seems redundant to me, and in 95% of the cases, it is identical to the module name. Maybe it can be omitted if there is only one include?

2. Technically, the release date is redundant too.

3. What is a "filedate" (first module) and why does its absence trigger a warning? I don't understand what this means, or why it matters.

I'll look at the code next! Looks good so far! :)

1. I started by looking at the screenshot and I'm a bit confused by the "Includes". It seems redundant to me, and in 95% of the cases, it is identical to the module name. Maybe it can be omitted if there is only one include?

I tried this and didn't like the way it looked.

2. Technically, the release date is redundant too.

It is? I don't follow how it's redundant. I suppose it's moderately redundant in one case, where you have a -dev version that's current.

3. What is a "filedate" (first module) and why does its absence trigger a warning? I don't understand what this means, or why it matters.

When running a -dev version, the only way it can tell whether or not your release is out of date is by the filedate in the .info file in the archive. Figuring out how to communicate this is a bit difficult.

new patch to fix some minor bugs on the status report page (admin/logs/status):

• if the cache of available update data is was empty, we generated a 1/2 complete status report line that triggered a notice.
• if the site has no contribs at all, we no longer generate a status report line about contrib. ;)

re: Includes: yeah, it is redundant most of the time, but it's important when a project includes multiple modules or themes, and i'm not sure it works better omitting it when empty. i'll hack up a screenshot of that in a little bit to compare.

re: release date, i think it's valuable info, and would prefer to leave it in. especially since update.module is smart enough (given enough info) to tell you your dev snapshot is stale, based on the dates.

re: the error message when this data isn't there, yeah, it'd be nice if there was a better message. however, this is *only* for CVS deploy people, so most regular drupal users will never see it. i'd like to figure out ways for cvs_deploy to handle this better, but it sounds like you don't like that module anyway, so i'm not sure it's worth the effort. ;)

realized i forgot to attach the screenshot of the settings tab themes vs. modules. unfortunately, the settings won't include lines for projects without release data, since the settings only make sense for projects with available releases. so, you can't actually see how it looks with themes. ;) however, at least you see the divider rows in the table for core and modules, and you can imagine how it'd look with themes in there, too. ;)

Here's a screenie of only printing "Includes: ..." for N>1. Not sure what i think, yet. On one hand, it's certainly nice to remove the space when we don't need it. However, it also makes the report more busy, since more things are crammed in tighter together. The patch to the source for the change in behavior is trivial, but I don't want to maintain 2 versions in the sandbox with/without this change, so I'll just leave it as it is for now, and once we reach a decision on which way we want it, i'll include that in a later iteration of the patch.

minor re-roll to deal with a small translation problem (http://drupal.org/project/issues/122616)

re-roll to use theme('links') instead of hard-coding the link separator.

A minor point: don't use raw $_POST in function update_settings_submit. See also: http://drupal.org/node/153425

Delete that line and change this:

to something like:

or use a separate button-specific submit function.

@pwolanin: Good catch, thanks. I think I just grabbed that straight from system_theme_settings_submit() without even thinking about it, since there used to be no other way to get your op in FAPI. Re-rolled to use $form_state. Nice work finding and reporting #153425, too, those are bugs we should definitely fix after the freeze. Cheers.

Just had a thought about merlin's comment in #30: When running a -dev version, the only way it can tell whether or not your release is out of date is by the filedate in the .info file in the archive. Figuring out how to communicate this is a bit difficult.

Perhaps we should re-use the note field in this case for a more descriptive error message? Please see the 5.x-2.0 screenshot and pay attention to the record for CVS deploy. There's a line "Adminstrator note: ..." that we get from the settings tab if the admin is ignoring the release and left a note. We could do something similar like that for the more obscure error conditions and just print out a little explaination right there. Thoughts?

I haven't tested this lately, so can only offer some comments on the code at this point.

In general, it's looking very good. I'm very impressed with the many improvements that have been added since I last looked at this code.

Comments:

1. Enabled/disabled

Elsewhere in core we usually use   $options = array('1' => t('Enabled'), '0' => t('Disabled'));, so probably we should do the same here.

2. Emulation of system settings form

Maybe a simpler approach would be:

We've used this approach in multidomain module. It avoids having to repeat the system settings form generation and handling code.

3. XML parsing

We now have various XML parsers in core (aggregator, xmlrpc, unicode, openid) each with pretty different implementations (classes vs. functions, etc.). The most generic appears to be drupal_xml_parser_create() in unicode.inc. I vaguely wonder if it would be feasible (and if so, whether it would be desirable) to build this new parser off that function. I also wonder if the parser - used only in the hook_cron() implementation - should be in an include file.

4. Drupal tagging conventions

The approach is obviously very closely tied to our particular CVS tagging and version numbering system, and some of the more complex code blocks are dedicated to handling those particularities. I think that's fine, but it's worth noting that even minor changes in our approach could require some significant reworking of the module.

5. Minor text issues

Drupal contributions, however you

is a run-on sentence, and should be

Drupal contributions. However, you

updates report.  For each project,

should be single space between sentences.

Altogether, admirable work and a valuable addition to core.

Nedjo, thanks for the excellent review and input. New patch addresses all concerns. A few specific replies:

1. Enabled/Disabled -- done. Earl and I find the strings more readable in the code, but it's true that consistency with core is more important. ;)

2. system_settings_form() -- good advice. I didn't know if it was going to be hackier to use it and then unset() stuff, or just emulate. The diff to my sandbox (http://drupal.org/cvs?commit=71806) speaks for itself: lines: +10 -32. ;) However, note that $form['#base'] is now gone from FAPI3: http://drupal.org/node/144132#base So, getting this working was a little different than you suggested, but still better than what I had previously.

3. XML parsing -- All I know is that Dries was pushing hard for this to *not* use the same formatting of XML as XML-RPC (an earlier suggestion to make it easier to re-use the existing core parser), and that he prefered we write our own parser, instead. So that's what happened. And it's been tested via 5.x-2.0 of update_status in contrib, and it works. I'd really rather not mess with that so close to the freeze, and try to re-write it. That said, splitting it into a separate update.fetch.inc file is a great suggestion (along with the rest of the code that's only needed while fetching/parsing the update info).

While I was at it, I moved all the functions for the settings tab into update.settings.inc and everything for the visual rendering of the available updates report into update.report.inc. So, each of these .inc files is around 220-250 lines of code. There's still a ton of code in update.module that should probably be in update.compare.inc or something (about 400 lines by my count). However, it's all used directly via hook_help() and hook_requirements(), so I can't use the menu system's include file handling for this. I wasn't quite sure the cleanest way to deal with this, but perhaps I should just have a trivial wrapper function for the public facing method, update_calculate_project_data(), which does the include_once() itself. Then, at least all the code won't be parsed on every page load, but can still be there whenever update_requirements() is called. I'll take a look at this final refactoring next. Since we only call this function from a few places, perhaps we should just have the callsites do the include_once themselves?

4. Re: Tagging and release conventions -- yeah, that's just reality. There's no way to make this module provide the functionality it does without some assumptions about our version schemes, etc. However, I have almost entirely prevented CVS-specific logic in here (with the one special case of a comparison against 'HEAD', see http://drupal.org/node/151656) and kept that in the CVS deploy module. Most of the logic is probably going to be generically true of any software releases (e.g. versions with "beta" or extra stuff at the end of the string are going to be less stable and shouldn't be as recommended as releases without "extra", etc). And, of course, the logic and code are tied to the actual data in the release history files, and there's no getting around that. ;) Short of a generic, standard langugage for describing releases (which doesn't exist), we need to specialize a little. We chose a relatively simple and sane XML representation of the data, and I'm sure it could be useful for other things besides this particular client code. But, this client depends on the data the server provides, that's just the natue of the beast. ;)

5. Minor text issues -- done, thanks for the careful review. ;)

= New version that splits out comparison-related logic into update.compare.inc.
= Patch now includes a CHANGELOG.txt line about the new module. ;)

Should have included this in my previous comment, but here's the wc (word count) output on the directory with the new layout of .inc files:

IMHO, that's a very clean and even layout, and the update.module file itself is relatively tiny. Without comments, the whole directory is 1184 lines total.

Bah, when converting to system_settings_form() and friends, I forgot to unset() the 'data' and 'avail' #value elements in the form array. We use these for the theme function to generate the per-project settings table, but we definitely don't want them in the {variables} table.

2. Technically, the release date is redundant too.

It is? I don't follow how it's redundant. I suppose it's moderately redundant in one case, where you have a -dev version that's current.

Well, do you know when the latest version of MacOS is released? Do you know when the latest version of VISTA is released? What matters are the release numbers, and not so much the release date. I can't think of a single piece of software where the release date actually mattered to me, or where I had a desire to find out the exact release date.

I'll still have to review the code and test the module. Soon!

The dates are important when doing date-based comparisions, which is what happens for dev snapshots.
If you thought it was better, we could only print the date if we're using it to decide what to say, but I think it's more consistent to always show it if we know it. ;)

Well, do you know when the latest version of MacOS is released? Do you know when the latest version of VISTA is released? What matters are the release numbers, and not so much the release date. I can't think of a single piece of software where the release date actually mattered to me, or where I had a desire to find out the exact release date.

Ahh, but what about Security Update 1384.5? That number doesn't mean much to me, but if it were released on Jun 29, 2007 -- I'd know that it's pretty new. I don't think the release dates are redundant at all. They tell me a lot -- how long it's been since that project had a new release is very valuable information. I actively use that information with this module when maintaining my sites, because it is a hint about how modules I'm using are maintained.

I finally had the time to install the module.

My first impression is that it isn't simple enough. The settings page is too complex and hard to understand:

1. I don't know what "Error threshold" means and I don't know what I'm supposed to set it to. What does it mean for "an update to be marked as an error"?

2. I don't like the level of control I have over ignoring module update notifications. Frankly, I'd like to see us take that out. If you look at _all_ other software, be it Firefox, MacOS, RedHat or whatnot, you never get this level of configuration. This is something that should "just work".

3. The "Allow anonymous usage reporting" setting is full of techno lingo. What is an "anonymous key" and why would I care? I'd rename that feature and rewrite that description to be less geeky. Also, how can this work? The site is fetching information from drupal.org -- it's pretty clear that we can gather information about that site.

I know this may sound extreme, but I vote to almost completely eliminate the settings page. It's a good example of what make people think that Drupal is too complex. We should use Firefox and MacOS as an example here and keep it simple. I don't think I'll commit this patch unless it gets drastically simplified.

I've given it some more thought, and I decided NOT to accept this patch/module as is. Specifically, I want the _entire_ settings page to be eliminated for the core module. Feel free to maintain an "Advanced update notification module" in contrib, but for for core, removing these settings is a must.

I know this sounds harsh, and I know we risk debating about this for days, but I can almost guarantee you that it would be a moot point and we have no time to lose. It's only 24 hours to the code freeze. So let's just remove the settings, drastically clean up the module, and get this in core.

As I said, I won't commit the full-blown version so let's not argue about that (too much).

1. i think error threshold is a geek setting today... i think it is the try to get some requests about email notification closed.

2. i can say from the half year using update_status, i like this helpful feature... it stops the many warnings / emails for modules i don't like or cannot be updated. Maybe i'm running a patched version of a module that contains code, not in the module yet (or maybe never get in) and i should be able to ignore such updates.

3. it's really a fake... the ip packets cannot be without an IP... it's more about accuracy :-). somewhat senseless...

Without a settings page we are unable to have an email notification... this is more an administrative job. i'm not sure if it is a good idea to use the site email for this as an example.

2. i can say from the half year using update_status, i like this helpful feature... (...) Maybe i'm running a patched version of a module that contains code, not in the module yet (or maybe never get in) and i should be able to ignore such updates.

+1 - I also need the ability to not be warned about some specific releases, for the reasons hass exposed. Leaving admin notes also quite handy. But I could live happily if these features are only provided by a contrib module - not sure that's possible however.

About email settings : Maybe we can force user 1's email to be used for notifications (_not_ the site email anyway, it belongs to the client, he's not the one that should tell me about releases), but the other settings (on / off, all releases / security releases only) cannot be thrown IMO. If the settings page goes away, they have to live somewhere else.

I also want to push on the importance of displaying dates on both local release and available releases. It _is_ needed when working with cvs / _dev releases, and actually informative in the other cases.

I'd assume that all those values can live on as persistent variables, thus a contrib module could just provide the settings page as a UI for altering the default values.

I will not support removing the settings page.

If Dries is putting his foot down, that's fine. It can stay in contrib.

I should clarify:

In my mind, this feature is a requirement.

Actual use of the update_status.module has shown that there are often instances when I do not wish to update a module.

1) Some contrib authors may release modules that break, and I either have to downgrade or I know better than to upgrade. I don't want this to show up as an error condition on my site.
2) I may have had to hack a module to get it to do what I want. At that point, upgrades may become difficult or impossible. I don't want that to show up as an error condition on my site.
3) Some contrib updates may be quite drastic, and it might take some time to prepare to actually do it. Again, I don't want this to turn up as an error condition on my site.

I realize these arguments will not sway you, Dries. I don't care.

People don't think Drupal is too complicated because a module has a complex settings page with a lot of fields that aren't immediately obvious. Users get over that. People think Drupal is too complicated because they cannot tell what's happening when interactions are hidden from them, and the results aren't what they expect.

@Dries: I think that the people in favour of the "settings" page (among them myself) have far more experience than you when it comes to setting up real world Drupal sites. This settings page is very convenient to keep some notes and to mark modules as modified (which you sometimes have to do). Without the page the whole idea is more of proof of concept thing which isn't really usable. In the case that the settings page can't get in I'd prefer to have the whole module stay in contrib.

I agree the per-project settings stuff is complex, and proposed to merlin that we rip it out of the contrib version, too. He resisted (for many good reasons), so we kept it it contrib for 5.x-2.0, but I'd be willing to take it out of core. See http://drupal.org/node/154050 for more. However, if it comes out, I'd like to have a chance to implement it in contrib without maintaining a fork of the entire module. That might require a few changes, and I don't necessarily have time to solve the entire problem right now (just got off a plane, etc). So, given this 11th hour requirement, I'd like to publically request an exception for API changes so I have a chance to come up with a solution to this that makes everyone happy. It's not a broad API change, but something very specific to update.module, so the usual arguments about API changes post-freeze shouldn't apply...

That said, I think it'd be a big mistake to remove the settings tab entirely. Dries, most of your complaints are about the specific wording of the descriptions, which can obviously be improved post-freeze if necessary (though I've taken another pass at simplifying the page already).

To be specific, here's why each of the other settings should remain:

E-mail notification

Ideally, we'd have actions in core and update.module would invoke actions when new releases are available. Unfortunately, action's still hadn't landed by the time I was working on this, so that's not a viable option given the impending code freeze. So, I hard-coded simple support for email notification, which many people have requested of the 5.x version. This is important for many site's use case, where a given admin might maintain multiple sites, and not regularly log in to administer them directly. In this case, they'd have no way to know about sites that are out of date without some form of out-of-band notification. This is the most important setting, and the one sites are most often to want to specify, so I've left it as the primary setting on the tab. Hard-coding this to uid 1's email address is inflexible and wrong in many cases, so I refuse to write code for that.

Check for updates

This setting seems completely self-obvious. Given the simple-minded nag-ware implementation of the email notifications, this also controls how often you get emailed if you're out of date.

Error threshold

I'll agree the specific name and description for this needed work, however, the underlying functionality is important (and not complex in the code). Here, we are defending ourselves and our users from contrib maintainers who are not as careful about releases as we'd like them to be. We don't want the available updates report to "cry wolf" so much that people start ignoring it. If they happen to be using modules maintained by people who release too carelessly and too often, we want to provide a way for them to "quiet down" the stream of notifications, and only get pinged for security updates. I don't want people to have to turn off the module completely (and miss security updates) if some modules are generating too much noise. (Side note: this is the motivation for the more fine-grained, per-project settings, but I agree it's too much for core). For now, I've renamed it to "Notification threshold" and given the description some love. If there's a better name, great, let's hear it. We can change this right up until the string freeze if necessary for better usability. But that's no reason to kill this patch.

Allow anonymous usage reporting

Clearly, we want to support opt-out for this. We don't want privacy paranoid folks to have to disable the module completely if they don't want to participate. I will not repeat the implementation details of this feature and how it works, but I'll just remind people that site != IP (multi-site, shared hosting, etc). Read http://drupal.org/node/153741 and http://drupal.org/node/128827 if you care, but that's not what we're debating here. Again, specific suggestions for improved wording are welcome (right up until the string freeze), but we must keep this setting.

Attached is a screenshot of my new proposal, with the following changes:
1) Ripped out per-project settings table.
2) Moved email notification to the top of the page.
3) Put everything else in a collapsed "Advanced settings" fieldset (screenshot shows the fieldset expanded so you can see everything, but it's collapsed by default).
4) Renamed "Error threshold" to "Notification threshold" and improved the description.
5) Simplified the title and description of the "Anonymous usage reporting".

Final point: since we have to keep at least 1 setting (opt-out for usage reporting) and since the others should also stay, then a tab directly off the report is the easiest to find and therefore best for usability IMHO. However, if you prefered a separate settings page in admin/settings/updates or something, we could, but I don't think that's an improvement.

Against my better judgement, here's the patch. It certainly simplifies update.settings.inc (since we no longer need a theme function, yay!), but barely touches the rest of the code. In spite of the complex UI, the implementation of these things is very light-weight and simple, so there wasn't much I could do to "drastically simplify" the module itself.

However, if this approach is being seriously considered, then this patch needs work, for it needs a way to re-implement the per-project stuff in settings.

Either way, I'm just recovering from basically 2 solid/lost days to flight cancelation and airport hell, so I won't be pulling an all-nighter to scramble for last minute changes. Oh, and I just learned that actions did land, but you've got to be joking if you think I can port update.module to actions immediately.

Given the circumstances, we either need a) to commit a patch now but agree to further refine it post freeze, b) agree to an exception/extension for this patch to resolve these last minute changes, or c) I "won't fix" this issue and leave update_status in contrib.

i only hope it does not end with c)... drupal shouldn't be the last cms with this critical feature.

It is hard to know how best to move forward at this point.

This module lived in contrib long enough to accumulate a number of features and UI elements, some of which would not have been added to core, or at least not in the same form. Blithe generalizations like mine above, "I'm very impressed with the many improvements that have been added", aren't exactly to the point, which is: which of these features are appropriate to core? and in what form? On reflection I tend to agree that bringing them all in wholesale is not a good idea. It would be better to get a somewhat basic functionality in and then refine from there, like we generally do when bringing a module into core from contrib. But achieving that looks like a fair bit of work. Doing so completely and cleanly is probably not a realistic expectation in the timeframe remaining.

Possibly an approach would be:

* see if we can get a version with enough removed to achieve the general aim, and apply that.
* do the remaining fixing up in cleanup patches.

dww's proposal in #54 seems like the minimal amount of settings to have the functionnality work in a satisfying way. Whether the settings are located as a tab on the releases page or in the site admin section, I think is more of a usability question that should be allowed to be settled post freeze. Functionally and feature-wise, the patch is ready.

I'd vote +1 for allowing subsequent edits to let a contrib module fine-tune the per project settings. Short of that, I'd probably actually consider using the contrib version of update_releases (should I have to port it myself...) for sites I hand over to clients. You pretty much always end with 1 or 2 hacked modules for which I should at least have a way of leaving a 'do not update this one without letting me know first' message.

I could live with the simplified version of the settings page (http://drupal.org/files/issues/update_simple_settings.png) except for:

1. Remove the "Anonymous posting reporting" setting. Be honest about what data we can collect and how we might use it. Then, just tell people to disable the module if they are concerned with us using that data. People have used Drupal for 6-7 years without a module update notification system. They'll manage just fine without it, if privacy is a concern. Keep it simple!

2. Remove the fieldset and put the remaining settings at the same level.

If people want to see this in Drupal 6, we better start reviewing the code rather than posting "+1"s and "-1"s. It probably needs several rounds of code reviews before this patch can even be considered.

I've already extended the code freeze with one month so I don't think I'll allow any exceptions at this point. I'll think about it though but chances are slim.

Re: "Annonymous usage reporting": guess I have to explain how it works again. When update_status fetches XML history files, if this setting is turned on, it appends 2 query args:

site_key -- md5 hash of the concatenation of the drupal private key and the base_url
version -- the specific version number of that project currently installed

The URLs then become something like this:

With the setting disabled, the URLs are just:

We need sitekey to do meaningful usage stats, since IP != site (shared hosting, multi-site, etc). this unique but anonymous md5 hash "sitekey" is what will allow us to build tables of real data, not rough approximations that won't tell us much. version is also something we can't know without them providing the info. That's the honest truth. I tried to "be honest about what data we collect and how we might use it", and you said the description was too complex, so I made it more simple.

Are you advocating we *always* append these query args if the module is turned on, and if for whatever reason they don't like the idea of these 2 pieces of data being included in their fetch URLs, they have to disable the module entirely? That seems like overkill for removing a relatively simple setting from the page... I really disagree that a settings tab with 4 settings will drive people into fits of "Why is this so complicated, why doesn't it just work?!". If anything, the fact we provide a clear indication of the privacy stuff and an easy way to opt out of the data reporting without turning off the functionality will be greeted with praise, not scorn.

I'm not up for fighting to get this in core. I was under the (apparently mistaken) impression you considered this a killer feature that you really wanted in core. The code is very high quality, clearly commented (perhaps overly so), split up into proper include files, rigorously tested in the real world in contrib, all of my changes have been closely reviewed by merlin and others (see the update_status issue queue if you doubt this). If you don't want to commit it, that's your decision. Unlike some other patches that apparently aren't getting into D6, this can live just fine in contrib for at least another cycle, if not forever.

Wearing my security team hat, I think it'd be great to have this in core so that it's on by default and most sites are using it to alert them about security updates. Wearing my project quality metrics hat, I think it'd be great to have this in core so that we can finally gather some meaningful usage stats for contrib. But, I don't have the time and energy to champion this effort any further.

Are you advocating we *always* append these query args if the module is turned on, and if for whatever reason they don't like the idea of these 2 pieces of data being included in their fetch URLs, they have to disable the module entirely?

Yes, I'm advocating to always append this query data. It's not like we are sending the user's e-mail address or phone number. We're sending a random number that has no value whatsoever.

1. What other software gives you this level of control?
2. What would we be able to learn from this number that a user could possibly have issues with?

Re-rolled with the changes Dries asked for:
- Removed Advanced settings fieldset
- Removed anonymous statistics flag

Tested this on my version and it looks okay: Patch applies cleanly, and the admin/logs/update pages are fine too. Assuming I can do that with my minimal experience here, I'll mark this as ready.

I finally was able to look at the code, and as Derek hinted, we're in really good shape. It's been a while since I've seen code of this quality, and the documentation is simply mind-blowing. I've spent 35 minutes looking at the code and I've no complaints other than:

1. I would rename $avail to $available.
2. drupal_mail() defaults $from to variable_get('site_mail', ini_get('sendmail_from')) so you can safely remove that $form field from your module's code.

That said, I hereby guarantee that this patch will be committed to D6. I'm going to leave this as RTBC for a couple more hours, so people can still make suggestions if they want to. Best patch in weeks, dww and merlinofchaos! Thanks.

Re-rolled with Dries's requests:
- Renamed $avail to $available in all files.
- Removed $from variable from update.fetch.inc.

Reviewed the patch from #56, but got sidetracked with other issues, so here is the review:

- it is great that you thought about the RTL CSS stuff, cool.
- _update_process_info_list() would be great to have a one line comment about its purpose... it looks obvious from the code, but from the api docs, it will not be obvious
- the 'includes' key in the project array is named quite misleadingly... it means 'includes these files' if I understand it right... Shouldn't it be 'files' or something like that?
- update_get_project(): does an info file supposed to contain the project name? if not, why check for this info array key?
- the line break in the elseif looks odd, we don't do these kind of linebreaks, only if we have *very long* conditionals
- the directory guessing would do well, with an example... from the looks, it seems like it could catch 'sites' as the directory for modules in 'sites/all/modules/...'
- sometimes you use $project in place of a project name, sometimes as a project information array, that could be confusing, especially if you don't document return types
- the $avail param on update_calculate_project_data() is both poorly named and poorly documented... although the function is very well documented, the parameter is not
- "Initialize variables needed to find the recommended version." seems to be overdocumentation :)
- t('No filedate available') and t('Invalid info') could be more helpful to the user
- _update_refresh(): I don't think it is Drupal practice to put global (or static) keywords inside a function body
- _update_cron_notify(): drupal_mail() recently changed, although it is so recent that it is not documented yet in the update docs, see mail.inc
- The docs on the constants on t