i just noticed that update_status doesn't gracefully handle the case of when you enable/disable modules on your site. for example:

1) fresh install
2) visit admin/logs/updates
3) enable some contribs
4) revisit admin/logs/updates

at step #4, none of your newly enabled modules are displayed at all, not even an entry saying something like "this module is now enabled, but no update status data has been fetched for it yet...".

perhaps the best thing would be to just clear the cache entry for the update_status data whenever we save the admin/build/modules form to ensure that when we land back on the status report page, we have re-fetch.

CommentFileSizeAuthor
#5 update_status_conditional_refresh.patch.txt4.15 KBdww

Comments

merlinofchaos’s picture

I agree. It's a simple hook_form_alter to do this, as well. Could probably cut&paste the one from latest Views.

dww’s picture

Assigned: Unassigned » dww
Status: Active » Fixed

it wasn't quite cut+paste from views. views_form_alter() immediately invalides the views cache when you hit the form at all, we only want to invalidate on form submission.

I committed a fix to HEAD: http://drupal.org/cvs?commit=69774
Please re-open (preferably with a patch) if anyone objects to my change.

thanks,
-derek

dww’s picture

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

Upon further testing and consideration, this patch (in combination with other changes like automatically re-fetching when the cache is empty) has created a critical bug in update_status: every time any Drupal site anywhere submits their modules page, we re-fetch all the XML release history files from drupal.org. :(

Here's what happens:

  • Submit the modules page, invalidate the update_status available updates cache.
  • Redirect sends us back to admin/build/modules
  • hook_help() hits update_status_help(), which calls update_status_requirements('runtime')
  • update_status_requirements () calls update_status_get_available()
  • cache was cleared, so update_status_get_available() automatically re-fetches

EVIL!!! :(

Once again, there are a few options for breaking this chain of doom:

  1. update_status_get_available() could take an argument on whether or not it should automatically refresh the data. A lot of the code in update_status is still written to handle the case where it returns an empty array, back when it was possible that cron hadn't run and we had no data. Then, update_status_requirements() would call it with this arg set to FALSE so it doesn't automatically refresh, and just prints a message about "Update status information is unavailable, [cron] may need to run or [check for updates manually]" (where the stuff in [] are links, etc).
  2. Don't actually purge the cache on the modules page, but set a "dirty bit" variable or something, so that we can still print that things are stale, but continue to use whatever data we still have.
  3. Split up the cache to be per-module, and complicate the code in a few places so that we only fetch data for modules that we don't have data for. Then, we'd only immediately re-fetch when you submit the modules page if you enabled new modules, and we'd only fetch data for those modules, not all N you have installed. This still seems crappy, but it'd be less crappy.

I think I prefer (1) from this list, but I'm open to other suggestions...

merlinofchaos’s picture

I'm cool with #1

dww’s picture

Status: Active » Needs review
StatusFileSize
new4.15 KB

attached patch cleans up the code in the following ways:

  • update_status_get_available() now takes a $refresh bool to indicate if it should refresh the cache. defaults to FALSE.
  • Fixed up the places that call it to more gracefully handle no data.
  • Added a _update_status_no_data() helper to share the code that generates the message about missing data.
  • Fixed up update_status_requirements() to mark it as a warning if there's no available update data.
  • Fixed update_status_help() so that the admin/build/modules page prints the message (not as an error, just 'status'), when there's no update data.

Currently, *no* call-sites are invoking update_status_get_available() with $refresh=TRUE. So, once you save your modules page or clear your cache, you get the nice message about no data, and you have to manually check if you want to, even when you land on the status report page. it's trivial to change this if we want, e.g. the status report to always attempt to refresh when you land there, but I kind of like the explicit nature of how things behave now.

Other than that, I'm very happy with how things look and behave now. It's quite slick, IMHO, and at least it's no longer re-fetching and hitting d.o whenever anyone saves their modules page. ;)

dww’s picture

Category: bug » feature
Priority: Critical » Normal
Status: Needs review » Fixed

After discussion in IRC w/ merlin, I agreed to automatically refresh the cache on the status report and settings pages. With that minor change, committed the patch to HEAD.

Setting the status back to match the original issue for posterity.

Anonymous’s picture

Status: Fixed » Closed (fixed)