move all issue tracking into a separate module

FYI: http://drupal.org/node/74997 is the core patch for db_table_exists()

kjartan (aka natrak) sent me some useful feedback on this. i'm posting it, along with my replies here. attached is a slightly modified patch to address some of the points he raises. also, i realized http://drupal.org/node/74608 was being perpetuated with the original patch, so i'm fixing that, too. be sure to update modules/project_issue (since i committed a few minor changes there, too).

one other area of separation that should be addressed (though isn't exactly urgent) is that currently, project/project.inc is still responsible for generating the issue-tracking-related links on the project node. it kind makes more sense to do that in project_issue. however, you have much less control via hook_nodeapi where such links get added if you're modifying another node's body (basically, you can either prepend or append, but that's it). so, i'm leaving it in project for now (it's already conditional on if $node->issues is set), so that seems good enough. we can revisit this in the future, but i don't see it as a show stopper for getting this into cvs.

so, onto kjartan's comments and my replies...

On Jul 22, 2006, at 11:18 AM, Kjartan Mannes wrote:

project.module
- Function project_page() seems like dead code and should be removed.

there's probably a lot of dead code. that's been dead a while, and i was trying to change the minimum possible with this patch. i'd rather remove dead code as separate issues with separate patches...

- After refactoring project_project_nodeapi() should replace project_nodeapi().

that was actually a conscious decision:

* NOTE: there's currently only 1 value left here, but I'm planning to
* make project_release its own node-type, so we'll probably want to
* leave this plumbing in place for that... -dww

also, see http://drupal.org/node/69349 for why i put the code like that in the first place. there were quite a few bugs, including some serious ones, based on confusion about the fact that the nodeapi() hook is per-module, not per-nodetype. so, i wanted to organize the code like this so that the module-specific hook just farmed out the calls to node-type-specific methods (some of which had already been implemented but weren't being called, etc).

- project_settings():
variable_set('project_reply_to', "issue-replies@...");
Should probably have another default.

good one. ;) actually, those 3 variable_set() calls in project_settings() were a tiny bit of stray debugging code i added at the last minute to fully test the project_update_4() DB conversion update... all 3 are now gone with this updated patch.

project_issue.module
- project_issue_form_alter() and project_issue_alter_project_form() should probably be merged.

maybe this is just my personal coding style opinion, which isn't shared by others, but i vastly prefer lots of smaller functions, with (hopefully) reasonable names, that do smaller tasks. i don't like giant, monolithic functions that try to do everything with lots of switch() statements, nested if()s, etc, etc. it might be slightly less efficient to hit the extra function calls, but frankly, i'm more concerned about human time understanding, debugging, and maintaining the code than how fast computers can execute it.

Not sure its checking for the right node type either

you mean

  if ($form_id == 'project_project_node_form') {

??

yeah, that's the right form id... you can try it yourself if you don't believe me. ;)

- All the *_nodeapi() functions should probably be cleaned up and re-structured. Seems to be rather many functions to do very simple things.

see above. yeah, in this case, it's partly driven by the fact that these are functions moved over from node-type hooks from project.inc, so i'm just maintaining the same basic code organization as i moved everything over. however, i honestly prefer it this way. i can easily look at project_issue_project_nodeapi() and see what hook operations are being implemented by this module, since the whole function fits on a single screen in my editor. each of the functions that handles a specific operation are small, easily understood, and tested/verified.

anyway, that's the motivation. if there are compelling reasons not to do it this way, so be it, but that's how i'd prefer to leave things, and why.

thanks for the review!
-derek

i just realized i had ripped out some queries against {project_projects}.issues, since i moved that to {project_issue_projects}. however, there are a few places where this is a problem:

1. project_projects_select_options() now returns all projects, even ones where issue tracking is disabled. this is good for the "project navigation" block, since it shouldn't matter if you've enabled issue tracking on a module to find it with that block (not sure anyone actually uses this block, but whatever). however, this is bad for the drop-down menu for what projects are available when dealing with issues (from project_issue/issue.inc). i'll probably refactor this code a little bit so that you can pass an optional argument specifying if you want it to restrict to nodes with issue tracking enabled or not... also, see http://drupal.org/node/57709 about the fact there are a few other places in issue.inc where we should be using this method and we're not...
2. in project_page_overview(), we no longer construct the $project objects with the $project->issues field, so the "Bugs and feature requests" link on the main project listing pages aren't getting made. this one will be slightly more work (since that's such a complicated, weird method to begin with), but it won't be too hard.

stay tuned, i'll submit a new patch shortly.

new patch, fixes both of the problems i mentioned in #3. while i was at it, i fixed a bug where if the taxonomy stuff was not enabled, you never got the link for "Bugs and feature requests" on the main project listing page, even for projects with issue tracking enabled. this is exactly backwards from what the comment in the code was claiming. ;) anyway, it's fixed by this patch, too.

drat, upon futher testing of the taxonomy case, there's a syntax error in one of the SQL queries in the last patch. this fixes it...

bumping prio. now that i've done this reorganization, i've found/fixed a fair # of other issues, some of them critical. i'd rather commit this and move forward with the fixes to those in the new layout, instead of making a series of new patches in this thread as i maintain 2 different copies of the project source, posting 2 patches for each fix to the other bugs (1 for each layout of the code), etc, etc.

let's get this done. ;) thanks.

quick note about recent CVS activity related to this issue:

• killes was kind enough to help me save the revision history for comment.inc, issue.inc, and mail.inc. i had him directly copy the ,v files in the repository into the project_issue directory. then, i removed those 3 files from each of the older branches (everything before 4.7). this is basically what my cvs_rename.pl script does.
• i then applied the small diffs to mail.inc and comment.inc that were required because of the move to project_issue/ as commits on the DRUPAL-4-7 branch
• in order to create a valid DRUPAL-4-7 branch on the rest of the files in the project_issues directory, i had to create a project node for project_issue (http://drupal.org/project/project_issue). as soon as i finished the cvs operations, i unpublished the node (though it's a good placekeeper for once this patch goes in).
• DRUPAL-4-7 is definitely the branch to use. i have *not* made a re-organization patch against HEAD, and i haven't written project_issue.module against HEAD's API. currently, there are only 2 real patches against project specific to core's HEAD API (hook_links and hook_settings). so, it'll be easy to "port" the reorganized project + project_issues modules to HEAD's API once the dust settles from the initial reorganzation.

i asked chx and kjartan's opinions on all of this, and both thought this was a reasonable approach while waiting for more input from dries...

woo hoo! committed to DRUPAL-4-7. ;) thanks for the reviews kjartan!

i published http://drupal.org/project/project_issue, and updated the text at http://drupal.org/project/project.

now, i just have to get the HEAD version of each cleaned up and working, but that's a lower priority task. of course, there's also the whole issue of updating d.o with all of this goodness, but that should probably wait until at least the flurry of fixes i'm going to commit in the next few days (sadly, i'm on jury duty tomorrow, so i won't be available for much hacking)...

for the interested reader, here's a new reorg patch which applies cleanly against HEAD that i'm about to commit. in the process of resolving conflicts (mostly dealing with the hook_link() changes for HEAD), i also generated a patch for http://drupal.org/node/75410 so that i can port project_issues.module to hook_link() whenever i feel like it (which will not be immediately -- for now, i'm going to keep the HEAD version of project_issue as a development branch against the 4.7 core API, until the 4.8/5.0 code freeze or http://drupal.org/node/58066 (new method for project releases/versions) is done and in effect...

applied to HEAD. :) phew. i also fixed up the text on both the project and project_issue node's body to match all the changes...

you can't be the *most* recent versions, since i committed the fix for the duplicate project_issue_nodeapi() last night (revision 1.172.2.11, project_issue/issue.inc)...

in terms of your "blank screen of death", have you looked in [site]/admin for the logs? are there errors getting reported there? are you sure you're not running out of memory b/c of your php configuration? see http://drupal.org/node/70438 for more on this.

if you're still having problems, please just open a new ticket, instead of adding to this one. thanks!

-derek

oh, and i should mention i recently fixed some php5-only problems in project.module, maybe that's what you're hitting. i also just found some more known issues with project.module on php5... i'll be opening an issue and posting a patch in a few minutes about that, too...

wersmart: FYI -- the php5 problems i was talking about are described and fixed here: http://drupal.org/node/75571...

sweet. ;) i guess we were both typing that follow-up at the same time... glad to hear there's no real problem here.