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.

Comments

dww’s picture

Status: Active » Postponed

#330829: Split theme functions out into .inc files. should land first, to avoid patch conflicts.

dww’s picture

Assigned: Unassigned » dww
Status: Postponed » Needs review
StatusFileSize
new141.14 KB

Here's the wc (word count) output in the includes directory after applying this patch:

     223    1205   10527 admin.settings.inc
     202     661    6328 admin.signup_administration.inc
     158     572    5424 broadcast.inc
     127     559    4681 cron.inc
     386    1573   14171 date.inc
     102     329    2932 event.6x-2.inc
     171     604    5853 node_admin.inc
      69     206    2148 node_admin_summary.inc
     225    1254   10021 node_form.inc
     191     909    7347 node_output.inc
     120     550    5147 node_settings.inc
     152     366    3539 scheduler.inc
      40     125    1070 signup_cancel_form.inc
     192     793    6544 signup_form.inc
      67     212    1638 token_help.inc
     141     539    4309 views.none.inc
    2566   10457   91679 total

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?

dww’s picture

StatusFileSize
new141.2 KB

Found a missing module_load_include() as I clicked around in various combinations.

dww’s picture

At 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...

dww’s picture

StatusFileSize
new141.94 KB

Missed 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.

dww’s picture

StatusFileSize
new141.95 KB

Minor reroll after #330824: Test/fix panels integration landed.

dww’s picture

StatusFileSize
new142.14 KB

Rerolled 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

    2688   12040  102962 signup.module
     386    1573   14171 includes/date.inc
     102     329    2932 includes/event.6x-2.inc
     141     539    4305 includes/no_views.inc
     152     366    3539 includes/scheduler.inc
    3469   14847  127909 total

after

    1043    4719   38903 signup.module
     223    1205   10527 includes/admin.settings.inc
     202     661    6328 includes/admin.signup_administration.inc
     158     572    5424 includes/broadcast.inc
     127     559    4681 includes/cron.inc
     386    1573   14171 includes/date.inc
     102     329    2932 includes/event.6x-2.inc
     141     539    4305 includes/no_views.inc
     171     604    5853 includes/node_admin.inc
      69     206    2148 includes/node_admin_summary.inc
     230    1267   10183 includes/node_form.inc
     191     909    7347 includes/node_output.inc
     120     550    5147 includes/node_settings.inc
     152     366    3539 includes/scheduler.inc
      40     125    1070 includes/signup_cancel_form.inc
     192     793    6544 includes/signup_form.inc
      67     212    1638 includes/token_help.inc
    3614   15189  130740 total

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.

jrbeeman’s picture

I 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.

dww’s picture

Nope, 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.

dww’s picture

Version: 6.x-1.x-dev » 6.x-1.0-rc1
Status: Needs review » Fixed

I committed this to HEAD. Before I did, I created a DRUPAL-6--1-0-BETA1 tag (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:

cvs annotate -r DRUPAL-6--1-0-BETA1 signup.module

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.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.