Do a proper page/code split of project_issue

I started working on this:

http://drupal.org/cvs?commit=192708
http://drupal.org/cvs?commit=192710
http://drupal.org/cvs?commit=192718

I wanted to get some of these done before really working on #423304: Add a per-project default component setting to simplify the UI when submitting issues in depth. Still lots more to go, but it's something... ;)

subscribe

FYI, I've had a related issue with modules (og) doing stuff in hook_init() which trigger node_access() which doesn't find project_issue_access (being in issue.inc, and project_issue_init() not fired yet) and thus returning FALSE.

Duly noted, thanks for the update. I'll be sure to keep an eye out for other hook implementations that need to live in the .module file itself.

Tagging for #439958: Document issues left before 6.x official release

Marked both of these duplicates:
#479510: Fatal error: Call to undefined function project_issue_comment_view()
#480034: Fatal error: Call to undefined function project_issue_state() ...

There are so many ways this is broken depending on what modules you install and their weights, that I'm bumping this to a critical bug...

subscribe

On the plane today, I worked on fixing the OG integration bugs from #376377: Issue + organic group incompatibility and the broader problem of the code split in here. I'm not yet done, but this seems a lot better. Here's what I did:

A) Moved hook_access() and hook_load() implementations into project.module and project_issue.module (already committed, see #376377-20: Issue + organic group incompatibility).

B) Moved code for the issue statistics pages from issue.inc to includes/statistics.inc

C) Moved code for the issue subscription pages from issue.inc to includes/subscribe.inc

D) Moved code for the JS callback when switching projects during an issue comment from issue.inc into includes/comment.inc -- however, before I commit this, I'm wondering if I should do the history-preserving CVS hack to move comment.inc to includes/comment.inc first...

E) Moved all code related to the issue node form into includes/issue_node_form.inc from issue.inc. The core node hooks now live in project_issue.module, but hook_form(), hook_validate() and hook_insert() all just load issue_node_form.inc and call private methods in there (since they're pretty huge).

F) Moved all code needed when viewing an issue node from issue.inc into includes/issue_node_view.inc. The core hook_view() implementation now lives in project_issue.module, but it's just a thin wrapper to load issue_node_view.inc and invoke a (quite large) private helper method to do the heavy lifting.

G) Moved code related to hook_nodeapi() for project_project nodes from project_issue.module into includes/project_node.inc.

Before:

After:

Any objections to any of this? Seems a little silly to post all the patches for this, since they're really just moving functions around and touching a few things in hook_menu() or hook_theme(). Really, I just want reviews on the strategy of the split outlined above and the filenames I'm using.

Just so I've got a nice backup, here are the patches for all of that. ;) I'm not sure they'll apply cleanly if you don't do them in order...

Note: I'm specifically *NOT* "cleaning up" or otherwise modifying any of this code "while I'm at it". That can all happen in followup commits. I don't want to confuse things. These patches just move code around, and fix hook_menu() and hook_theme() where appropriate to point to the new locations.

D) yeah, if it's not too hard, we should preserve the comment.inc history.

E & F) i think the core convention is more to put all regular pages in a pages.inc file. i'm not opposed to doing it differently, i just wanted to check with the reasoning here -- is it mostly because we have so much form/view code that a further split makes sense?

also (and possibly related to E & F), do you feel that the split above is forward-looking for D7?

Right, we can move the existing comment.inc into files first, and preserve this history. It's a bit of a hack, and it can confuse git/bzr mirrors, but it's not that terrible. We're going to want a comment.inc even in a sane code split, since that's a ton of code that's only needed on certain pages.

Re E/F: core's standard way is pretty coarse-grained. The point of the page split is to only load code you actually need to render a given page. The view tab and the edit tab on an issue node are completely separate page loads, that require completely different code. The directive around the page split is not to do anything that's less than ~50 lines since it's not really worth loading a separate file for that little code. However, E is 437 and F is 182 lines each, so each one is more than large enough to qualify for its own file, and none of E needs to be loaded during F or vice versa.

Re: forward looking for D7 -- absolutely. The registry in D7 makes it even easier to have your code in separate files and only loaded on demand. Instead of having to manually include the files you need in the right spots and define locations in hook_menu() and hook_theme(), that's all done automatically by declaring all the files in the .info file. But, it's still a good idea to only put code in a .inc file that needs to be loaded together. Ultimately, that's the performance win of *all* of this effort.

sounds good.

- Did the history-preserving cvs_rename from comment.inc and mail.inc into the includes directory
- Committed B, C, E, F and G to HEAD

Setting back to active for:

D) Actually, this JS callback code is *not* needed when comments are rendered. It's only used as an AJAX callback. A few alternatives for where to move this:
D1) project_issue.module itself
D2) includes/autocomplete.inc
D3) its own file in includes -- probably not

H) There's still a bunch of code in issue.inc that needs to move out of there. We should just remove issue.inc before this issue is marked "fixed".

I) project_issue.module has some code that could be moved into .inc files.

J) comment.inc and mail.inc probably need some lovin'.

H is still causing fatal PHP errors in some module combinations, so I'm still leaving this marked as a critical bug. Perhaps I'll work on this some more tomorrow on the plane, so I'm still leaving this assigned to me.

subscribing

Any updates on this?

I posted a related project_issue patch at #429792: Add API for manipulating issue status options to fix a bug in the d.o testing installation profile.