Add hook_update_projects_alter()

Dave Reid - April 27, 2009 - 01:44
Project:Drupal
Version:6.x-dev
Component:update.module
Category:task
Priority:normal
Assigned:dww
Status:closed
Description

From #162788: Include modules that aren't enabled I have learned that the easiest way to get update.module to be able to check disabled modules and themes is too allow the list of projects that are fetched and compared by update.module to be altered. Without this new hook, this task is nearly impossible without a very, very ugly hack.

#1

Dave Reid - April 27, 2009 - 01:47
Status:active» needs review

Patch attached for review. This will also need backport to D6 for use with the update_advanced module.

AttachmentSizeStatusTest resultOperations
445748-update-hook-projects-alter-D7.patch1.68 KBIdleFailed: Failed to apply patch.View details | Re-test

#2

dmitrig01 - April 27, 2009 - 02:24
Status:needs review» needs work

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

#3

Dave Reid - April 27, 2009 - 02:32
Status:needs work» needs review

Hmm... I can't just replace an actual array since it needs to be in the format of:

      $projects[$file->info['project']] = array(
        'name' => $file->info['project'],
        'info' => $file->info,
        'datestamp' => isset($file->info['datestamp']) ? $file->info['datestamp'] : 0,
        'includes' => array($file->name => $file->info['name']),
        'project_type' => $project_name == 'drupal' ? 'core' : $project_type,
      );

I did fix some spelling errors and improved the first doc line.

AttachmentSizeStatusTest resultOperations
445748-update-hook-projects-alter-D7.patch1.69 KBIdleFailed: Failed to apply patch.View details | Re-test

#4

dmitrig01 - April 27, 2009 - 03:04

Well the format needs to be documented

#5

webchick - April 27, 2009 - 03:14
Status:needs review» needs work

Agreed with dmitrig01. The example could have:

<?php
$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: Reduce RAM resource consumption.

#6

dww - April 27, 2009 - 06:19

I don't mind how this is going to impact #238950: Reduce RAM resource 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...

#7

Dave Reid - April 27, 2009 - 06:23

@dww What do you think about this change? If the project type has the word 'disabled' in it (aka disabled-modules disabled-themes), then no site key:

-    $url = _update_build_fetch_url($project, $site_key);
+    $disabled = strpos($project['project_type'], 'disabled') !== FALSE;
+    $url = _update_build_fetch_url($project, $disabled ? '' : $site_key);

#8

dww - April 27, 2009 - 06:47

@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

#9

dww - April 27, 2009 - 06:48

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

#10

dww - April 27, 2009 - 10:30

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

AttachmentSizeStatusTest resultOperations
445748-10.update-hook-projects-alter.d7.patch2.5 KBIdleFailed: Failed to apply patch.View details | Re-test

#11

dww - April 29, 2009 - 01:33

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

#12

dww - April 29, 2009 - 19:22

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.

#13

dww - June 1, 2009 - 20:46
Assigned to:Dave Reid» dww
Status:needs work» needs review

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

AttachmentSizeStatusTest resultOperations
445748-13.update-hook-projects-alter.d7.patch3.69 KBIdlePassed: 11835 passes, 0 fails, 0 exceptionsView details | Re-test
445748-13.update-hook-projects-alter.d6.patch1.68 KBIgnoredNoneNone

#14

webchick - June 1, 2009 - 21:19
Status:needs review» needs work

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.

#15

dww - June 5, 2009 - 00:32
Status:needs work» needs review

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

AttachmentSizeStatusTest resultOperations
445748-15.update-hook-projects-alter.d7.patch5.3 KBIdlePassed: 11835 passes, 0 fails, 0 exceptionsView details | Re-test

#16

webchick - June 5, 2009 - 00:38

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.

#17

dww - June 5, 2009 - 00:58

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:

<?php
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.

AttachmentSizeStatusTest resultOperations
445748-17.update-hook-projects-alter.d7.patch5.52 KBIdleUnable to apply patch 445748-17.update-hook-projects-alter.d7.patchView details | Re-test
445748-17.update-hook-projects-alter.d6.patch1.92 KBIgnoredNoneNone
445748-17.update-hook-projects-alter.d6.png181.68 KBIgnoredNoneNone

#18

webchick - June 5, 2009 - 01:05
Version:7.x-dev» 6.x-dev
Status:needs review» reviewed & tested by the community

Nice. Committed this to HEAD.

Marking to 6.x for consideration.

#19

Gábor Hojtsy - June 7, 2009 - 17:25
Status:reviewed & tested by the community» fixed

Looks good, committed to Drupal 6.

#20

webchick - June 7, 2009 - 17:40
Status:fixed» needs work
Issue tags:-needs backport to D6+Needs Documentation

Great! :)

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

#21

dww - June 7, 2009 - 18:42

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!

AttachmentSizeStatusTest resultOperations
445748-21.update-hook-projects-alter.d5.patch836 bytesIgnoredNoneNone

#22

dww - June 8, 2009 - 05:22
Project:Drupal» Update Status
Version:6.x-dev» 5.x-2.x-dev
Component:update.module» Code
Status:needs work» needs review

I committed the doc fixes to D6.

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

#23

dww - June 13, 2009 - 03:04

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?

AttachmentSizeStatusTest resultOperations
445748-23.update-hook-projects-alter.d5.patch1.5 KBIgnoredNoneNone

#24

dww - June 13, 2009 - 03:10
Project:Update Status» Drupal
Version:5.x-2.x-dev» 6.x-dev
Component:Code» update.module
Status:needs review» fixed

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

#25

System Message - June 27, 2009 - 03:20
Status:fixed» closed

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

 
 

Drupal is a registered trademark of Dries Buytaert.