now that http://drupal.org/node/152282 is done, the .xml release history files on d.o have a <tag> attribute that says what CVS tag they came from. we can use this to provide better version strings for stuff checked out from HEAD, since we can search the history for the release node pointing to HEAD and use the human-readable version string it's got, instead of just printing "HEAD" like we do now.

Comments

dww’s picture

Status: Active » Needs review
StatusFileSize
new1.62 KB

(depends on changes I *just* committed to update_status.module for this to work: http://drupal.org/cvs?commit=70620).

dww’s picture

Status: Needs review » Needs work

whoops, something about this patch screws up the modules page and it goes into an infinite loop. ;) i didn't notice this in testing on my flight home, since i was offline and never trying to aquire release data. will have to look into this more closely once i've had some sleep...

dww’s picture

Here's the deal:

  • cvs_deploy_alter_form() calls cvs_deploy_version_alter()
  • cvs_deploy_version_alter() calls update_status_get_available()
  • if there's no cached data (e.g. we just submitted the modules page, and invalidated the cache) update_status_get_available() calls update_status_refresh()
  • update_status_refresh() calls update_status_get_projects()
  • update_status_get_projects() calls cvs_deploy_version_alter()
  • repeat... :(

There are a few ways to break this chain:

  1. update_status_refresh() doesn't really need update_status_get_projects() to alter the versions. That's only done for cosmetic reasons, and to make sure the dev snapshot logic is right when we're going to call update_status_calculate_project_data(). update_status_refresh() really only needs the list of installed projects and the URL to query for the release history XML. So, we could add an argument to update_status_get_projects() that controls the mode it's in, and update_status_refresh() could call it one way (without the version altering) and update_status_calculate_project_data() could call it the other.
  2. We could add an argument to update_status_get_available() that tells it that if it can't get the data from the cache, not to attempt to refresh it and just return NULL.

Also, there's a problem with this patch that it's looking in the $avail array for each module on the admin/build/modules page, but that array is only per-project. Unfortunately, the data about grouping modules into projects lives in the $projects array returned by update_status_get_projects(), and that info isn't stored/cached inside $avail. :( So, we should really be calling that from cvs_deploy_alter_version(), too (and caching the results in RAM, at least), so we can find the version based on the project for modules that are contained inside another module's project (e.g. project_release.module vs. project.module). That'd be a strong argument in favor of option (1) above. OR we could stash this project/module info in the $avail array, so we could get to this data without re-doing all the work of update_status_get_projects(), in which case (2) might not be so bad.

Maybe we need a little bit more of a reorganization of update_status_get_projects() and update_status_refresh(), perhaps moving some of the code that's specific to update_status_calculate_project_data() out of update_status_get_projects(). I'm not yet clear what's the best way to go with all of this...

dww’s picture

Status: Needs work » Needs review
StatusFileSize
new11.22 KB

this turned into quite a complication, but the resulting refactoring should make things mostly easier to read and maintain, anyway. there's still some lingering badness regarding projects that define multiple modules... we cut a few corners here and there, though still works. the basic idea here is to break up the formerly-monolithic update_status_get_projects() into some smaller pieces:

update_status_get_projects()
Get the raw list of projects for the currently installed modules. This is shared by update_status_calculate_project_data() and update_status_refresh(), but only includes the data required by update_status_refresh() which needs less info. This avoids the infinite recursion above, since we don't invoke hook_version_alter(), etc, and it (slightly) improves performance by not doing work we don't need.
update_status_get_project($module)
Figures out the project a given module belongs to. Shared by update_status_get_projects() and cvs_deploy_form_alter().
update_status_process_project_info()
All the stuff to process the $projects array that's only required for update_status_calculate_project_data(), such as invoking hook_version_alter(), finding the currently-installed major version, and figuring out the status $type (official, dev, etc).

Make sense? Therefore, here's the basic logic and call tree in the 3 important cases:

  1. fetching the available release data -- update_status_refresh():
    • update_status_get_projects()
    • calls update_status_get_project() for every installed module
    • uses this simple array in update_status_refresh() to fetch and parse what we need
  2. available update report -- update_status_calculate_project_data():
    • update_status_get_projects() to get all the projects/modules
    • update_status_get_available() to get all the available releases
    • update_status_process_project_info() to get all the version and type data, invokes hook_version_alter(), etc
    • update_status_calculate_project_data() does all the existing logic to compare the installed info with the fetched data and produces the report
  3. admin/build/modules page -- cvs_deploy_form_alter():
    • Gets the info about installed modules directly from $form
    • Calls update_status_get_project() on each one
    • Calls cvs_deploy_version_alter() on each one, passing in the .info data, and the project name
      • calls update_status_get_available() once to get the $avail array of all the available releases
      • for each module, does the usual goodness to find the version from CVS/Tag if necessary
      • If the version is "HEAD", looks up release info in the $avail based on the project name and searches for a release node with "HEAD" as the tag

Sorry this reply is so long, but I wanted to record the motivation, since it's a huge patch. Again, probably clearer to just apply the patch and look at the above functions yourself, then trying to understand what changed only via inspecting the diff.

One lingering gotcha in the UI: since we're only fetch data for installed projects, we can only resolve HEAD for installed modules (or at least modules from projects that have at least 1 module installed).

dww’s picture

Project: CVS deploy » Update Status
Version: 5.x-1.x-dev » 5.x-2.x-dev

Actually, this patch is overwhelmingly about reorganizing the code inside update_status, so i'm moving it over to that queue for now.

dww’s picture

Project: Update Status » CVS deploy
Version: 5.x-2.x-dev » 5.x-1.x-dev
Status: Needs review » Fixed

After brief discussion with merlin on IRC, committed this to HEAD for both modules.
Moving this back to the cvs_deploy issue queue for posterity. ;)

Anonymous’s picture

Status: Fixed » Closed (fixed)