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
Comment | File | Size | Author |
---|---|---|---|
#60 | project_release_taxonomy_check_plain.patch.txt | 2.26 KB | dww |
#52 | release_as_node.patch_29.txt | 110.08 KB | dww |
#50 | release_as_node.patch_28.txt | 108.02 KB | dww |
#42 | release_as_node.patch_27.txt | 107.51 KB | dww |
#41 | release_as_node.patch_26.txt | 106.99 KB | dww |
Comments
Comment #1
dwwnew 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. ;)
Comment #2
dwwComment #3
dwwmerlin, 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). ;)
Comment #4
dww(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.
Comment #5
dwwnew patch that:
project_release_update()
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...Comment #6
dwwFYI: 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.
Comment #7
webchickdww asked my opinion today on IRC about dropping "rid" from the releases table in favour of just using "nid".
Pros
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.
Comment #8
dwwre: 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
cons
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
Comment #9
dwwnew patch with the following changes:
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.project_release_insert()
isn't smart about this yet).Comment #10
dwwto 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
Comment #11
dwwnew patch with some big changes:
project_release_get_version()
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.LIKE
.phew. ;) still plenty of work to do, but it's definitely getting closer...
Comment #12
killes@www.drop.org CreditAttribution: killes@www.drop.org commented11: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
Comment #13
dwwspent 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_get_version()
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.$node = $form['#node'];
, and since i was using the wrong variable name in 1 place. ;)Comment #14
dwwkilles and i resolved the bottleneck in project_releases_update.php! the time was going, overwhelmingly, to this:
why? b/c there was no KEY on rid! by adding this line:
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.
Comment #15
dwwgetting 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:
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.$nids_by_rid
global in all scopes where we use itComment #16
dwwas promised, here's a print out of the table of missing {project_release} rid's that are referenced by entries in {project_comments}.
Comment #17
dwwto 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
Comment #18
dwwback to the task at hand...
new patch with the following improvements:
define('PROJECT_RELEASE_DEFAULT_VERSION_FORMAT', ...)
so that we're consistent about the default when dealing w/ the site-wide global setting.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.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 termComment #19
dwwnew patch after the fallout from http://drupal.org/node/86863 seems to be mostly resolved. includes the following:
project_release_db_save()
project_page_overview()
due to using the wrong column name from the DB ("path", not "file_path" as it is now)._project_release_form_add_version_element()
a little bitproject_release_update.php
has been modified to match these changes (e.g. 2 vs. 3-digits for core/contrib, subprojects (releases just disabled)), etc.Comment #20
dwwa few minor but important fixes:
!==
not!=
when testing for an empty string, so that a 0 doesn't match it. this fixes some problems withproject_release_get_version()
getting confused by version elements set to 0.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.#extra
thing for handling the delimiters...Comment #21
dwwproject_release_get_releases
:Comment #22
Kjartan CreditAttribution: Kjartan commentedThis 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.
Comment #23
dwwyeah, 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". ;)
Comment #24
dwwnew patch:
form_set_value()
mojo to protect ourselves from badness in case cvs_form_alter() is turning us into a funky 2-page form...project_release_view()
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).hook_view()
,node_prepare()
isn't called, so we have to usecheck_markup()
ourselves.Comment #25
dwwnew patch:
Comment #26
dwwnew patch:
Comment #27
dwwnew patch that fixes all the release node creation ACL problems
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. ;)
Comment #28
dwwnew patch for better version # validation and UI (which is shared in the TRUNK special case code path to manually enter version numbers):
TODO:
- the validation to check if a given version already exists doesn't work b/c of the db_query() handling of NULL.
Comment #29
dwwhere'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. ;)
Comment #30
dwwnew patch for having a more reasonable form during release node edits:
Comment #31
webchickBased 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)?
Comment #32
dwwthanks 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...
Comment #33
dwwnew patch with a few minor fixes:
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. ;)
Comment #34
dwwnew 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".
in other news...
this is getting dangerously close to "needs review". ;) as far as i can tell, here's the remaining show-stopper TODO items:
Comment #35
dwwnew patch with lots of fixes:
still TODO:
Comment #36
dwwminor changes from last patch:
TODO items from comment #35 still need doing...
Comment #37
dwwfixes for validation:
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.sadly, i just uncovered a whole new area of work: the project browse pages are busted. :(
TODO:
just when i thought i was almost done... :(
Comment #38
dwwnew patch that fixes other bugs, the 5 TODO from above still hold...
Comment #39
dwwnew patch based on initial dww + nedjo debugging session. ;) browse by category is now basically working:
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.
Comment #40
dwwwoo hoo, another quality session w/ nedjo and i think nearly all of the browse-by stuff is working now. ;)
TODO:
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.
Comment #41
dwwComment #42
dwwi'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:
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...).
Comment #43
dwwsome TODO items from IRC feedback:
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
Comment #44
drewish CreditAttribution: drewish commentedone 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?
Comment #45
drewish CreditAttribution: drewish commentedon 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.
Comment #46
dwwthanks 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
Comment #47
drewish CreditAttribution: drewish commentedhumm, 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.
Comment #48
dwwi'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
Comment #49
dwwhere'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...
Comment #50
dwwnew patch that fixes most of the TODO items from comment #43:
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.
Comment #51
dwwi 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... ;)
Comment #52
dwwnew, possibly final version of this patch before it's RTBC:
Comment #53
dwwa few minor items that need fixing:
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.
Comment #54
dwwi 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...
Comment #55
dwwfixed 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
Comment #56
dwwfixed 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:
Comment #57
dwwwhoops, sorry, i pasted the wrong cvs checkout command above. should be:
(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).
Comment #58
chx CreditAttribution: chx commentedI 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.
Comment #59
dwwheine 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.
Comment #60
dwweverything 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?
Comment #61
dwwafter 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. ;)
Comment #62
(not verified) CreditAttribution: commented