This is missing some important bits:

1) the directory scanning hasn't been updated.
2) not all of project has been scanned to ensure it's utilizing the new fields
3) the SQL in the install function has been updated for the new table, but there isn't an _update_X() function to do any kind of translation, so tables will (at the moment) have to be created by hand
4) I used file_create_path and file_create_url quite a bit, but I just realized those don't work outside of the files directory and the file scanning stuff may in fact work outside of the files directory, so that probably will need another layer of file handling to make sure that works right in all of the various cases.
5) The releases list looks pretty blah right now but we need to really work on that anyway.
6) It's possible the releases should be their own module entirely, which means some code in project.inc and project.module should move to release.inc and that of course renamed to project_release.module etc etc.
7) probably buggy =)]
8) It does 'filesize' a lot; filesize should probably be stored with the release along with filedate
9) filedate should be used more, though I'm not sure that's actually feasible
10) Lots of the fields are basically unused, meant to be filled in by the cvs module for things that are more specific to drupal.org

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww’s picture

FileSize
51.76 KB

new version, with the following changes/fixes:

1) moved all release stuff into project_release.module
2) capitalized "Minor version" on the project release node UI for consistency
3) if there's a conflict in version numbers, we now mark the minor version as the error, since that's the thing users are more likely to want/need to change to avoid the conflict
4) added the beginnings of project_update_5() to at least create the new {project_release_node} table -- still more to be done.
5) converted a few more places over to the new schema, and added some // TODO comments in a few others

other known bug:
- the "You must add releases only from projects." stuff from node/add/project_release isn't working

but, it's closer. ;)

dww’s picture

Priority: Normal » Critical
dww’s picture

Title: First draft at releases as nodes » make releases real nodes

merlin, what do you think about calling the new table {project_release_nodes} (plural) to match {files}, {node_revisions}, etc. see http://drupal.org/node/2497 for conventions on this (which, sadly the {node} table doesn't adhere to). ;)

dww’s picture

Assigned: Unassigned » dww
Status: Active » Needs work
FileSize
52.38 KB

(there are patches here, they're just not done, yet) ;)

- renames to {project_release_nodes} as per comment #3
- adds CHANGELOG.txt entry ;)

now that i've got a copy of the entire d.o {project_releases} table, and given the plans for "dev" in the version string for the nightly tarballs, my next step is adding a string field to {project_release_nodes} as the final version modifier. this will be an optional field, but will allow folks to enter in stuff like "rc2", "dev" (added automatically by the nightly tarball-generating script), etc.

dww’s picture

FileSize
54.02 KB

new patch that:

  • adds a version_extra field to the {project_release_nodes} table and all appropriate places in the code
  • fixes some help text that was bogus, out of date, or not showing up right
  • fixes SQL bugs in project_release_update()
  • fixes project_update_5() to only create the new tables if they don't already exist
  • fixes a few places i missed in the conversion from {project_release_node} to {project_release_nodes}

also, in IRC i confirmed w/ killes that the release dir *is* a subdir of files. so, i think all the file_create_url() stuff is cool. we just need to make sure that the release dir is validated as a subdir of files or some such...

dww’s picture

FYI: i've started working on the migrate path now ... this will be interesting. ;) probably won't have a new patch ready today, but hopefully soon.

webchick’s picture

dww asked my opinion today on IRC about dropping "rid" from the releases table in favour of just using "nid".

Pros

  1. You've already got a unique ID, why not use it?
  2. It's clear that this corresponds to a node.
  3. Less storage space in the database.
  4. Cons

    Only one, fairly big one... all links like http://drupal.org/project/issues?projects=3060&versions=6487&categories=... would now be extinct, because 6487 would turn into 80,000-something.

    I'm not a fan of link rot, however:

    a) there is literally no point of storing rid, other than legacy support.
    b) links like the monster one above should really have nice aliases like "drupal_head_bugs" and _those_ should be spread around rather than the "ugly" version.

    So I am ok with dropping the rid field.

dww’s picture

re: legacy rid vs. nid, killes in IRC hath spoken:

killes: I think it is a relatively minor inconvenience to change the links.
Derek: killes: great, that's what i was hoping you'd say. ;)
Derek: keeping a legacy rid in the DB and the code seems like a tragedy. ;)
killes: yeah
killes: no legacy

so, i'm leaning *heavily* towards ditching rid. it'd take *a lot* to convince me otherwise...

however, the more i mess with this patch, the more torn i am about the whole buisness of splitting up the version string into all these separate fields. here are the pros/cons as i see them:

pros

  1. forces a certain format on version strings and the meanings of the different parts
  2. allows faster, simpler queries (e.g. to find all releases for a given version of core) without having to do string manipulations each time.

cons

  1. adds complexity since not all projects need the same version fields (e.g core needs major.minor.patch, but no super_major or super_minor, whereas most contrib modules need super_major.super_minor (e.g. 4.7) along with major.minor (e.g. 1.3) to get the real version ("4.7-1.3", or perhaps "4.7.x-1.3" as previously discussed).
  2. adds complexity since 0 is a valid value, so either we have to use -1 to mean "this field isn't in use" or we have to do lots of extra grief to have these as "default NULL" columns in the db. we can't use db_query() to insert them as NULL values, we have to programmatically create a SQL string to leave out the fields we don't want to use if we want to actually store them in the DB as NULL.
  3. adds more drupal.org-specific assumptions to project releases. sure, we can make the function that converts these fields into a version string themeable (which is how it is in the versions of the patches attached so far), and we could even make it a per-project string with %major, %minor %patch %extra %super_major and %super_minor so people could specify their own version format. however, some sites might want a plain, free-form string as the version (i guess they could just use version_extra for that, and theme it appropriately, but still...).

in the end, at least at drupal.org, i envision the UI not allowing you to select/specify any of that crap. all you'd get is a drop-down of all the tags on your project that don't already have release nodes for them. ;) you'd select which tag or branch you want to create a release for, and all the version info is extracted directly from the tag name (which will follow a strict convention). so, from the UI perspective, it doesn't really matter, since folks won't see the crap either way. but, for the internals of how we store and process these versions, i'm torn...

i can see both sides of it. what do the experts think? ;)

thanks,
-derek

dww’s picture

FileSize
58.76 KB

new patch with the following changes:

  • adds most of an upgrade path for drupal.org (project_release_update.php). it still doesn't convert {project_comments} table right b/c of rid values stuffed into serialized data (see TODO comments). ripped out project_update_5(), since that update wasn't doing anything except creating the new table (which happens when you install project_release.module, anyway), and we'll want it as this stand-alone script, instead.
  • partially converts to storing all the version fields as NULL when not in use. manually creating project release nodes still gets this wrong (project_release_insert() isn't smart about this yet).
  • adds a "version_patch" column (e.g. for core releases that want to say "4.7.4").
  • fixed bug i introduced in last patch where "version_extra" wasn't getting disabled when it should have been.
  • if a file has already been attached to a project node, the edit tab shouldn't give you a way to attach a different file (at least not on d.o, we should consider how we really want this to behave).
dww’s picture

to be clear about the {project_comments} update hell, here's an example of what i'm talking about:
http://drupal.org/node/44276#comment-126310
that moved the version of the issue from 1 value to another.
in the DB, here's the value of the 'data' column in the {project_comments} table's row for this followup:
a:2:{s:3:"old";O:8:"stdClass":1:{s:3:"rid";s:4:"9703";}s:3:"new";O:8:"stdClass":1:{s:3:"rid";s:5:"12052";}}

:( so, while fixing {project_issues} itself is trivial (db_query("UPDATE {project_issues} SET rid = %d WHERE rid = %d", $nid, $rid);), fixing {project_comments} seems like a colossal pain in the ass and insanely expensive. i suppose during the update i could be creating a mapping of old rid to new nid (either as a temp DB table or just in RAM, and then at the end of the whole process, once i know all the new nids, i could walk through the entire {project_issues} table, unserialize the data column for each entry, do the rid-specific replacements based on my mapping, re-serialize, and stuff it back in the db. is there a better alternative?

what's the best way to solve this problem? any insight is welcome, since i've got other things to worry about and i'd rather someone just told me The Right Way(tm) to do this. ;)

thanks,
-derek

dww’s picture

FileSize
70.11 KB

new patch with some big changes:

  1. added a {project_release_projects} table (as per discussion in IRC) which stores project-wide data regarding releases (much like {project_issue_projects} does for issue tracking). in particular, there's a setting on the project edit page to enable/disable releases (which currently doesn't do anything, but easily will in another version of this patch) and a setting for the project-specific format string for the version string. this presents you with a bunch of %foo variables you can use to specify exactly the kind of format of the version numbers for each project. this setting is only visible if you have "administer projects" permissions, so you can given people (e.g. contrib authors) powers to create releases, but not to change the naming convention of the version #s for their releases...
  2. there's a global setting for the version string format that applies when projects don't define their own in the {project_release_projects} table (called 'project_release_default_version_format').
  3. removed the version string theme function in favor of project_release_get_version()
  4. added project_release_get_version_format(), which returns the active format string for a given project, either from the project object itself, the {project_release_projects} table, the global admin setting, or a hard-coded default if all else fails.
  5. ended up adding a string-only "version" field to {project_release_nodes} to aid in queries in the rest of the project code that are directly trying to match the full version string with LIKE.
  6. added an initial (still mostly untested and probably buggy) hook_access() so that things start to work for uid != 1. ;)
  7. ripped all the release directory scan stuff out and put it in a separate project_release_scan.inc file (which is currently not being included anywhere). commented out a few other instances where we were trying to touch this code. i believe it's all totally dead, and that d.o is the only site using it, and once we switch to the new CVS tag-driven system, there's no reason for directory scans anymore. but, for now, i'm keeping all that code in a separate file, just in case...
  8. converted some additional queries to {project_release_nodes} instead of {project_releases}
  9. initial attempt (still unsuccessful) to fix the "Download" link on the project overview pages
  10. fixed bug where file_path UI element was incorrectly being hidden when it wasn't really set, yet.
  11. added a bunch more TODO comments where needed

phew. ;) still plenty of work to do, but it's definitely getting closer...

killes@www.drop.org’s picture

11:42 < dww> so, the workflow is 1) create cvs tag w/ the right name, 2) add a release node
to your project that points to your tag, 3) external script notices there's a
release node that doesn't have a package built, does the magic, and updates
the release node w/ the real filename, publishes it, etc, etc.
11:42 < dww> are you cool with that?

Yes

dww’s picture

FileSize
78.31 KB

spent a while on the trip over to geneva working on the update stuff and a few other improvements. i've now gotten basically the entire update script working, though it's still expensive as hell (takes about 45 minutes on my laptop). :( i've started to get some pointers from killes about some optimizations, and the version of the script included in this patch includes a little more benchmarking code to try to identify the real bottlenecks...

summary of improvements over the previous patch:

  • project_release_update.php is code-complete, but slow
    • adds user output, timing values, and some benchmarking code
    • populates the {project_release_projects} table correctly, with code to handle d.o special-cases (e.g. documentation project doesn't need releases enabled, core has a different version format string than the site default, etc).
    • initial support for continuing the conversion if you abort it mid-way
    • converted to use project_release_get_version()
    • produces a {project_release_legacy} table that stores the mapping of the old rid to new nid values, along with the times that various parts of the conversion took
    • added convert_issue_followups() to iterate through all {project_comments} entries that refer to 'rid' in the data field, unserialize, fix up the old values, and updates the DB record. if it fails to map the old values into new nids, it adds a record to a{project_comments_conversion_errors} table.
    • sets a huge php timeout limit so it can run for the 40-60 minutes it's been taking on my laptop...
  • fixed bugs in project_release_alter_project_form() where we weren't correctly setting the default values from the node since i forgot to initialize $node = $form['#node'];, and since i was using the wrong variable name in 1 place. ;)
  • description of the per-project version string format field now says that if you leave it blank, you get the site default, and prints the current value of the site default.
  • release info is collapsed by default on project nodes, since most people never have to see this stuff at all (in fact, i'm not sure there are *any* fields that should be exposed to users that aren't project admins.
dww’s picture

killes and i resolved the bottleneck in project_releases_update.php! the time was going, overwhelmingly, to this:

db_query("UPDATE {project_issues} SET rid = %d WHERE rid = %d", $nid, $rid);

why? b/c there was no KEY on rid! by adding this line:

db_query("ALTER TABLE {project_issues} ADD KEY rid (rid)");

the whole update went from taking 30-60 minutes to just over 1 minute. ;) woo hoo!

i've still got some problems to sort out with the {project_comments} conversion, since it's still complaining that it can't convert 6789 of the comments, even though the tables that show what happened seem to indicate they should have worked. once i sort that out, i'll post another version.

dww’s picture

FileSize
83.79 KB

getting closer... ;) i fixed a bug in project_release_update.php that was causing some of the comment conversions to fail, based on not having our rid-to-nid mapping array declared global in the local scope of 1 of the functions. that fixed about 4000 of the errors. however, we've still got 2622 issue followups that can't be converted.

closer investigation reveals a deeper problem: all of those 2622 comments refer to a rid (release id) that doesn't exist in the {project_releases} table. :( i don't know what happened to these releases, but somehow they were deleted from the table at some point. *sniff*. i'm really not sure what i can do about it during the conversion, other than set the rid to 0 (since if i leave it set to the legacy rid value, it'll now be pointing to some random node). there are 462 missing {project_releases} records (i'll attach a list of them as another followup to this issue for the interested reader).

these missing entries are definitely visible on d.o itself. for example, if you go to http://drupal.org/node/51247, you'll notice that comment #1 there is moving the version from "" to "4.7.0". that initial, old value of version that's empty is really pointing to rid "9717", which doesn't exist in the {project_releases} table at all.

anyway, here's the summary of improvements since the last patch:

  • ported project_issue.module to the new system. project_release_load() has radically changed into a valid hook_load() implementation, so i had to change all the call-sites in project_issue/issue.inc and project_issue/mail.inc to either use that method correctly or call a different method as appropriate.
  • update script improvements:
    • more consistent about the fact that the initial '-' has to be included in the "extra" field, since it's too crazy to attempt to conditionally include the '-' in the format string only if there's a value in %extra...
    • adds the KEY on rid to the {project_issues} table
    • makes $nids_by_rid global in all scopes where we use it
    • adds "version" field to converted release nodes
    • stores a little more info (the pid (project id)) in the tables it creates to store what happened and any problems it encounters, and adds some more KEYs we might need for fast access
    • when we find rid values in {project_comments} that don't exist, we set them to 0
dww’s picture

FileSize
4.62 KB

as promised, here's a print out of the table of missing {project_release} rid's that are referenced by entries in {project_comments}.

dww’s picture

to avoid cluttering this issue, i moved further discussion of these missing {project_release} records into http://drupal.org/node/84655 (and posted more data)...

i don't think there's anything we can do during the upgrade to deal with this problem (now that it's been identified and hacked around), so let's use this issue only for the release-to-node conversion stuff...

thanks,
-derek

dww’s picture

FileSize
86.43 KB

back to the task at hand...

new patch with the following improvements:

  • added a define('PROJECT_RELEASE_DEFAULT_VERSION_FORMAT', ...) so that we're consistent about the default when dealing w/ the site-wide global setting.
  • major doxygen effort on project_release.module:
    • added function groups
    • every method is doc'ed
    • re-ordered a few things to put them more logically together
  • fixed project_release_form() to only add form elements for the parts of the version that are defined in the version format string for this project. to reduce duplication of code, added an internal _project_release_form_add_version_element() method, since the form elements are basically identical, with only a few minor variations.
  • renamed the body field from "Changes" to "Description", since "Changes" doesn't make sense for the nightly tarball release nodes, but "Description" makes sense for both.
  • ported project_releases_list() to the new schema, and added a big TODO comment about why this method isn't really what we're going to want long term
dww’s picture

FileSize
88.19 KB

new patch after the fallout from http://drupal.org/node/86863 seems to be mostly resolved. includes the following:

  1. version fields that aren't used are stored in the DB as NULL. this is accomplished via dynamically constructing the right SQL for insert/update in project_release_db_save()
  2. the format string now handles delimiters as described here: http://drupal.org/node/86863#comment-159319
  3. added more thorough help text for the format string
  4. fixed bug in project_page_overview() due to using the wrong column name from the DB ("path", not "file_path" as it is now).
  5. simplified _project_release_form_add_version_element() a little bit
  6. project_release_update.php has been modified to match these changes (e.g. 2 vs. 3-digits for core/contrib, subprojects (releases just disabled)), etc.
  7. we're converging on the 2 digits of the contrib-specific stuff should be "major.patch" for consistency w/ 2-digit core
dww’s picture

FileSize
88.56 KB

a few minor but important fixes:

  • i now use !== not != when testing for an empty string, so that a 0 doesn't match it. this fixes some problems with project_release_get_version() getting confused by version elements set to 0.
  • in the non-cvs case, we can't really make *everything* in the format string a required variable, since the whole idea is that some of the elements in the format string might not be present. we need (not before the system goes live, but some day) a better solution for this.
  • when viewing a release node, if the rebuild flag is set, we print out that it's a nightly development snapshot, and print out the branch, too. also, i moved the description below the header info about the release, and removed a line of dead code.
  • in project_release_update.php i used to set extra to "-dev" in one place, before i finalized on the whole #extra thing for handling the delimiters...
dww’s picture

  • only provide the 'Add new release' link if the project has releases enabled
  • fixes to project_release_get_releases:
    • only display published release nodes
    • add an argument to control sort order, so we can display options sorted by version in the issue queue, but list nodes sorted by creation date on the /releases page for a project (for now).
Kjartan’s picture

This is from a rather superficial testing. The default site wide version string seems to be completely wrong based on the help text. Also changing this won't actually cause the title of any existing nodes to be rebuilt, nor will going to edit the release node and saving it again. Also without an update script any you cant enable releases on existing project nodes as it tries to do an UPDATE in a table there are no entries in.

I'll try and setup a clean site and get some quality time with this later for a further review.

dww’s picture

yeah, it still needs work, as advertised. ;)

that said, there is an upgrade script included to convert old (non-node) releases into nodes and do everything else. it's project_release_update.php. it just can't live in hook_update_N() for a variety of reasons...

right, the format string just effects new releases, not old ones. once a release node has a version, that's fixed. we don't recompute the version everytime we want to use it based on the version fields and the format string... that's by design. however, i'm not surprised the site-wide default is wrong from the help text -- that stuff has been in flux...

thanks for taking a look, but honestly, your time would be better spent once i set this to "needs review". ;)

dww’s picture

FileSize
88.99 KB

new patch:

  • fixed PROJECT_RELEASE_DEFAULT_VERSION_FORMAT based on new handling of % ! and #
  • fixes to project_release node form:
    • form_set_value() mojo to protect ourselves from badness in case cvs_form_alter() is turning us into a funky 2-page form...
    • grouped body and filter into a 'body_filter' array to match how core usually does it
  • fixes to project_release_view()
    • added a check_plain() on the tag just to be extra paranoid (the tag comes from CVS, and our CVS scripts already validate, but hey, better safe than sorry).
    • since we're defining our own hook_view(), node_prepare() isn't called, so we have to use check_markup() ourselves.
dww’s picture

FileSize
89.19 KB

new patch:

  • renamed all occurances of "super" with "API" to be more reasonable and clear. so, it's now the "API major version", "version_api_major", etc.
  • we correctly set the "rebuild" flag if the selected tag is a branch
dww’s picture

FileSize
89.52 KB

new patch:

  • to make form_alter() life easier, we now group related elements on the release node form into their own arrays (e.g. $form['version'][], $form['tag'][], $form['file'][], etc).
  • also, slightly smarter setting of the title so to play more nicely with the form_alter()'ed versions of the release node form...
  • better checking to make sure we don't try to insert empty strings or non-numeric values into the DB for version form elements (it's better to leave them NULL in these cases).
dww’s picture

FileSize
90.52 KB

new patch that fixes all the release node creation ACL problems

  • only allow creation of release nodes if we know the project nid (for real, this time)
  • lots of cleanup/fixing to project_release_access()
  • added project_release_access_check() to be shared in a few places, which has knowledge of both project owner, and cvs maintainers (if cvs.module is installed)
  • the link to "add new release" on the project node conditionally appears using the same code
  • disabled deletion of release nodes entirely. we'd never want to delete them, only unpublish (or else all hell would break loose for the issue queue for historical issues still pointing at one of those releases...)

heavily tested on local site for uid 1, project owner, cvs maintainer, and regular users. everything works as expected. it's slightly rough in a few cases and you don't get the best possible feedback about what's going on, but you have to be manually constructing bogus URLs in that case, so it's nothing anyone would just stumble into on their own. ;) but, all ACLs are faithfully enforced, even if the feedback for why they're being denied isn't always there in the obscure edge cases.

this also avoids (for now, and probably forever) the complication of introducing another page on the release node form for if there's no project nid in the URL to have a page to act as a project selector, and trying to do all the ACL checking correctly, etc, etc. ugh. no thanks. ;)

dww’s picture

FileSize
92.65 KB

new patch for better version # validation and UI (which is shared in the TRUNK special case code path to manually enter version numbers):

  • ensure that at least 1 of the version elements is filled in
  • ensure all elements are numeric (except patch, which we allow as 'x', too).
  • add a hidden form element to control if we want this validation, so that the cvs_form_alter() case can turn this stuff off when necessary for proper functioning of the N-page form.
  • make $form['version'] a fieldset for CSS goodness and so we don't have to duplicate the words "version number" for each element
  • shrink down the size of the boxes to something more reasonable
  • allow version_extra to be 40 chars on the form
  • CSS float/clear magic to keep everything on the same line

TODO:
- the validation to check if a given version already exists doesn't work b/c of the db_query() handling of NULL.

dww’s picture

here's a screen-shot of the page when selected 'TRUNK' as the tag and hit 'next'. it's where you have to fill in some version identification for your 'TRUNK' release, with the nice fieldset and CSS goodness. ;)

dww’s picture

FileSize
94.45 KB

new patch for having a more reasonable form during release node edits:

  • project administrators can change everything (except file md5 hash and date)
  • non-admins can at least view everything
  • version element fieldset is in its own array within $form
  • added a _project_release_form_add_text_element() helper to insert similar textfield form elements.
webchick’s picture

Based purely on the screenshot, I'm kind of left wondering 'uh. what do I actually fill in here?' :) Could the description be extended (or perhaps a #prefix on the fieldset)? Otherwise, maybe have these things load up with default values? like 1 - 0 - 0 - 0? (assuming that's what I'm to type in)?

dww’s picture

thanks for the feedback, webchick.
this is still a work in progress, i mostly just wanted to show that i got the damn form working. ;)

in terms of usability...

a) certainly i could add a description property on the fieldset itself to help out.

b) my latest scheme is in the cvs_local.inc file (for truly d.o-specific hacks) i'd further alter this form to hide the 2 API fields and replace them with a dropdown that has values like "5.x", "4.7.x", "4.6.x", etc. then, the validate step would put the right things into api major and api minor for you. and yeah, i could add default values for major, patch and extra (0, 0, and "dev") respectively. but that's really for http://drupal.org/node/84706, not this issue.

the main point of this page is to force people to select a core API version that this particular HEAD/TRUNK release is intended for...

dww’s picture

FileSize
94.55 KB

new patch with a few minor fixes:

  • d.o conversion script (project_release_update.php) better handles populating the version_extra field (no need to store the initial - in there like we used to).
  • better setting of required fields during edit (e.g. the file-related junk).

p.s. massively new-improved UI for the tag-selector parts of the form in http://drupal.org/node/84706. see new patch and screenshots there. ;)

dww’s picture

FileSize
98.01 KB

new patch with big changes (hopefully for the better). instead of all this hard-coded "API version" crap, i decided to just make that stuff an automatically-created taxonomy. so, project_release.module now creates a 'Project release API compatibility' vocabulary. just like the magic project taxonomy (for classifying projects into types, etc), if there are no terms in this vocabulary, it is ignored. however, if taxonomy is enabled, and this vocab has terms, then it appears as another version selector element, instead of the text fields for "API major" and "API minor".

  • we don't store api_* in the {project_release_nodes} table
  • tons lots of goodness for proper handling of this special taxonomy
  • everything works great without any terms, if there are terms but no "*api" in the format string for the current project, or working for real. also works w/ the cvs_form_alter() stuff (see new patch coming in http://drupal.org/node/84706).

in other news...

  • moved the "default download" version selector out of project.inc into project_release.module where it belongs (needs work)
  • added a project_release_project_download_table() method since creating the $body is too painful via pure hook_nodeapi() in 4.7.x. i needed control over where the table shows up, so it was easier to just make my own special function for it. the table is currently just stubbed out, but can be a real table in the next version of the patch
  • further conversion of TRUNK -> HEAD for consistency

this is getting dangerously close to "needs review". ;) as far as i can tell, here's the remaining show-stopper TODO items:

  1. implement project_release_project_download_table() for real
  2. fix the version # validation stuff to prevent duplicate versions
  3. testing of the current iteration of the d.o conversion script with all the new taxonomy stuff...
dww’s picture

FileSize
99.59 KB

new patch with lots of fixes:

  • project_release_update.php (for the d.o conversion) now correctly handles the taxonomy stuff, and even customizes the api taxonomy for d.o and pre-populates the terms with the ones we need during the conversion.
  • renamed an "$edit" to "$is_edit" to be more clear this isn't like the $edit var you ususally see in drupal code.
  • when editing an existing release, only allow users with 'administer projects' permission to change the API compatibility taxonomy
  • fixed bug in project_release_taxonomy() where i was calling the wrong function to find the api vocab id.

still TODO:

  1. download table for project nodes
  2. validation step that prevents duplicate releases w/ the same identification
dww’s picture

FileSize
99.89 KB

minor changes from last patch:

  • restored a few key links we'll want in the download table, since i want those back during debugging and so eaton has something to play with. ;)
  • if there are no published releases, /node/N/release now tells you about it, instead of being a totally blank page.

TODO items from comment #35 still need doing...

dww’s picture

FileSize
101.15 KB

fixes for validation:

  • properly prevents you from submitting a duplicate release with the same version as an existing one. this works both without cvs.module installed, and in the nasty N-page form. ;) accomplished by means of a new project_release_exists() method that really works. ;)
  • we need to check isset($edit->version_major) and friends, not just !$edit->version_major, or we incorrectly flag validation errors if you want to submit a 0.0 release without "dev" or anything in the "extra" field.
  • added some additional check_plain() when using form_set_value()... not sure if this is necessary or a good move, but i was feeling paranoid. ;)

sadly, i just uncovered a whole new area of work: the project browse pages are busted. :(

TODO:

  1. the download table on project nodes (from above)
  2. the "filter by version" selector on, e.g. /project/Modules, doesn't work.
  3. making sure the "Download" link on those project browse pages is reasonable
  4. browse by category appears fubar: no categories are showing up. :(
  5. browse by date is busted care of a faulty SQL query where we try to JOIN on {project_release_nodes} twice with the same alias.

just when i thought i was almost done... :(

dww’s picture

FileSize
101.49 KB

new patch that fixes other bugs, the 5 TODO from above still hold...

  • resolved the mysterious form errors reported by eaton when submitting release nodes. ;) project_release_validate() was trying to call form_set_value() on the file-related form elements, but was using the old form locations (i moved those all into a single $form['file'] array a while ago). these errors only showed up if you didn't enable cvs.module, which is why i wasn't seeing them in my usual testing (though i've mostly verified that everything works fine in the non-cvs case -- i guess i never actually tried to submit a release node without cvs.module installed during the last few iterations of the patch).
  • fixed up the automatic setting of title in cases where we don't have $edit->project set to the entire project object. also, to be safe, we're using the project uri for the title now, not the full-blown project title, since that more closely matches the filename.
dww’s picture

FileSize
102.33 KB

new patch based on initial dww + nedjo debugging session. ;) browse by category is now basically working:

  • fixed silly SQL error that was causing the empty categories on /project/Modules/category based on confusion between the release nid (nid) and project nid (pid) in the {project_release_nodes} table.
  • removed the project_default_tid() method, which wasn't really necessary, was buggy, and was causing additional problems because of JOINs on {project_releases} which no longer exists.

the "Filter by version" stuff is still fubar, and we have to figure out what version to link to via the "download" link in these pages, but this is definitely closer, and i already have a better handle on this stuff. we're going to do another session this afternoon... stay tuned.

dww’s picture

FileSize
106.25 KB

woo hoo, another quality session w/ nedjo and i think nearly all of the browse-by stuff is working now. ;)

  • moved all the release-related settings for the browse stuff into project_release.module, where they belong.
  • renamed the old "project_browse_releases" setting to "project_release_browse_versions"
  • fixed version filter form to use compatibility taxonomy terms, not the whole complex thing with regexps and creating the ".x" list, caching it, etc, etc.
  • fixed SQL for the version filter
  • fixed SQL for browse by date so that it only adds the JOIN on {project_release_nodes} if we didn't already JOIN on that for the version filter.
  • added a project_release_compatibility_list() method to generate the list of compatbility taxonomy terms for use in various places, and ripped out the problematic project_releases_list() function that was never fully working.
  • instead of just "Filter by version", we print out "Filter by ", e.g. "Filter by project release API compatibility" by default, or "Filter by Drupal Core compatibility" on d.o...
  • changed project_release_use_taxonomy() to just return the tree, so we don't have to get the tree twice all the time...

TODO:

  1. making sure the "Download" link on those project browse pages is reasonable. currently, it's just getting the most recent release compatible with the selected version. this will potentially be wrong for projects that have a stable vs. devel branch against the same version of core.
  2. related to this, the download table. ;)

also, (not urgent) i don't think all of this project browsing stuff really works correctly if you disable project_release.module. this should be cleaned up someday soon, but isn't a show-stopper for releasing all of this on d.o.

dww’s picture

FileSize
106.99 KB
  • moved the version filter form into project_release.module, where it belongs.
  • fixed the download link to at least filter from the right version if $version != -1. this still needs work, but at least it works as currently expected now...
dww’s picture

Status: Needs work » Needs review
FileSize
107.51 KB

i've decided to just wait on the full-blown download table on the project node. it'll be nice, but there's no reason to further delay the release system. that can be easily handled in a separate issue once this goes live.

so, new patch with a few minor changes:

  • restored download link on the project node (and called it "download default release", since it's not necessarily "latest", anyway).
  • renamed "project_release_use_taxonomy()" to become "project_release_get_api_taxonomy()", since that's what it's really doing now (returning the taxo tree). it can still be used as a boolean to test if we're using this taxonomy (or have taxonomy enabled at all), since it just returns false if there's no taxonomy on the site, or if the vocab is empty.
  • allow/require projects to set the compatibility taxonomy even if "*api" isn't in the format string. we need to classify core with this taxonomy so the version filter on the project browsing pages work for core, too...
  • fixed d.o conversion script to classify core releases with the right taxonomy terms based on the cvs tag.

this is hereby in need of review. ;) be sure to apply against a DRUPAL-4-7 checkout of project and project_issue.

i've got a test site up, but i need to spend a little time doing some setup to make it useful to anyone else, and i need to sleep right now. i'll post another issue with instructions once the site is ready for public consumption.

there's definitely more to be done for functionality and usability, but i think this is potentially an acceptable initial starting point. at this point, i'd rather get this in and live, and worry about extending/improving it via separate issues.

thanks,
-derek

p.s. if you're going to test on a local site, and want to see how things are going to be on d.o, you should also install the latest patch from http://drupal.org/node/84706, since that modifies a lot of the forms and behavior. http://drupal.org/files/issues/cvs_release_node.patch_16.txt is *nearly* ready to go, there's just 1 lingering FAPI bug in the d.o-specific customization code (which we could easily just disabled for version 0.1...).

dww’s picture

some TODO items from IRC feedback:

  1. the "download default release" link should link to the file itself, not the release node (this is already done in a workspace and installed on the test site, not worth posting a new patch just for this 1 line change) just yet...
  2. the default choice for the compatibility selector shouldn't be "None", it should be "-----" or "[select one]" or something...
  3. if the default version isn't set correctly, we should just not print the link, instead of printing a bogus link pointing to node/0 or something.
  4. the d.o conversion script should automatically update the default versions from projects, instead of leaving them with the old bogus values. in fact, that field should really move out of {project_projects} and live in {project_release_projects} instead.

i also submitted separate issues for a few other ideas i've had on usability improvements for this:
http://drupal.org/node/89535 add version filter to each project's "view all releases" page
http://drupal.org/node/89537 project-specific releases page needs sorting options
http://drupal.org/node/89538 need branch-aware notion of "default" or "latest" releases
http://drupal.org/node/89539 table of downloads on the project node, not just a "default release" link

drewish’s picture

one small thing, i kept trying to edit the "Version string" textfield on the submit project release form. perhaps it could be made a form item rather than textfield?

drewish’s picture

on more small thing: the default values for the default release on the station module wasn't set correctly: http://drupal-release.dwwright.net/node/40312/ . i looked at a few other modules and they all seem to be this way. it was easy enough to change but i'm assuming that lots of maintainers won't get around to setting it so it's probably a good idea to get the default set correctly.

dww’s picture

thanks for the testing/feedback...

re: comment #44: you mean you'd rather just hide the version string entirely, instead of displaying it as a read-only text field? the intention of displaying it is so that you'd see the final version string, given your choices on the earlier page(s). i also hoped that the description on that fieldset would be enough to explain it. do you have a better suggestion on how to handle this?

re: comment #45: see TODO #4 from comment #43. ;)

thanks,
-derek

drewish’s picture

humm, maybe rather than a combobox and textfield both should be form items. so that it's obviously they're readonly. adding a description seems like your treating the symptom rather than the cause.

whoops, sorry, read right over #4.

dww’s picture

i've had all kinds of problems with form values not working in this N-page form because type=item, attribute=disabled, etc, etc, all prevent the form element from getting into POST, and then all hell breaks loose. chx suggested i just use hidden elements for the real values i care about (since those *do* end up in POST), and i can then do whatever visual stuff i want (type=item, disabled select box, whatever) in the elements being displayed. this is working fine in a local workspace. i'll roll a new patch (addressing the other minor formatting issues raised above and in http://drupal.org/node/84706#comment-146262) in the next day or so...

meanwhile, i want to give other folks a chance to comment.

thanks,
-derek

dww’s picture

here's another cool idea i had:

http://drupal.org/node/89673 add input filter to convert #12345 into a link to an issue

add this to the list in comment #43 for things we can do to make all this more usable and happy for project maintainers and end users...

dww’s picture

FileSize
108.02 KB

new patch that fixes most of the TODO items from comment #43:

  1. default link now points to the file to download, not the release node
  2. if the default version is bogus, we don't print a bogus link
  3. conversion script now updates the default release value correctly, so #2 here shouldn't ever really be a problem (but better safe than sorry).

additionally, i fixed a bug in the conversion script that was causing a few of the legacy conversion tables to not get created due to a stupid SQL error (trying to set an unsigned int column to have a -1 as the default value).

the other remaining issues are all actually from the patch in http://drupal.org/node/84706 (not that you'd necessarily know that from playing with the test site). ;) so, i just added some TODO items in a followup there (http://drupal.org/node/84706#comment-146511) to encapsulate the remaining tasks.

dww’s picture

i just installed the latest patch and fixed the DB on the test site (to run the same SQL that the d.o conversion script now runs). so, all the default release link problems are now gone over there...

of course, i think http://drupal.org/node/89538 and http://drupal.org/node/89539 are going to be the real solution to the download links, but that's another story... ;)

dww’s picture

FileSize
110.08 KB

new, possibly final version of this patch before it's RTBC:

  • added a setting to control which terms from the api compatibility vocabulary should be "active". drop-down lists where you get to select a term are limited to active tids for non-project administrators.
  • much more consistent formatting and use of fieldsets in the forms for adding and editing releases.
  • some other minor cosmetic changes for the release node form.
  • completely hide the "rebuild" value, even for admins, since it only means anything with cvs.module for now, and it's not even displayed in that case (it's determined by cvs branch vs. tag).
  • since i'm not implementing any of the release directory scanning stuff in the first release of this code, i made the setting for it deprecated with custom validation.
  • inside project_release_update.php, when setting the compatibility taxonomy, we set the 'module' property, too.
dww’s picture

Status: Needs review » Needs work

a few minor items that need fixing:

  1. the conversion script should put things from default branches into major level 1 (i.e., "4.7.x-1.0-dev" should be the version string for the release nodes pointing to DRUPAL-4-7)
  2. fix the "Add new release" link so it doesn't show up unless someone has permissions to use it
  3. double check the default download link on project nodes when they're unpublished (e.g. from the browse-by pages). we should never try to use a link to an unpublished release node...

as far as i know, those are the only show-stoppers. also, i'm planning to put all of project_release.* in a "release" subdirectory of modules/project (in anticipation of adding other add-ons to project, like "views", "vcs" (version control systems), etc).

finally, i'd like to consider better use of .inc files to not have to parse all this code on every page load on d.o, though this part could certainly happen after the initial commit and even after the system first goes live.

dww’s picture

i just committed everything from this issue into the DRUPAL-4-7--2 branch of project and project_issue. new changes will either just be committed directly to that branch, or posted as patches against the end of that branch, not as complete patches against DRUPAL-4-7 anymore...

as promised, project_release* now lives in a "release" subdirectory, too...

dww’s picture

fixed TODO #2 from comment #53 in revision 1.1.2.29 of project_release.module on DRUPAL-4-7--2 branch:
fix the "Add new release" link so it doesn't show up unless someone has permissions to use it

dww’s picture

Status: Needs work » Needs review

fixed TODO #1 from comment #53 in revision 1.1.2.15 of project_release_update.php on DRUPAL-4-7--2 branch:
he conversion script should put things from default branches into major level 1 (i.e., "4.7.x-1.0-dev" should be the version string for the release nodes pointing to DRUPAL-4-7)

i can't reproduce the problem i mentioned in #3 there, either on a local site, or on scratch.d.o, in either the project node or the browse-by pages... so, i'm going to ignore that problem for now. the real solution to that crap is http://drupal.org/node/89538, anyway.

ideally, this would all be reviewed before it goes live on d.o:

cvs co -r DRUPAL-4-7 contributions/modules/project contributions/modules/project_issue
cd contributions/modules
cvs diff -uN -r DRUPAL-4-7 -r DRUPAL-4-7--2 project project_issue > full.dif
dww’s picture

whoops, sorry, i pasted the wrong cvs checkout command above. should be:

cvs co -r DRUPAL-4-7--2 contributions/modules/project contributions/modules/project_issue
cd contributions/modules
cvs diff -uN -r DRUPAL-4-7 -r DRUPAL-4-7--2 project project_issue > full.dif

(you need to checkout a copy from DRUPAL-4-7--2 to get the project/release directory, which is where the bulk of the code lives).

chx’s picture

I read this diff and found no security holes. Whether this is because dww's is a superb careful coder (looks like it...) and there are indeed none or because I am blind remains to be seen.

dww’s picture

Status: Needs review » Needs work

heine pointed out that we're printing out the "API compatibility" taxonomy terms in a few places without a check_plain(). i'll fix this ASAP.

dww’s picture

Status: Needs work » Needs review
FileSize
2.26 KB

everything else has been committed to the DRUPAL-4-7--2 branch. the attached patch fixes the above mentioned problem where i was printing out terms from the API compatibility taxonomy without a check_plain(), which could have allowed XSS for folks with administer taxonomy permissions. ;) (not exactly a dangerous attack vector... but it'd be a potential privledge escalation, so i'm happy to fix it). is this little patch RTBC?

dww’s picture

Status: Needs review » Fixed

after review from chx and heine, committed #60 as revision 1.1.2.31 to the DRUPAL-4-7--2 branch.

i'm marking this issue as fixed. at this point, everything is committed. deploying it on d.o is a separate issue. any futher problems or requests should just be filed as separate issues with patches against the DRUPAL-4-7--2 branch, instead of yet more follow-ups to this monstrous post.

/me trembles with excitement... so close to being deployed. ;)

Anonymous’s picture

Status: Fixed » Closed (fixed)