OK, I'm posting "work to date". Note, I believe I've done all required to complete the port. However, without having a full test set-up to test every aspect on, I expect some small fixes. But, it's up for review at least.

Comments

AjK’s picture

StatusFileSize
new10.6 KB

Fixing a bug in hook_form_alter() found while testing against project D5 port

AjK’s picture

Status: Needs review » Needs work
AjK’s picture

StatusFileSize
new36.86 KB

doh, ignore patch in #1, rolled against wrong version. Here's the right one. Still need to roll in Zen's two new patches from the DRUPAL-4-7--2 branch as noted above (hence "needs work" status)

dww’s picture

everything from DRUPAL-4-7--2 branch (as of DRUPAL-4-7--2-1) is now in HEAD (http://drupal.org/cvs?commit=47203).

please re-roll this patch against HEAD (since there were a bunch of changes in the last few days, too).

thanks!
-derek

dww’s picture

Status: Needs work » Needs review

AjK posted a new patch for this here: http://drupal.org/files/issues/cvslog_d5_15_12_2006.patch
(it's in a separate issue, but that's duplicate... let's keep using this one, instead).

thanks,
-derek

dww’s picture

Status: Needs review » Needs work

this is broken in a few important ways (mostly bugs in hook_menu()). i'll roll a new patch later, i'm just doing more testing now before i do.
overall, this patch is a great start, don't get me wrong. ;)

thanks,
-derek

dww’s picture

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

haven't had a chance for the most thorough testing yet, but this at least fixes some of the bugs in hook_menu(). it also includes a cvs.info file.
needs much more testing, but at least it gets past most of the most trivial errors when running it on a live site. ;)

dww’s picture

Version: 4.7.x-2.x-dev » 6.x-1.x-dev
Assigned: Unassigned » dww
Priority: Normal » Critical
Status: Needs review » Needs work

found a bunch more problems with the crazy FAPI stuff for release nodes. :( i'm working on it.
also, i discovered a core bug in the process: http://drupal.org/node/104280 ;)

cheers,
-derek

dww’s picture

StatusFileSize
new50.98 KB

yay, my patch for #104280 is already committed. ;) that's nice.

furthermore, with some help from eaton in IRC, i got the evil FAPI stuff figured out. basically, in 4.7.x, form_builder() was responsible for looking at $_POST directly. in 5.x, it expects everything to be stuffed into '#post' ahead of time. so we just do that ourselves, and all appears to be well.

also, the taxonomy hackery was busted, thanks to taxonomy.module revision 1.320:

date: 2006-10-20 20:55:03 +0000;  author: drumm;  state: Exp;  lines: +4 -2;  commitid: 3994453937674567;
#24023 by chx. Allow multiple select options with the same value and implement it for taxonomy selection.

now, instead of a simple array of options, there's this crazy array of option objects, instead. :(

so, i added a _cvs_get_option_from_taxo() helper method to search the array of objects for the desired taxonomy option and return it.

after fairly heavy (but not exhaustive) testing, all the crazy N-page crap for adding release nodes seems to be working now. ;) a brief survey of other parts of cvs.module's functionality (cvs-application page, repo admin stuff, etc) is also working thanks to the menu fixes also included in this patch.

the final major TODO item is to finalize our plans for an /admin/project block and move everything there. as it is now, you can't easily get to the usual admin/cvs page on your own, since the admin page thinks you just want a cvs block, with only 1 element -- the repo editing pages.

  • do we want to put all the CVS account stuff under /admin/user?
  • should we leave all the CVS stuff together in an /admin/cvs block, and make sure that the accounts stuff is its own page like /admin/cvs/accounts (with subtabs under that)?
  • should we use /admin/project and put all the CVS stuff under that (e.g. /admin/project/cvs-accounts vs. /admin/project/cvs-repositories vs. /admin/project/settings/cvs?

it's just not clear to me what's the best layout for all these pages.

however, once that's resolved, i think this would be fully working now. ;)

cheers,
-derek

dww’s picture

Status: Needs work » Needs review
StatusFileSize
new54.62 KB

after discussion w/ merlinofchaos in IRC, we decided it was best to use a new /admin/project block for all the project-related admin pages (settings for project, project_release, project_issue and cvs, plus the cvs accounts and cvs repositories pages).

i also discovered a bug in how we were using $may_cache in hook_menu() (since it was screwing up my attempts to get this project block working). i'll back-port that to 4.7.x-* in a separate issue.

as far as i know, this completes the patch. everything appears to be working ok, though more reviews and testing would be most appreciated!

dww’s picture

StatusFileSize
new54.58 KB

left a little debugging output in the last patch.

dww’s picture

committed to HEAD and installed on s.d.o (to make it easier for others to test).
i'm still leaving this issue as "needs review", since i'd appreciate more reviews and testing...
thanks,
-derek

dww’s picture

Status: Needs review » Needs work

hunmonk pointed out that the "CVS access" tab on project nodes is fubar.

a) there's form_id vs. function name disagreement for "cvs_project_access" vs. "cvs_project_access_form" in the "theme_", "_validate", and "_submit" handlers for that form.

b) the confirm form (at least for deleting access) is fubar. see http://drupal.org/node/64279#confirm_form

hunmonk’s picture

Status: Needs work » Needs review
StatusFileSize
new3.07 KB

attached should fix both 'cvs access' tab bugs. not tested

dww’s picture

StatusFileSize
new3.41 KB

not quite... sorry, the confirm_form() upgrade docs were a little broken (which i've now fixed). ;)
new patch based on hunmonk's previous one that gets the confirm_form() stuff right. tested locally.

hunmonk’s picture

Status: Needs review » Reviewed & tested by the community

code style looks good.

dww’s picture

Status: Reviewed & tested by the community » Fixed

tested on s.d.o. all good. committed to HEAD as revision 1.134.

dww’s picture

Status: Fixed » Needs work

oh, but i was still fishing for reviews of the diff for the 5.x port. ;)

cvs diff -kk -up -r DRUPAL-4-7--2 -r HEAD

thanks,
-derek

dww’s picture

Status: Needs work » Needs review
dww’s picture

Status: Needs review » Fixed

i'm going to give up on people reviewing these porting patches, so i'm going to mark them as fixed now.

Anonymous’s picture

Status: Fixed » Closed (fixed)