Closed (fixed)
Project:
Signup
Version:
6.x-1.0-rc1
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
6 Nov 2008 at 10:53 UTC
Updated:
29 Nov 2008 at 02:41 UTC
Jump to comment: Most recent file
I've committed the initial D6 port to HEAD (#222217: Port to 6.x), but I haven't yet split the module up into separate .inc files for the various menu callbacks. This would be nice to do before the 6.x-1.0 release.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 330828_signup_module_split.7.patch | 142.14 KB | dww |
| #6 | 330828_signup_module_split.6.patch | 141.95 KB | dww |
| #5 | 330828_signup_module_split.5.patch | 141.94 KB | dww |
| #3 | 330828_signup_module_split.3.patch | 141.2 KB | dww |
| #2 | 330828_signup_module_split.2.patch | 141.14 KB | dww |
Comments
Comment #1
dww#330829: Split theme functions out into .inc files. should land first, to avoid patch conflicts.
Comment #2
dwwHere's the wc (word count) output in the includes directory after applying this patch:
Not that date.inc, event.6x-2.inc, scheduler.inc and views.none.inc already exist, and are untouched. Should hopefully be pretty self-documenting what these files are for, if not, they all have @file PHP doc comments.
Any objections or suggestions for improvement, either in the filenames themselves, or the basic layout/split?
Comment #3
dwwFound a missing module_load_include() as I clicked around in various combinations.
Comment #4
dwwAt the suggestion of merlin in IRC, I just renamed includes/views.none.inc to includes/no_views.inc. That seemed like a clear win, so I did it. Still soliciting feedback on the rest of this...
Comment #5
dwwMissed a spot in panels/content_types/signup_form.inc. Panels support still seems broken in general, but that's for #330824: Test/fix panels integration, not here.
Comment #6
dwwMinor reroll after #330824: Test/fix panels integration landed.
Comment #7
dwwRerolled after I committed #333264-4: Bugs when editing a node since hook_nodeapi() no longer has a fully-loaded $node.
Here's the (hopefully final) wc output of signup.module and includes/* from before and after the patch.
before
after
The extra size is from the @file PHP doc comments in each .inc file, the extra lines in hook_menu() to specify the include files for various page handlers, the handful of extra calls to module_load_include(), and mostly from some extra PHPdoc comments I was adding to functions as I moved them. I guess I should have kept all the comment fixes in another patch... oh well.
Comment #8
jrbeemanI haven't tested it yet, but I like the structure. Does this restructuring involve moving any theme-related stuff out of the /theme folder? It doesn't appear to, but I want to check on that, as I like that convention.
Comment #9
dwwNope, all the theme stuff still lives in theme/* with two minor exceptions:
theme_signup_settings_view_label() now lives in includes/admin.settings.inc
theme_signup_token_help() now lives in includes/token_help.inc
Neither of those fit in with any of the theme .inc files from #330829: Split theme functions out into .inc files., and it seemed silly to make separate .inc files for these little theme functions, so I just put them in with the .inc file for the module code they went with.
Comment #10
dwwI committed this to HEAD. Before I did, I created a
DRUPAL-6--1-0-BETA1tag (although I'm not going to create a release node for that tag). So, if you're ever debugging something in one of the include files and want to know the history, do this:and search for the function you care about.
I also created a 6.x-1.0-rc1 release after I committed this, so folks can test that out as the official release candidate.