once the new release system is live (about 4 hours from now *grin*) it'll finally be possible to have the modules page in core somehow notify you if any of the modules you are using are out of date and if new releases are available. i see 2 primary ways this could work:

  1. RSS based: once project.module is providing an RSS feed for all new releases (http://drupal.org/node/94138), it would be possible to have some kind of aggregator block that showed up on your modules page, that subscribed to the feeds for all enabled modules (filtering the feed for releases that match the core releases series you're on (5.x vs. 6.x), ideally filtering based on the major revision of the module you've installed (e.g. if i'm running 5.x-1.3, only tell me if 5.x-1.4 comes out, not if 5.x-2.4 is available).
  2. XML-RPC based: could potentially do more fancy stuff than simple RSS aggregation could, but might be overkill. for example, if it was XML-RPC based, i think it'd be easier to do stuff directly in the module page UI itself (like change the color of rows for modules that are out of date). if a security release was made, the row could become bright red and a huge [SECURITY RISK] icon could appear, etc... this would obviously require some project.module modifications to provide the XML-RPC server side that ran on d.o (which would be a separate issue, but i don't want to file that unless it looks like XML-RPC is considered a good approach to this one).

to be perfectly honest, i don't yet know enough about either RSS feeds or XML-RPC to have a strong opinion on which way is best, or if there are other, better options i'm not even considering. i'd love some input from others about this...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

merlinofchaos’s picture

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):

function blogapi_xmlrpc() {
  return array(
    array(
      'blogger.getUsersBlogs',
      'blogapi_blogger_get_users_blogs',
      array('array', 'string', 'string', 'string'),
      t('Returns a list of weblogs to which an author has posting privileges.')),
    array(
      'blogger.getUserInfo',
      'blogapi_blogger_get_user_info',
      array('struct', 'string', 'string', 'string'),
      t('Returns information about an author in the system.')),
    ...

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

It's very very simple.

dww’s picture

Version: x.y.z » 6.x-dev

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

merlinofchaos’s picture

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

nedjo’s picture

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?

dww’s picture

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

dww’s picture

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

nedjo’s picture

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.

dww’s picture

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.

moonray’s picture

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.

merlinofchaos’s picture

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.

nedjo’s picture

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.

moshe weitzman’s picture

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):

  [releases]
      [release]x.y.z[/release]
      [release]a.b.c[/release]
  [/releases]
  [announcements]
    [announcement]
      [date]
      [title]
      [body]
      [author]
   [/announcement]
  [/announcements]
moshe weitzman’s picture

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.

Paul Natsuo Kishimoto’s picture

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?

jacauc’s picture

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.

moshe weitzman’s picture

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

dww’s picture

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

NancyDru’s picture

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.

merlinofchaos’s picture

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.

JohnAlbin’s picture

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?

dww’s picture

Assigned: Unassigned » dww
Status: Active » Postponed

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

dman’s picture

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?

Paul Natsuo Kishimoto’s picture

FileSize
68.38 KB

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.

/**
 * Module List Generation Script
 *
 * Generate a Drupal module list containing project-release information for all
 * modules with official (non-beta) releases for a given Drupal major version.
 * The list is gzipped and served as a download. The PEAR packages HTTP_Request
 * and HTTP_Download (and any dependencies) are required. Developed using PHP
 * 5.2.2.
 */

require_once 'HTTP/Request.php';
require_once 'HTTP/Download.php';

// Use '5' or '4.7'
$major_version = '5';

$req = new HTTP_Request('http://drupal.org/project/Modules/name');

// Retrieve modules with 5.x-#.# releases
if (!PEAR::isError($req->sendRequest())) {
  $pattern = '@http://ftp.osuosl.org/pub/drupal/files/projects/([a-z_]*)-'.
    $major_version. '.x-[0-9].[0-9].tar.gz@';
  preg_match_all($pattern, $req->getResponseBody(), $modules);
}

echo count($modules[1]) .' with non-dev 5.x releases<br/>';

// Empty temporary file for module list
file_put_contents(sys_get_temp_dir() .'/module-list.xml', '');

// Read project-release information into module list
foreach ($modules[1] as $module) {
  $req->setURL('http://drupal.org/project/release-history/'. $module .'/'. $major_version .'.x');
  $req->sendRequest();
  $size += file_put_contents(sys_get_temp_dir() .'/module-list.xml',
    $req->getResponseBody(), FILE_APPEND);
}

unset($modules);

echo 'Uncompressed module list: '. $size .' bytes<br/>';

// Re-read and gzip module list
$list = file_get_contents(sys_get_temp_dir() .'/module-list.xml');
$gzdata = gzencode($list, 9);

// Discard raw list
unlink(sys_get_temp_dir() .'/module-list.xml');
unset($list);

echo 'Compressed module list: '. strlen($gzdata) .' bytes<br/>';
echo 'Serving download...';

// Serve compressed package list
$dl = new HTTP_Download();
$dl->setData($gzdata);
$dl->setContentType('application/x-gzip');
$dl->setContentDisposition(HTTP_DOWNLOAD_ATTACHMENT, 'module-list-5.x.gz');
$dl->send();

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.

dww’s picture

@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

dww’s picture

Title: module page could say if new releases are available » update.module in core (formerly known as update_status)
Component: system.module » other
Priority: Normal » Critical
Status: Postponed » Needs review
FileSize
52.47 KB

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

hass’s picture

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

dww’s picture

FileSize
53.45 KB

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.

dww’s picture

FileSize
244.17 KB

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.

Dries’s picture

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! :)

merlinofchaos’s picture

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.

dww’s picture

FileSize
53.58 KB

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

dww’s picture

FileSize
165.84 KB

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

dww’s picture

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.

dww’s picture

FileSize
53.51 KB

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

dww’s picture

FileSize
53.7 KB

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

pwolanin’s picture

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:

if ($op == t('Reset to defaults')) {

to something like:

if ($form_state['values']['op'] == t('Reset to defaults')) {

or use a separate button-specific submit function.

dww’s picture

FileSize
53.68 KB

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

dww’s picture

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?

nedjo’s picture

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


+      '#default_value' => variable_get('update_usage_stats', 'enabled'),
+      '#options' => array(
+        'enabled' => t('Enabled'),
+        'disabled' => t('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


 * We can't use system_settings_form() here for a few reasons: it forces a
 * certain #theme on the form which we don't want, and it automatically
 * handles all the variables its own way. We need special treatment for the
 * emails and per-project settings, so we emulate system_settings_form() as
 * closely as possible instead of actually using it.

Maybe a simpler approach would be:


function update_settings() {
  $form = array();
  ...
  $form = system_settings_form($form);
  // Don't use the default form handling.
  unset($form['#base']);
  return $form;
}

function update_settings_submit($form, $form_state) {
  // Do custom handling.
  // Unset data already saved through custom handling.
  unset($form_state['whatever']);
  // Save the remaining settings.
  system_settings_form_submit($form, $form_state);
}

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.

dww’s picture

FileSize
54.83 KB

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

dww’s picture

FileSize
56.26 KB

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

dww’s picture

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:

      31      64     538 update-rtl.css
     110     242    1822 update.css
     407    1855   16197 update.compare.inc
     212     673    6792 update.fetch.inc
     223     744    7738 update.report.inc
     233     903    8741 update.settings.inc
     243    1113   10280 update.module
    1459    5594   52108 total

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.

dww’s picture

FileSize
56.34 KB

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.

Dries’s picture

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!

dww’s picture

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

merlinofchaos’s picture

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.

Dries’s picture

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.

Dries’s picture

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

hass’s picture

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.

yched’s picture

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.

pwolanin’s picture

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.

merlinofchaos’s picture

I will not support removing the settings page.

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

merlinofchaos’s picture

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.

killes@www.drop.org’s picture

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

dww’s picture

FileSize
57.75 KB

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.

dww’s picture

FileSize
50.32 KB

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.

hass’s picture

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

nedjo’s picture

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.

yched’s picture

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.

Dries’s picture

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.

dww’s picture

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:

http://updates.drupal.org/release-history/drupal/6.x?site_key=c2a2f67a4dc537f32ebcdb2a72ec2038&version=6.0-dev

With the setting disabled, the URLs are just:

http://updates.drupal.org/release-history/drupal/6.x

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.

Dries’s picture

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?

webchick’s picture

FileSize
48.55 KB

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

cburschka’s picture

Status: Needs review » Reviewed & tested by the community

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.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

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.

webchick’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
48.64 KB

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

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

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 top of update.module could use some conversion to phpdoc syntax, so the API docs will pick them up...
- hook_help() recently changed, the update docs have information about it, it is basically just a $section to $path rename and a new $arg array param, which you won't use here
- update_menu(): I would name 'admin/logs/updates/force-check' just 'admin/logs/updates/check' instead, that is a verb
- update_requirements(): links in help texts are done with t('See the <a href="!available_updates">available updates</a> page for more information.', array('!available_updates' => url('admin/logs/updates'))), so the translator can go translate the text in line if it needs to be different in this context in the given language. It is also visible that it is a link. This happens in _update_no_data() too.
- t('%emails are not valid e-mail addresses.', array('%email' => implode(', ', $invalid))) should have '%emails'
- seems like a CHANGELOG diff is missing

The code comments seem to be really great and to the point. The complicated processes are explained. The very deep array structures sometimes look quite scary, but given how much information this module organizes, I can hardly think about simplifying them. The above comments are mostly cosmetic and recent Drupal API change update requests.

Let me join Dries in thanking for this high quality code!

merlinofchaos’s picture

- 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?
It includes either themes or modules. It's not really 'files' since a module can be multiple files.

- update_get_project(): does an info file supposed to contain the project name? if not, why check for this info array key?
The project name is added by project.module -- it may not exist if using a CVS checkout; this guessing was a major improvement.

- 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/...'
The directory guessing should stop when it sees 'modules' unless I messed something up. So it wouldn't go up to the 'sites'.

- t('No filedate available') and t('Invalid info') could be more helpful to the user
That's a tough one. Alternate wording suggestions that are concise?

webchick’s picture

I will try and address all of these after supper. The main one I'm worried about is:

- 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 obvious solution is to name all the arrays $project_info and all the names $project_name. However, it's very error-prone to do this manually and I'm not sure how to script it intelligently. I don't want to break the patch. :( I'll try my best though!

Gábor Hojtsy’s picture

It is only a few places where the project name is used, so that should be safe to rename to $project_name, and leave project arrays as $project (which often come out of the $projects array, so that is fine :)

webchick’s picture

FileSize
49.46 KB

Here's the first batch:

- Added CHANGELOG.txt entry. Removed the Deletion API one while I was there.
- Fixed %email => %emails
- Fixed t() link formatting in various places. Also fixed a !site_name => @site_name (this is admin-entered text) I found as I was going through.
- Renamed admin/update/force-check to admin/update/check, and renamed update_force_status to update_manual_status, since everywhere else it's referred to as "manually update".
- Fixed hook_help().
- PHPDoced the constants.
- Changed $project to $project_name inside the update_get_project function. Also renamed the function update_get_project to update_get_project_name.
- Removed the comment // Initialize variables needed to find the recommended version.
- added one-liner description to _update_process_info_list: Populate an array of project data.
- Changed $avail to $available (as of the previous patch) and changed its description in the PHPDoc to: Array of data about available project releases, including both recommended and latest versions.

- the line break in the elseif looks odd, we don't do these kind of linebreaks, only if we have *very long* conditionals

I couldn't find this one?

- _update_refresh(): I don't think it is Drupal practice to put global (or static) keywords inside a function body

I think $base_url is an exception; it's used elsewhere in core too (node_feed, filter_filter_tips, etc.) Unless I misunderstood this point?

I'm out for another hour or two; if someone could pick up the last stragglers, that would be awesome!

dww’s picture

a) Thanks for the encouraging words about code quality, etc. The considerable time I've spent in the last month on this seems to have paid off. ;)

b) Thanks to webchick for spending some time on the cosmetic stuff attempting to get this in.

c) I'm worried that neither Dries nor Gabor has replied to my request for a way to re-implement per-project settings in contrib without having to maintain an entire fork of the code. :( I don't want to rush this, and as you can tell from the work in this module/patch, I abhor hacks and really must see this done right. I'd be really upset if this goes in now, and then "sorry, it's post freeze" is the answer when I post a follow-up issue to address this.

d) Likewise with actions integration. Sorry my actions-related terminology is weak, but it'd be simply wonderful if when update.module finds your site out of date, that was an "action point" that you could assign actions to. It'll take a little while to get that done right, but given how late actions went in, I think it'd be reasonable to make an exception for this.

I'd hate to find myself opposing this patch getting committed for fear that these things will be impossible to fix in the next week or so. At the same time, it's unrealistic for me to solve them all right now (I really shouldn't even be online, I'm supposed to be spending some rare time with my mom)...

Since Dries already said he'd "guarantee" the patch was going into D6, can I get a similar guarantee that you'll let me fix these lingering problems in a sane, clean, careful way in the near future? If so, I can sleep easily again. ;)

Thanks,
-Derek

p.s. My other contribs need me. I don't want to maintain a fork of update_status in contrib for D6. A tiny module to alter the settings tab for the per-project stuff is one thing, a whole module is quite another. Please don't consider that a viable option...

webchick’s picture

Status: Needs work » Needs review
FileSize
49.48 KB

Ok, chx helped me out with the two above and those are now addressed... moved global and include in that function to the top, and moved the else split on two lines to one.

- Also renamed 'includes' to 'sub_projects' (internally only; the UI still says "Includes" because the UI doesn't explain anywhere that these are projects, so "Sub-projects" would be confusing).
- Fixed the drupal_mail call (I think). It is now:

        drupal_mail('update_status', 'notify', $target, $subject, $body);

- Changed filedate error to: "Unknown release date" ... needs a second set of eyes, as I'm not 100% sure that's what that error means.

Sorry... I have no idea what to change "Invalid info" to. It's the default error if it doesn't catch on any of the other ones. I left it for now.

I think Earl addressed the rest... marking CNR because it could probably use a set of eyes on all the cosmetic fixes.

I'd fully support dww's two remaining concerns being addressed post-code freeze. The vast majority of the "API change" type of thing is in this patch; the other two could conceivably be considered "clean-up" after the fact (particularly actions integration). Addressing those two issues would give us the best of all worlds; core would supply a very simple interface for the vast majority of users, but "power" users could have an advanced_update.module (or whatever) which added the logging capabilities, etc.

dww’s picture

My only concern with "Unknown release date" is that it sort of implies we don't know the date of the newly available release. It really means "We don't know the date of what you're currently running". That said, we can certainly change this UI text for usability post-freeze...

Re: "Includes", "sub-projects", etc. The help hook on the available updates report most certainly does explain this:

function update_help($path, $arg) {
  switch ($path) {
    case 'admin/logs/updates':
      return '<p>'. t('Here you can find information about available updates for your installed modules and themes. Note that each module or theme is part of a "project", which may or may not have the same name, and might include multiple modules or themes within it.') .'</p>';

But, I still think "Includes:" is best in the UI. Personally, I think it's cleaner if the code matches the UI for stuff like this, but if people think "includes" is too vague for variable names, etc, so be it.

p.s. The confusion has already begun: http://drupal.org/node/156013 -- are we sure we want to call this the "update" module when it doesn't do updates? ;) *sigh*

dww’s picture

FileSize
50.44 KB

Thinking about this more, I really don't like "sub_projects" in the code. This is a step backwards in clarity.

a) These aren't subprojects at all. It just says what modules or themes are included in the project. Most of the time, there's only one thing in here. We wouldn't call the "panels" module a "sub project" of the "panels" project. That makes no sense. It's just that the panels project includes a module called "panels". In the case of the "views" project, it includes two modules: "views.module" and "views_ui.module". I don't believe anyone thinks of these as "subprojects" of views. It's just that views happens to include 2 modules.

b) The UI says "Includes". The CSS class is "includes". Why should the variable name be "sub_projects"? That kind of inconsistency is far worse for readability and maintainability, IMHO.

So, here's a patch identical to webchick's latest, except it restores "includes" as the array key for this particular bit of data. Keep in mind, this is a nested array, within an array of per-project data. So, the array has stuff like 'name', 'info', 'project_type', 'datestamp', etc. Having an attribute in there called 'includes' seems plenty clear and self-documenting to me. I honestly think this is better all the way around, but I guess I'll leave it to Dries to make the final call.

Otherwise, again, many thanks to webchick for helping in the final hours of this issue.

dww’s picture

FileSize
50.66 KB

Not sure this is going to fly, but here's a relatively simple approach to implementing per-project settings in contrib... introduce hook_update_status_alter(). ;)

At the end of update_calculate_project_data(), right before we're going to return the calculated array of project status info, we invoke an alter hook using a 1-line call to drupal_alter(). This should be all we need, since update_calculate_project_data() is what's called by everything else (available updates report, hook_requirements(), etc, etc). So, even if update.module itself thought a module was out of date, if a contrib implemented this alter hook, it could modify the array to change the status and reason of that particular project, and we could very elegantly ignore specific projects or specific recommended releases, just like we can now in update_status.module.

I just tried this on my local test site as a proof of concept, by having the cvs_deploy module always mark itself ignored on the status report (not that you'd really do this, but to convince myself this is all we need). Lo, it worked like a charm, so if this goes in, I'm happy about our needs for flexibility and power in contrib. Note: this array we'd be altering also includes the recommended, latest, also available, etc, etc, so there's a) plenty of context to do exactly what update_status does now (ignore a specific recommended release) *and* we could even do more complicated stuff like change what's being recommended, completely remove the "Latest" or "Also available" suggestions, etc, etc.

Of course, it could get messy if other modules start trying to alter this, so perhaps a generic alter hook isn't ideal. However, we rarely have core conditionally check for a contrib, so this seems worse:

if (module_exists('advanced_update') {
  advanced_update_status_alter($projects);
}

That said, I know Dries hasn't been thrilled with hook_system_info_alter(), so he probably considers this a special-case hack, too. :( But, I just wanted to post it now as 1 possible solution to my concerns about this.

Of course, actions-integration will still have to wait. That's a much bigger undertaking...

dww’s picture

Sorry, one last comment then I'm going to sleep. Originally when Dries was complaining about the complexity of the settings tab and this per-project stuff, he kept saying "Let's look at MacOS or Firefox -- it should just work."

I want to point out the (IMHO, painfully obvious) fact that Drupal development is *NOTHING* like MacOS development. ;) MacOS's update function tells you about "MacOS core". There's a team of a few bazillion developers, release managers, security auditors, etc, etc, who work on this full time, coordinate releases, and only do so under extremely careful circumstances. When you run "Update software" on your mac, you never have to worry about some clueless plugin maintainer who doesn't understand branches and tags spamming you with 3 new releases in so many days since they couldn't figure out what the hell they were doing. This is a totally unfair comparison. Back to one of my favorite Einstein quotes:

Everything should be as simple as possible, but not simpler.

If all Drupal development was under command control, by a giant company full of professional developers and release wranglers, the "update software" feature could make a lot of assumptions, be more simple, and Just Work(tm).

Of course, that's completely impossible, and I doubt anyone involved in the project wants that. At the same time, Dries has frequently mentioned to me how essential it is that our contributions are hosted via d.o in a centralized way, that it's a huge benefit to the entire project (which I definitely agree with). But you can't have your cake and eat it, too. Given the organic nature of Drupal development, and the incredibly low barrier to people maintaining contribs that are hosted on the d.o infrastructure (and therefore, handled by this UI), the functionality really needs to be more complicated to actually be useful in the real world.

Just had to put this in perspective, to hopefully drive home the need for more functionality here. If it's not going into core, please allow us do this in contrib in a sane way.

Thanks,
-Derek

hass’s picture

@dww: only as a side note... name the required contrib module not "advanced_update", please. A name like "update_advanced" makes clear this is related to "update" module... and i like this naming much more.

dww’s picture

@hass: sure, that name was just an example based on an earlier comment from Dries, and I agree update_[something] would be better. Maybe, even, the D6 "port" of update_status itself should be the advanced settings module? However, this discussion is really unrelated to this issue provided that Dries commits the version with the generic hook instead of a hard-coded check for a single module (patch #76). Then, we're free to name this module whatever makes sense and it'll still work...

webchick’s picture

Just a quick reply to say "renaming back to includes makes sense to me" and "+1 for maintaining the ability for contrib modules to alter the output of update.module" :)

Gábor Hojtsy’s picture

I don't have strong feelings about renaming (or not) of the includes key, although I think it does not look 100% right, I can buy the consistency argument.

The drupal_mail() changes however are not adequate. See the top of mail.inc for detailed overview on how sending a mail works now in Drupal 6.

dww’s picture

I don't understand how to use drupal_mail()'s $language argument in this case. We don't necessarily have Drupal users that correspond with this list of email addresses, so I don't know how to lookup the users's prefered language, etc. Do you advocate I attempt to search {users} based on email, see if I find an account, if so, use their prefered language, else, default to English? Just don't want to spend a bunch of work on this if it's not the direction you want to see. Please let me know ASAP, since I'd love to get this RTBC, committed, and wrapped up...

Thanks,
-Derek

Gábor Hojtsy’s picture

I hoped that the docs on how to select language is cleaner then what it seems to be :) Anyway, we need a docs update for that, if they are not clean:

1. if you have users to send mail to, use user_preferred_language($account)
2. if you don't have users, consider when the code will run... if a person initiates the mail with a web form, them he understands the language the web form was displayed in, so the global $language used to display the page will be fine
3. if the mail is sent with some automated means, ie. on cron or to multiple destinations in the same HTTP request, and the recipients are not users, your best bet is language_default(), which is the site default language (not necessarily English)

For the site-wide contact form for example, where there is a list of email addresses to send email to, the site default language is the best bet. This situation seems to be very similar. Especially that the site admins are expected to understand the language the site is defaulted to.

All-in all you need to consider what language the recipient will understand, and you can use the user preference information (if you have a user), the current page language (global $language), and the site default language (language_default()) as basis to choose.

dww’s picture

Status: Needs review » Needs work

Thanks for the clarification, that helps. Sounds like language_default() is the simplest. Do you want me to spend time on the looking-up-users-based-on-email-to-find-their-prefered language, or is that overkill? I know you're Mr. International, so making you happy is important for this. But, this is a pretty rare case, and I don't want to make the code too complicated, either. ;)

Cheers,
-Derek

p.s. Either way, this needs fixing, and I'll work on a patch, so I'm marking this CNW for now.

dww’s picture

Woe is me. Reading the php docs, looking at user.module's implementation, and the thread where the new drupal_mail() and hook_mail() were added, I'm finding it incredibly difficult to figure out how to get this email sent in the new way. :( Please seem my comments at http://drupal.org/node/82499#comment-268758 (comment #52) about the problems I'm facing in here. I wanted to post in the other issue, since I don't think the problems I'm having are unique to update.module, I think the new drupal_mail() approach is rather fundamentally flawed (at least for programatically generated emails, which appears to be a use-case that never entered the design discussions over there). :(

Again, I'm all for internationalization, but I wonder if we really need more time to get drupal_mail() right. :( Sorry I didn't know about this sooner, this only came up in this issue basically on the eve of the code freeze...

Any advice or input would be greatly appreciated.

Thanks,
-Derek

dww’s picture

Sorry, 1 other question about this relative to the 2 week extention and the freeze. What's the story with actions integration? Honestly, I'd prefer to fix up the email sending (if possible) and then commit this now. Then, work on actions as another issue/patch still within the 2 week deadline. Similarly, we might consider adding some more watchdog() messages in here to take advantage of the new watchdog stuff. I don't want this patch to get more complicated, linger in the queue, etc, and risk not being fully done on time. Once email is fixed, can this go in, and then I open 2 other (much simpler, easier to review/test) issues/patches to address these other 2 final points? Just trying to strategize here. Thanks.

dww’s picture

Status: Needs work » Needs review
FileSize
55.24 KB

After much help from Gabor and Jose over at http://drupal.org/node/82499, I think I finally have a handle on the language-specific email stuff now. ;) Unfortuantely, the results are a more complicated patch, but I *think* this will actually send those notification emails in the right language now. ;)

New patch attached with the following changes:

  • Split out error message texts into a new helper method called _update_message_text(). Depending on what kind of message you want (specified by 2 args, one to indicate core vs. contrib, the other with the status reason constant (UPDATE_NOT_SECURE, etc), this will return the properly translated text, using the optional $language arg.
  • Changed update_requirements() to use the new helper to generate the strings. It doesn't pass a $language, so I believe it'll always display the results in the language of the user viewing the status report. I also added the reason code into the array it returns, since we'll need that in _update_cron_notify(), and since it could be very useful for other modules invoking update_requirements() to see if the site is up to date or not. Added a thorough phpdoc comment to explain the array returned by this function. The good news is this allowed me to get rid of the somewhat ugly (and ambiguously named) $plaintext argument, which we no longer need. Yay. ;)
  • _update_cron_notify() now iterates over the target emails and attempts to user_load() on each one. If that works, we use the user's prefered language code to generate the email, otherwise, we use language_default().
  • Added update_mail() (hook_mail implementation) to construct the right email message given the language and the reason params passed in. This of course relies heavily on _update_message_text(). Also removed wordwrap(), since drupal_mail() now handles that.
  • Fixed an E_NOTICE bug in _update_cron_notify() since we weren't including install.inc before using update_requirements() (and therefore, the REQUIREMENT_* constants).
  • Fixed a bug introduced by webchick in #71. ;) a) site_name is admin entered text and all, but this is being put in an email subject, not displayed on the web, so check_plain() isn't necessary. Furthermore, the change was incomplete, and she had '@!site_name' in the array, not '@site_name', so the subject was always turning up with '@site_name' in there. ;)
  • Resolved conflict on CHANGELOG.txt
  • Added myself to MAINTAINERS.txt for update.module. Not sure if that's what y'all want to do, but I guess I know this code better than anyone at this point, so I'm volunteering if that's desired. Feel free to not commit that part if for whatever reason you don't think it's a good idea.

I've tested pretty heavily by hacking my copy of drupal_mail_send() to dvm() to the screen and return, since my current test site can't send emails, and I haven't had a chance to setup another. It'd also take me a while to figure out how to set it up as a multi-language site and test notification emails for users in different languages, etc.

So, this definitely needs a close review by Gabor or someone else who fully groks the language-dependent email stuff, just to make sure it's conceptually sound and that it matches the expectations for the new email API. After that, it could probably use a little testing on an email-enabled site to make sure that the emails actually go out (though visually, the $message array that's there at the time my test site hits drupal_mail_send() looks fine, so I have no reason to doubt the emails are happy, assuming I got all the language stuff right).

Thanks,
-Derek

p.s. This came up in the language thread, but I wanted to ask in here -- should we invoke the notification stuff even when you manually check for updates? I was assuming no, since if an admin is manually checking, they probably don't need the emails to go out. However, the emails could be going to other folks, all of whom might want to know the site is out of date. Given the above refactoring for language, I don't think it'd matter at all in terms of language correctness if we just renamed this and called it directly from _update_refresh() instead of update_cron(). Thoughts?

p.p.s. Still wondering about actions support in a follow-up patch, adding extra watchdog() to _update_cron_notify(), and confirmation that hook_update_status_alter() is ok. I guess those all need Dries's decision...

hass’s picture

For sure don't send an email on a manually forced check, please. :-)

dww’s picture

So there's no misunderstanding, the emails are only sent if the site is in "error", i.e. there are security updates missing, or there are new releases and the "Notification threshold" is set to "All new releases". So, the question is this: if you're doing a manual check, and there's a security update missing, should that still trigger the emails, or only if the security update is first noticed by cron, not a human?

hass’s picture

I think we should only send emails if cron fires an error... why sending an email if you sitting in front of the page and you see the errors with your eyes on a *forced* check? This makes no big sense from my point of view. I would feel shirty if i'm getting an email additional to what i already see. sounds like another senseless email in my inbox...

Gábor Hojtsy’s picture

Hass, many sites employ more then one administrator. Only one administrator at a time can view the page manually, so the other administrators are not notified. The question is if this is a good thing or not.

hass’s picture

well - i know, but if i press 10 times the force check button i spam all admins... i cannot remember there is any email blocking (untested) - per email address, per module and per error type - logic in the code. So every check i get the warning/error email again. Maybe there have been something build in since my last review... not sure.

Dries’s picture

I'm with hass. Manual checks should not send e-mails, IMO.

dww’s picture

@Dries: excellent, me, too. Therefore, #87 is RTB-core-committer-reviewed. ;) It's code-complete, tested, and clean, as far as I'm concerned. Also, please see my questions at the end of that reply. Thanks!

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Reviewed the mail sending code:

- url() also has a 'language' option which should be used when composing emails, the URLs could be different based on what language the user prefers
- _update_message_text() has the $language parameter written as $languate
- _update_message_text() also has an url() call which should have a $langcode
- because you only use $langcode in _update_message_text(), I would use that parameter directly, which would save you the first line of the function, and would also simplify the fist line of update_mail() to $langcode = $message['language']->language;

Otherwise mail sending looks good.

dww’s picture

Status: Needs work » Needs review
FileSize
55.3 KB

Thanks for the review, Gabor. New patch:

  • Adds 'language' => $language to the array of params passed to url() in both update_mail() and _update_message_text(). Note: this API change is not documented anywhere in the url() phpdoc. :(
  • Fixes $languate typo. Sorry about that. :(

I don't understand what you mean with your last point in conjunction with your request for passing $language to url(). Reading the code for language_url_rewrite() -- the only docs available -- it needs a full $language object, not just a $langcode. Therefore, both update_mail() and _update_message_text() need both the $langcode (for t()) and the $language (for url()), so I don't see how to further simplify what parameters are being passed where. Please clarify if you still think this is a problem.

Also, even when it was just using $langcode, I found it made for clearer code to still pass the $language object at all the call sites, instead of duplicating the code to check for $language and if it's defined, using $language->language. But, now it doesn't matter, since it needs both $language and $langcode, anyway (at least as far as I can tell from reading the source to these language-related functions).

Thanks!
-Derek

dww’s picture

FileSize
55.32 KB

Re-rolled for new profiles/default/default.profile conflict.

dww’s picture

FileSize
57.79 KB

After playing with this on the plane home, I found a few more problems and made a few enhancements. New patch attached with the following changes:

Bug fixes:

  • Fixed the call to watchdog(), which now takes new arguments in D6.
  • Moved the cache of the available update data into a separate cache table, {cache_update}. See http://drupal.org/node/155450 for background on this if you care. I'm not 100% convinced this is a good idea, but chx seemed to think so when I asked in IRC.
  • Fixed an obscure php notice in _update_refresh() if you're trying to fetch available update data when your site isn't connected to the internet and can't reach updates.drupal.org. ;) Debugging on planes is good for you. ;)
  • Added a watchdog() if we fail to fetch any data.
  • Fixed a minor bug in the interaction of update_refresh(), _update_refresh() and the status report. The report is supposed to automatically attempt to refresh the data if the cache is empty. Unfortunately, the report was not *using* the refreshed data immediately, since update_refresh() wasn't returning the $available array that update_get_available() was expecting. _update_refresh() already returns the array. This is all fixed, so if the cache is empty and you land on the available updates page, it will now fetch and immediately display the results. The only time the report will display its error message is if it failed to fetch for some reason (e.g. site is off the net and you can't reach updates.d.o).
  • Changed the wording of the error message when the site is missing a security update to use a ! for added emphasis:

    There are security updates available for one or more of your modules or themes. To ensure the security of your server, you should update immediately!

  • Realized that theme_update_report() still had hard-coded logic to print out the administrator notes from the per-project settings, which are now moved into contrib. Instead of leaving code in core that's specific to a contrib like this, I modified it to be flexible so that A) we could use it in update.module as a way to provide a more verbose explanation for some of the error cases that aren't particularly clear with only a few words, and B) any contrib that implements hook_update_status_alter() can use it for other things, not just "Administrator notes".

Enhancements:

  • Added yet more nag-ware if the site is missing a security update. In this case, hook_help() generates an error message on every /admin page if the viewing user has 'administer site configuration' perms. ;)
  • Added a variable to control the default fetch URL if you want to override this site-wide without hacking core. There's no UI to set/change this, but this added flexibility would be very handy in some cases, e.g. on sites inside an intranet that use a proxy server for available update data, etc. A UI to configure this variable might make it into the update_advanced.module in D6 contrib (which is where all the per-project settings moved into -- I also wrote this on the plane, and it's working great).

Obviously, any of the above that we don't want to keep for some reason could be easy removed in another reroll if necessary, but I figured I should just include all of it in a single (perhaps final?) patch for this issue, in the hopes that this is truly RTBC at this point...

chx’s picture

FileSize
57.78 KB

There is a lot to be loved here. As I was reading the code (which posed no problems because it's so very throughly documented) I asked myself 'and what about my intranet? It'd be nice if I could use a central server not drupal.org' and yes even that's possible.

I am a bit torn with the XML parser. We do not use class structure anywhere else. However this bites noone, so why not? I even admit hesitantly that it's more elegant because $this is passed automatically, no need to pass around in a global or semiglobal (static in a get/set). It's sooooo enticing to say "we will use SImpleXML here, this is the killer PHP5 feature for Drupal 6". But I guess because this is security we must resist temptation. Ah well.

I made a very minor change: strpos($path, 'admin') !== FALSE is not good because it needs to check for admin in the beginning of the path and even so... so I have used $arg[0] == 'admin'. That's why you get the arg array. There is no other change whatsoever in the patch.

hass’s picture

@Dries: i know we moved the configuration to "ignore projects" out and i'd like to give another example why this settings page is really required (however it can stay in contrib, but i will install immediately together with D6). Many projects like "update_status", "views" and so on offers a "release-candidate" and "beta" as the "official release". This in mind will end with an "error" and update notification in update.module and i really don't like to install release candidates and beta's in production! I'm not sure if dww and merlin only testing update_status with this releases... It's nice to know about such additional releases that are not final, but they shouldn't be the "official" release... but this is up the maintainer and i may loose control about only have finals on my production site only to remove the annoying emails and warnings.

I think there should be a policy that no-one releases beta and rc's as official release.

dww’s picture

FileSize
58.11 KB

@chx: Thanks for the careful review, kind words, and good suggestion for hook_help(). ;)

@hass: Please, this is not the place for this debate. Yes, it's yet more evidence to support the need for hook_update_status_alter() to remain in this patch, but let's not side-track things too much.

@all: Tee hee, I forgot about the upgrade path. ;) Namely, if you've got a D5 site, and you upgrade to D6, we still need to install the new schema (just for {cache_update}) and enable the module. Attached patch includes system_update_6026() to do these 2 tasks. Also, I found a tiny bit of dead code in update_get_project_name(), which I also removed.

p.s. Isn't it strange that we handle all of this via system.install instead of update.install? Seems like it'd be cleaner if we did all of this via update.install, and the only thing that had to be in system.install was the upgrade path update to install the module (at which point, hook_install() would handle the rest). However, that's not how anything else works in core, so I'm doing it the "usual" way here...

dww’s picture

FileSize
58.61 KB

New patch:
- Moved all schema installation stuff into update.install.
- Removed the part from system_install() to manually create the update schema.
- Simplified system_update_2026() so it *just* installs the module, and lets hook_install() handle the schema.
- Added update_uninstall() while I was at it.

Tested:
* D5 -> D6 upgrade path.
* Fresh D6 install.
* Playing with D6 then uninstalling.
All is working great.

This seems much cleaner than making the update schema system.install's responsibility...

Gábor Hojtsy’s picture

The only problem with this kind of core reuse in update functions is that it reults in some uncertainty about what is actually in the database. Think:

- a D5 is updated to D 6.0
- a D5 is updated to D 6.1

If the update table (cache table) schema changes between Drupal 6.0 and 6.1 (not really likely), although a site could go from the D5 state to the same upgrade function, the database would not be the same. Later update functions, which think the database is in a certain state (and add or modify columns) will find the database in an inconsistent change.

This is not especially likely with the cache tables (but not entirely impossible). This kind of code reuse in update functions is not good practice especially because of this problem, and this is documented in the schema API docs: http://drupal.org/node/150220.

dww’s picture

FileSize
59.76 KB

New patch that hard-codes the particular version of the schema we want for now in system_update_6026(), and then uses module_enable() to install update.module without touching update_install(). Everything else unchanged.

bjaspan’s picture

dww asked for my feedback on the patch in #104.

1. system_update_6026() looks fine by itself. That is the right way to create a table in an update hook.

2. In this case, however, system.install is creating a table that is declared in update.schema. If update_install() runs before system_update_6026(), then system_update_6026() will get a "table exists" error, and vice versa. I don't know enough (read: anything) about update.module to understand the "hypothetical cases" in which the normal install mechanism would leave the db in an inconsistent state so I don't know what this approach was used. If this approach is necessary, perhaps if (!db_table_exists()) should be wrapped around the calls to db_create_table(). This will be a little bit of a pain in update_install() since it will no longer be able to just call drupal_install_schema().

dww’s picture

@bjaspan: the hypothetical case I refer to is the one Gabor mentions in #103. Namely: the schema for {cache_update} wants to change between 6.0 and 6.1. So, if someone upgrades directly from D5 -> D6.1, when they hit system_update_6026() they'll get the modified (6.1) version of the cache table, but then system_update_6101() will fail when it tries to alter {cache_update} from the 6.0 schema to the 6.1 schema.

However, there's no way for update_install() to fire before system_update_6026(). Unless someone goes *really* out of their way to break us intentionally, and I'm not bloating core to defend ourselves from that. Either it's a fresh D6 install, and update_install() fires first (and then system_update_6026() will never run, since the core schema version will be 6026 or higher), or, it's an upgrade from a D5 site, in which case nothing will work unless they run update.php first. Since update.php ensures that we hard-wire the 6.0 schema for {cache_update}, and then enable the module without calling update_install(), there seems to be no sane way to trigger both update_6026() and update_install().

Hope that clarifies.

bjaspan’s picture

@dww: I realized while attending a birthday party for a friend's 3-year-old today that system_update_6026 has the problem you describe in the first paragraph of #106: You are using a relative schema (a.k.a. "The Schema API No-No") so no future system_update hook can know for sure exactly what cache_update looks like.

It looks like your second paragraph tries to explain why that isn't a problem in this case. I'm too tired to decide if you are right (imagine a house full of a screaming kids for an afternoon :-). If this is a case in which it really is safe to use someone's hook_schema in an update function, you should copy the explanation from #106 into a comment in the file.

dww’s picture

FileSize
60.18 KB

@bjaspan: huh? ;) There's no relative schema in patch #104:

function system_update_6026() {
  $ret = array();
  $schema['cache_update'] = array(
    'fields' => array(
      'cid'        => array('type' => 'varchar', 'length' => 255, 'not null' => TRUE, 'default' => ''),
      'data'       => array('type' => 'blob', 'not null' => FALSE, 'size' => 'big'),
      'expire'     => array('type' => 'int', 'not null' => TRUE, 'default' => 0),
      'created'    => array('type' => 'int', 'not null' => TRUE, 'default' => 0),
      'headers'    => array('type' => 'text', 'not null' => FALSE),
      'serialized' => array('type' => 'int', 'size' => 'small', 'not null' => TRUE, 'default' => 0),
    ),
    'indexes' => array('expire' => array('expire')),
    'primary key' => array('cid'),
  );
  db_create_table($ret, 'cache_update', $schema['cache_update']);
  module_enable(array('update'));
  menu_rebuild();
  return $ret;
}

It's explicitly defining exactly the schema to use for creating the {cache_update} table, just like the docs suggest... I don't understand what you mean with comment #107. Maybe you were looking at my patch in #102, instead?

Anyway, here's a new patch that hopefully clarifies the situation in the comment for sytem_update_6026(). Hope this helps. Of course, we can always tweak the wording of this comment later, so please don't let disagreement over the text hold up the patch. ;)

Thanks,
-Derek

dww’s picture

p.s. All this grief, just so that we have a separate cache table, instead of storing our fetched data about available updates in {cache}... :( Are we sure that's a good idea? Perhaps we should go back to just using {cache}, and then *all* we have to worry about in update 6026() is a simple call to module_enable(). ;) Tempting, huh? Can one of the core committers please comment on this point? I don't want to maintain 2 copies of the patch, so let's just decide ASAP which way we want to go. (I suppose since I've already solved all the grief, it's better to leave things as they are in patch #108, but I wanted to raise the question)...

Thanks,
-Derek

hass’s picture

I'v seen this error on my custom module checking project status on my test box.
notice: Undefined index: tag in C:\Inetpub\wwwroot\drupal6\modules\update\update.fetch.inc on line 203.

Please don't change the release history url "every day" :-). All i've figured out in the last weeks was /release-history rewrites to project/release-history, /files/release-history without rewrite, /release-history rewrites to /files/release-history and now /release-history rewrites to /project/release-history. Sometimes with the requirement to execute project-release-history.php and now it's not required what's best.

However except the above error i haven't found any problems.

hass’s picture

Overseen one bug... i released a module version is 6.x-1.4 and update.module found this release as 1.4. Installed is 6.x-1.4. Now Update module tells me in big red - there is a update... what's wrong?

hass’s picture

Solved the last comment myself... looks like there is an undocumented change in the "version" field of .info files, that should only contain the version number themself and no more the core in front... good change... but undocumented :-(

hass’s picture

update.fetch.inc on line 203 is line 202 in your code...

hass’s picture

looked into chameleon.info and found a line version = "6.x-dev". someone able to clarify if i'm wrong in #111 about this change!? if so there looks like a bug in update module.

hass’s picture

Status: Needs review » Needs work

Bug in #110: this happens if "project_release_nodes.tag" is left blank, what is the situation if i release with the file upload dialog in project. Adding something to this field, re-execute project-release-history.php removes this annoying error.

Bug <tag/>: Seems like the source if this problem is the project module. It's really scaring what this module does and the version 0.x-dev is really the right version number for this piece of code... While comparing the XML output on my local project installation and the one on d.o i found the difference in the version string and corrected this by hand. re-execute project-release-history.php again and now the version check works.

Gábor Hojtsy’s picture

Derek, I don't think either, that Barry looked at the latest patch. It would be good to have bjaspan's reexamination.

bjaspan’s picture

dww: Re #108, sorry, like I said, it was late. Now it is morning but I am still bleary-eyed. Anyway, I meant that update_schema() is relative:

function update_schema() {
  $schema['cache_update'] = drupal_get_schema_unprocessed('system', 'cache');
  return $schema;
}

Thinking out loud... What can update_update_1() assume about the structure of cache_update? If the schema for {cache} changes, update_update_1() won't know which version it has to deal with. Actually, perhaps it will because core is always released as a monolithic block, so you always have the opportunity to write an update_update_N() function each time the schema for {cache} changes. This is different than with contrib modules where every schema can update individually at any time. So I guess at the moment this seems to be safe, but it would not be in a non-core module, so another comment is warranted to explain why it is.

I am not proposing holding up the patch for these comments. You asked me for schema api-related comments; I still haven't reviewied the whole module/patch.

merlinofchaos’s picture

hass: The .info file change IS documented: http://drupal.org/node/101009

dww’s picture

Status: Needs work » Needs review

@hass: Which part of "this is a work in progress" do you continue to fail to understand? "Please, don't make changes" is about the least productive thing you could possibly "contribute" to this thread.

dww’s picture

@bjaspan: Gabor thought it was a feature that the install always used the relative schema. I think we're totally safe here, since if you're installing clean, you'll always get the latest core schema for {cache} tables, and once you install, the schema version will be recorded in {system} as the highest available update for all installed modules. So, even if this were in contrib, and you were adding your own {cache_foo} table, your .schema could still do the exact same trick, and you'd just have to write foo_update_N() to modify the schema of {cache_foo} anytime core's {cache} schema changes. Yes, that's a slight pain, but things are always in a consistent state. If you install before foo_update_N(), you get the old schema and then the update moves you to the new. If you install after foo_update_N(), you get the new schema and foo_update_N() never runs. Either way, everything is deterministic. Just think back to pre-schema API: your hook_install() was always supposed to be the very most recent version of the schema, while hook_update_N() did the incremental changes to track the "diff" in hook_install(). This is just the new, slick way of saying "always grab the most recent version of the schema" in hook_install(), instead of replicating the effort yourself.

I'm not sure that a comment to that effect in the code is necessary, this seems more like a job for the schema API docs. We don't usually comment every call site about a slightly confusing API. ;) This doesn't seem like an obscure usage of the schema API, though I'll grant it's a little bit tricky (thanks to Gabor for forcing me to understand it all). That said, given my love of clear code, I'm happy to add a comment to update_schema() about this if the core committers thought it was a good idea.

But first, I'd love confirmation from Gabor/Dries that they want this separate {cache_update} table in the first place, before I spend another minute on these tasks. ;)

dww’s picture

Oh, @hass -- your "bug", such as it is, is really in project-release-history.php. It shouldn't add <tag></tag> if {project_release_nodes}.tag is blank.

You're trying to setup a parallel version of project* and this whole system to serve up update status info for themes you don't want to host on d.o. That's fine, but frankly, it's your problem. ;)

A) It's not my job to support every last question you have -- I'm getting this infrastructure working on d.o. If you're trying to do it elsewhere, it's up to you to figure it out, and understand the issues you face, instead of raising objections in inappropriate places because you don't know where else to complain.

B) This thread is NOT the place to voice your complaints about how project* is maintained.

C) You're not going to get much support from Dries to hold up this patch to make things work better for your not-on-d.o use case, so if I were you, I wouldn't spend much time trying...

Cheers,
-Derek

hass’s picture

@merlin: yes, i know and i was wrong about the speculation that "version" string in .info have changed. this is not the case - it was another time "project" that have a wrong version format string configured out of the box, i was not aware about. forget this info file questions.

@dww: It's good to know that a blank <tag></tag> is a projects bug. The patch for projects is in http://drupal.org/node/157696. I'm not trying to hold this patch - really not - i want this in, but bugfree! How should i know this and how should i test this update module without any modules upgraded to D6 on Drupal.org repository if not on my own box, where i can alter everything and find update module bugs that may give errors with d.o later? However you think this module is bug free, i try to test it and all i'm trying - is to get my fingers around this code and check it in all possible ways. I'm sorry to find tons of bugs not related to this case that looks related, but i tried to find out where this bug is located. If update module produces an error it's the big question if this may not an update bug. You say it's project, ok - i believe you and created a patch for project. But let me test and report this if i'm not sure what the problem is, please.

dww’s picture

FileSize
60.35 KB

new patch with a few minor usability improvements based on discussion with Dries:

- Changed the wording for the report when the "Notification threshold" is set to "Only security updates", and you have a non-security update available. Now, it just says "Update available", just like it does with the threshold set to "All newer releasese". Now, the only visual difference on the report itself is the color of the row and the icon.

- Removed the "Ignored" part whenever a row is yellow. Dries didn't think it added any useful info. The yellow and /!\ icon is enough to let you know something's not 100% normal, and the reason text tells you why.

- Added a drupal_set_message() whenever you manually check for new data. It even sees if it was successful and prints an error message if you failed to fetch for some reason. Also fixed up the wording of the watchdogs() that get generated for this to match.

- Used format_interval() on the "Last checked:" part of the available updates report.

Also, after further discussion with Earl, he wanted to be listed in MAINTAINERS.txt, too. Hope that's cool.

webchick’s picture

Status: Needs review » Reviewed & tested by the community

I dare to set this RTBC. I've tested it both with and without an Internet connection, fresh install and update path, and all seems well. If there are any outstanding bugs, I'm sure they can be dealt with in subsequent patches.

Dries’s picture

I've briefly talked to Steven about this patch, and he mentioned that it uses query arguments in its URL. These need to be escaped. This e-mail might shed some light on the issue: http://lists.drupal.org/pipermail/development/2007-May/023751.html. I'll try to investigate this more after work, if no one beats me to it. I can't actually look at this patch now to identify these.

webchick’s picture

FileSize
60.35 KB

Here's a simple re-roll that changes all href="!something" to href="@something" (and the t() arguments accordingly).

dww’s picture

Oh, and in case anyone's worried about _update_build_fetch_url(), please don't be:

a) These URLs are never displayed by the module, so arguments about XHTML validation don't apply.

b) They're entirely constructed either via hard-coded data, or stuff from the .info files on the site (and if those are compromised, the site has much bigger problems).

c) If you use a check_url() there or otherwise attempt to encode the & chars in the query string, they're never decoded during drupal_http_request() and the resulting query args at the server are bogus and useless for collecting stats. Just tried this on my local test site to confirm.

So, to the extent that passing around $destination via query args in the UI is a problem, webchick's is all we need (and then some). ;) I've tested the various places a URL is printed out that contains a destination in the query args, and all is working fine with #126. RTBC indeed... ;)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

C-o-m-m-i-t-t-e-d! :)

dww’s picture

Component: other » update.module

WOO HOO! ;)

FYI: people who want the fine-grained, per-project settings should look here: http://drupal.org/project/update_advanced

Also, there's now an "update.module" component in the core issue queue, so any followup changes anyone wants to propose, please DO NOT reopen this issue. Submit a new one and use the "update.module" component.

Thanks, all!
-Derek

dww’s picture

FYI: here's a direct link to the core update.module issues:
http://drupal.org/project/issues/drupal?components=update.module

Where, for example, you'll find the new issue about actions integration... ;)

http://drupal.org/node/158541 (this is in "Concept needs feedback" state for now, please comment there).

Thanks,
-Derek

Anonymous’s picture

Status: Fixed » Closed (fixed)
webchick’s picture

Version: 6.x-dev » 7.x-dev
Category: feature » bug
Status: Closed (fixed) » Active

hook_update_status_alter() was never documented. Please fix.

dww’s picture

Priority: Critical » Normal
Status: Active » Postponed (maintainer needs more info)

@webcheck: Where do you want it documented?

pwolanin’s picture

Status: Postponed (maintainer needs more info) » Fixed
FileSize
2.06 KB

Committed this patch to the 6.x and 7.x developer docs.

Feel free to go in and tweak further - the code is from http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/update_adva...

dww’s picture

Category: bug » task
Status: Fixed » Active

Cool, thanks pwolanin.

Although, I think that particular snippet from update_advanced is actually buggy, due to the joys I discovered over at #295152: Doesn't distinguish between 5.1 and 5.10. So I should fix both update_advanced and that snippet in the docs. ;)

I'll clean this up later today once I get update_status 5.x-2.3 out.

dww’s picture

Category: task » feature
Priority: Normal » Critical
Status: Active » Fixed

Committed a fix to the docs in DRUPAL-6--1 and HEAD based on my patch for update_advanced over at #295152: Doesn't distinguish between 5.1 and 5.10.
Setting this issue back to its original values for posterity.

Anonymous’s picture

Status: Fixed » Closed (fixed)

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