Add hook_update_projects_alter()

replace my_custom_list with a real array and it's ready to go

Well the format needs to be documented

Agreed with dmitrig01. The example could have:

$my_custom_modules = array(
  // a couple custom modules in whatever the format is. 
);
$projects += $my_custom_modules;

or something like that.

I'd also like to see a review from dww on this as to how or if this fits into his larger plans to reduce the RAM footprint of update module at #238950: Meta: update.module RAM consumption.

I don't mind how this is going to impact #238950: Meta: update.module RAM consumption. Either way, we still need a list of all the projects we're going to fetch update data about. #238950 is about doing smarter things with that list. Letting other modules alter the list is a fine proposal, I'm hugely in favor. When I previously wrote that #238950 should be the show-stopper that all other changes should wait on, I was under the impression that was a huge nasty bug, and we were going to be changing APIs in the D6 version, anyway. However, it seems like the RAM usage of update.module is a drop in the bucket compared to other D6 core memory hogs, so it doesn't seem like that's as urgent anymore, and probably won't result in any API-changing D6 backports on its own.

However, this needs work (at least more thought) for another reason...

When update.module fetches update data, if the GET parameters include an anonymous site key, updates.drupal.org counts that as a module in use by that site and that shows up in the usage stats. However, one of the biggest justifications for this alter hook is #162788: Include modules that aren't enabled and as I explained there, we need to make sure we do *not* include the site key when we fetch available updates for disabled modules. While the alter hook itself is fine, we need to include something in the altered data we put in the array to indicate not to use the site key when fetching available update data for disabled modules. So, really, if we're patching update.module to make #162788 possible (the primary point of this issue), we can't just leave it at this alter hook -- we need a slight change to the format of the array we're altering to give users of this hook the control they need.

Basically, we need a tiny change to _update_build_fetch_url() to decide to include the site key in the URL or not based on some other element in the $project array. Something like this:

--- update.fetch.inc    14 Mar 2009 23:01:37 -0000  1.16
+++ update.fetch.inc    27 Apr 2009 05:59:54 -0000
@@ -84,7 +84,7 @@ function _update_build_fetch_url($projec
   $name = $project['name'];
   $url = $project['info']['project status url'];
   $url .= '/' . $name . '/' . DRUPAL_CORE_COMPATIBILITY;
-  if (!empty($site_key)) {
+  if (!empty($site_key) && empty($project['skip_site_key'])) {
     $url .= (strpos($url, '?') === TRUE) ? '&' : '?';
     $url .= 'site_key=';
     $url .= drupal_urlencode($site_key);

That's an utter hack, but that's the basic idea. Make sense? If we name/treat this attribute positively ("use_site_key") instead of the negative I put in the above example ("skip_site_key"), it's a slightly bigger change since we need update.module to set use_site_key = TRUE for all enabled modules it's managing itself. That also takes slightly more RAM as opposed to only storing the "hide this" attribute for the projects we're trying to hide...

Anyway, _technically_ that isn't a problem with the alter hook itself, but I think it only makes sense to commit the alter hook along with a change like this to _update_build_fetch_url(), so we might as well do both in the same issue/patch...

@Dave Reid: Oh right, I forgot I already added quasi support for those at #162788-8: Include modules that aren't enabled. ;) (edited to fix this link)

However, please do that test directly in _update_build_fetch_url() like my example in #6, instead of at that particular call site. Then you get my +1. ;)

Thanks!
-Derek

Ugh, sorry. :( Wrong issue nid. I meant: #162788-8: Include modules that aren't enabled.

Untested, but something like so. Leaving needs work for the API doc improvements from #5...

Note, this patch conflicts with #220592-33: Core cache API breaks update.module: fetches data way too often, kills site performance, etc, which is frankly more important... Not sure the best way to proceed, other than to roll patches for this against the patched copy of update.compare.inc. The test bot will be confused, but we already know adding the new hook doesn't break any existing tests via #3, especially since update.module doesn't yet have any tests. ;) (yes, we know -- see #253501: Tests for update.module -- it's sort of complicated).

Ok, #11 doesn't matter, since #220592: Core cache API breaks update.module: fetches data way too often, kills site performance, etc is now in. So, this just needs a reroll to apply cleanly and deal with the API documentation stuff.

Re-rolled now that #220592 has landed, and now with better API docs.

I also attached a D6 backport. We really need this hook to finish supporting a feature we tried to allow for in D6 core (checking for disabled modules). In the rush before the 6.0 release, I failed to get the API right and didn't have time to try to implement this in contrib as planned. We're just now realizing our mistake. D6 core already has translated strings to support this feature, but there's no underlying code for it...

Very nice job documenting the $projects array! Some more comments on the PHPDoc.

+ * Alter the list of projects before fetching data and comparing versions.

This tells me what the hook does. I'd love another few sentences below this that explain why I would use it.

+      'name' => 'Custom project',
+      'description' => 'Some custom project that does stuff.',

This kind of goes back into the above comment, but is it possible to give this a name and description that reflects a "real world" example?

+      // The most most recent (largest) value of the ctime (change time) on
+      // all the .info files included in this project.
+      '_info_file_ctime' => 1243888165,

Um. What? :) While I'm sure that's technically accurate, could we have that re-written in English please? ;) And maybe a pointer on how would I find this value to be able to add it to this array myself?

+    // The date stamp when the project was released, if known.
+    'datestamp' => 1243888185,

This is English, but again I'm left wondering how I would put the right value in here for my custom module.

Also, is $projects enough context here for people altering this data to be able to do what they want?

Once those things are addressed, I'm happy to commit this, since it won't break anything.

Rerolled the comments for update.api.php.

p.s. "The most most recent (largest) ..." was a typo. ;) I only meant "The most recent ...". ;) But, I still completely re-worded that. Hopefully it's better, now...

Vastly improved. Looks great!

I was confused on this change:

-  if (!empty($site_key)) {
+  if (!empty($site_key) && (strpos($project['project_type'], 'disabled') === FALSE)) {

Let's get a quick comment in there that reflects conversation in this issue, and then this is good to go IMO.

Adds a comment for #16 for both D6 and D7.

Attached is also a screenshot of the available updates report on a local D6 d.o test site where I put the following in my copy of drupalorg.module:

function drupalorg_update_projects_alter(&$projects) {
  unset($projects['drupalorg']);
  $projects['cvslog']['project_type'] = 'disabled-module';
  $projects['signup'] = array(
    'name' => 'signup',
    'info' => array(
      'name' => 'Signup',
      'description' => "Allow users to sign up for content (especially events)."
,
      'core' => '6.x',
      'version' => '6.x-1.x-dev',
      '_info_file_ctime' => 1243990684,
    ),
    'datestamp' => 1243990684,
    'includes' => array(
      'signup' => 'Signup',
      'signup_status' => 'Signup status',
    ),
    'project_type' => 'disabled-module',
  );
}

Notice how beautiful the screenshot looks. ;) Furthermore, I dvm()'ed the fetch URLs, and confirmed that the ones for cvslog and signup did not include a site key or version info.

Nice. Committed this to HEAD.

Marking to 6.x for consideration.

Looks good, committed to Drupal 6.

Great! :)

The stuff from update.api.php needs to move to contributions/docs/developer/hooks for D6 now.

I'll commit the docs later (gotta run now).

Here's the start of a D5 contrib backport... There's no support for a project_type in D5 contrib, so the fetch hunk doesn't really apply at all. That needs a little more thought.

Thanks, Gabor, for committing!

I committed the doc fixes to D6.

Moving this to the update_status queue for the D5 backport now...

Better D5 backport. Deals with the part about not appending a site_key for disabled modules. This is still somewhat incomplete. Really, we'd need to backport #162788: Include modules that aren't enabled (and some of the stuff already in core) to get real support for disabled modules in D5 contrib. But, for the purposes of adding this hook, I this this is all we really need at this point. But, that said, I tested and the hook itself is working as expected, and the fetch URLs don't include a site key if you specify a 'project_type' of 'disabled_module'. Any objections, reviews, or other testing?

Bah, no one else is going to review or test this. ;) Committed to DRUPAL-5--2. Moving back to core for posterity.