Posted by dww on March 17, 2009 at 11:19pm
9 followers
| Project: | Project issue tracking |
| Version: | 6.x-1.x-dev |
| Component: | Miscellaneous |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | active |
| Issue tags: | 6.x-1.0 blocker |
Issue Summary
The *.inc files in project_issue are weirdly organized, and useless in terms of conditionally loading the code we need. Now that we're ported to D6 (and the dust has mostly settled), we can make use of the D6 menu system to do a proper page split into sane .inc files. I'd like to do this before the 6.x-1.0 release. This issue is specifically about project_issue itself. The rest of project* is over at #390856: Do a proper page/code split of project.
Comments
#1
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... ;)
#2
subscribe
FYI, I've had a related issue with modules (og) doing stuff in
hook_init()which triggernode_access()which doesn't findproject_issue_access(being in issue.inc, and project_issue_init() not fired yet) and thus returning FALSE.#3
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.
#4
Tagging for #439958: Document issues left before 6.x official release
#5
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...
#6
subscribe
#7
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:
% wc project_issue.module issue.inc1693 7185 63171 project_issue.module
1287 5378 47297 issue.inc
2980 12563 110468 total
After:
% wc project_issue.module issue.inc1694 7129 62683 project_issue.module
276 1255 9524 issue.inc
1970 8384 72207 total
% wc includes/comment.inc includes/issue_node_form.inc includes/issue_node_view.inc includes/project_node.inc includes/statistics.inc includes/subscribe.inc
95 352 3506 includes/comment.inc
437 1642 15119 includes/issue_node_form.inc
182 765 6907 includes/issue_node_view.inc
71 291 2919 includes/project_node.inc
132 754 6291 includes/statistics.inc
165 598 5558 includes/subscribe.inc
1082 4402 40300 total
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.
#8
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.
#9
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?
#10
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.
#11
sounds good.
#12
- 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.
#13
subscribing
#14
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.
#15
The issue mentioned in #5 still exists in the latest version, isn't there a fix for this yet?
#16
@markwittens #15: nope. See comment #12 above. Hence "critical bug" + "active". ;)
I'm not currently working on this, so having it assigned to me is misleading. If anyone wants to pick up the torch here and roll a patch for any of the remaining things outlined above, please do. Thanks!
#17
Old kjartan code, fun.
I slogged my way though mail.inc, renaming functions and checking how stuff was called.
Another major thing I touched on was the anchor parsing stuff in there. It had obviously been carried around in a cargo-cult fashion with minimal changes since before 4.7, and had some bugs.
I traded the preg_replace with e flag (EWWWW!) for a preg_replace_callback.
I renamed the functions to be in the right namespace. I _'d one or two. Actually, nearly all of the functions in there SHOULD be _, but I spent most of the night on the lovely (cough) anchor parsing and wanted to get SOMETHING posted before I fell asleep.
I also tidied up the comments a bit and added some comment blocks to the functions that were completely devoid of them.
#143154: URL regex assumes no whitespace in closing tag and no nested html should be solved to most people's satisfaction (Especially as in D6 and later by default the html corrector runs first and does a bunch of normalization), although I left a commented out version of the old regexp (with the e flag removed) in case you want to minimize behavioral changes.
I also took mail.inc out of _init(). There actually weren't too many cases to worry about, nearly all of mail.inc's functions are only called from mail.inc.
#18
bdragon, you're amazing. yay! you're a brave soul going into mail.inc. ;) but i'm so grateful you did. i'm too tired to review this now, but it clearly needs it, since this all sounds like wonderful improvements that should go in soon. the next few days are crazy for me, and i'm going to be mostly AFK next week, but hopefully I can sneak this in. i doubt hunmonk will get a chance to look at it before i do, since he's in the middle of moving across the country. :/ are you interested in being a co-maintainer of project*? if so, once you find a thorough review for this, you could just commit it yourself. ;) let me know what you think...
thanks!
-derek
#19
Sure!
What with the stats work, my interest in improving performance on d.o, and my ongoing fascination with code archaeology, I think it makes sense.
#20
Talked with bdragon in IRC just now, and he's up to speed on our process, so he's got CVS access now. Yay!
In terms of this patch, I only saw one incredibly minor point:
A)
+ if (is_array($match) && !empty($match)) {wouldn't it make more sense to test empty() first then is_array()? don't you get a PHP notice if you is_array() something that doesn't exist?
otherwise, once this is tested on d6.p.d.o, it's RTBC. This can be bdragon's first commit. ;)
Hurray for another victim^H^H^H^H^H^H co-maintainer ;)
#21
One other (potential?) bug from #17:
B)
+ //$pattern = '@<a +([^ >]+ )*?href *= *"([^>"]+?)"[^>]*>([^<]+?)</a>@i';+ $pattern = '@<a[^>]*\shref\s*=\s*([\'"])([^>]+?)\1[^>]*>(.+?)</a>@is';
don't we want this?
$pattern = '@<a[^>]*\s+href\s*=\s*([\'"])([^>]+?)\1[^>]*>(.+?)</a>@is';to allow more than 1 piece of whitespace before the href, so we catch stuff like this:
<a href="/node/94000">stuff</a>(with 3 spaces between
<aandhref)?Or, is the idea that those extra spaces are going to be consumed by the
[^>]*anyway, so it's irrelevant? I guess we're not saving any of those parts of the regexp in ()s, so we don't care how the whitespace is matched...Bah, forget it. ;) I think we only care about (A) above. Assuming this is tested and working, I'm okay with all this.
#22
OK, feedback integrated and diff done with -w this time to keep trailing-whitespace-stripping out of the patch.
This one is now going on d6.p.d.o for testing.
#23
project_mail_digest() runs without error and generates mails in the expected format.
project_mail_reminder() runs without error and generates mails in (I assume, I didn't actually have any of these on hand to compare to) the expected format.
Issue creation generates mail in the expected format.
Issue followup generates mail in the expected format.
Mailhandler is not tested yet.
#24
I've never tested mailhandler. Not worth holding this up. Fire at will. ;). Thanks!!
When you commit, please set this back to active for all the unfinished business.
#25
Aye aye, cap'n!
Committed as http://drupal.org/cvs?commit=325962.
#26
Pushed http://drupal.org/commitlog/commit/1894/06e0878b050c9421ff63481103b9e214... which is going to hopefully going to fix some tests since we had a dependency in hook_perm() that wasn't loaded since hook_init() wasn't being called yet. Thanks marvil07 for the pointer!
#27
Is there a summary of what still needs to be done here, after commits in #25 and 26?