I've committed the initial D6 port to HEAD (#222217: Port to 6.x), but I haven't yet split the theme functions out into template files or otherwise ported those to the D6 theme API. This would be nice to do before the 6.x-1.0 release.

CommentFileSizeAuthor
#2 signup.module.rej_.txt1.11 KBstborchert
#1 330829_signup_theme_split.1.patch42.21 KBdww

Comments

dww’s picture

dww
we/he/they
commented
Title: Split theme functions out into .tpl files. » Split theme functions out into .inc files.
Assigned: Unassigned » dww
Status: Active » Needs review
StatusFileSize
new 330829_signup_theme_split.1.patch42.21 KB

Upon closer inspection, it doesn't seem like many of the signup theme functions make much sense as templates. So, instead of .tpl.php files, this patch splits them out into separate .inc files, grouped by their basic function. This way, we only have to parse and load the theme functions we need to render various kinds of pages (like #330828: Split module code into separate .inc files for D6 menu API). Here's the proposed new theme/README.txt file that describes what goes where:

This directory contains separate include files for all of the theme
functions provided by the Signup module.

email.inc
Functions related to sending emails.

no_views.inc
Functions used when the Views module is not enabled.

node.admin.inc
Functions for the per-node signup administration page (node/N/signups).

node.inc
Functions for displaying signup-related information when viewing nodes.

signup_administration.inc
Functions for the site-wide signup administration page (admin/content/signup).

signup_form.inc
Functions related to the form presented to users when they signup.

The files seem pretty balanced this way. Here's the word-count (wc) output for theme/*:

      23      73     591 theme/README.txt
      77     342    2371 theme/email.inc
      43     174    1362 theme/no_views.inc
     105     377    2869 theme/node.admin.inc
     132     521    3713 theme/node.inc
      80     250    2298 theme/signup_administration.inc
      94     505    3236 theme/signup_form.inc
     554    2242   16440 total

Any thoughts or complaints before I commit this?

stborchert’s picture

stborchert
he/him
Primary language German
commented
StatusFileSize
new signup.module.rej_.txt1.11 KB

Tested with signup-6.x-1.x-dev.tar.gz (signup.module,v 1.187 2008/11/10 16:04:10; md5_file hash: cf0d958b0aea3d17244967373af698a7; November 12, 2008 - 10:15 [GMT]).

Patch doesn't apply cleanly (fuzz factor 18 and rejected content). As a result of that the function theme_signup_broadcast_sender_copy() could not be removed from signup.module (see attachment).
After removing it manually, all works fine.
Unforunately I couldn't test email functions and themes but from looking into the patch I would say they should work as expected :-)

One minor note: there should be an information in README.txt (the main one) where to find the theme funtion for the user signup form (signup_form.inc). Otherwise it could be a problem for some people to find it.

If the patch applies to HEAD and the information is added it is RTB(t)C.

 Stefan

dww’s picture

dww
we/he/they
commented
Status: Needs review » Fixed

After a brief debate in IRC about prefixing all the file names with "signup.", I decided to just commit this as-is. I did fix the top-level INSTALL.txt file to mention the new location of theme_signup_user_form(), and added a line about theme information to README.txt, too. Committed to HEAD.

Status: Fixed » Closed (fixed)

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