The signup.theme file (version 1.10) has a series of comments on lines 32 to 41 that lead me and possibly others to assume that editing that .theme file directly is ok. I really think we should change that so that people who haven't worked with this new .theme concept won't start hacking away at the signup form right there in the module and end up loosing their changes with the next upgrade.

/**
 * You can alter this user signup form to suit your needs -- feel free
 * to alter any elements in this section, remove them, or add any
 * others.
 *
 * In order for the form to be rendered properly, the name of the form
 * element must be $form['signup_form_data']['NameOfDataField'], where
 * NameOfDataField is replaced with the actual name of the data field.
 * We suggest that the displayed name of the field (the '#title'
 * property) be the same as the name of the data field, but it's not
 * required.  See below for examples.
*/

I'm willing to write a handbook page about this if you'll briefly explain the best practice for the process of adding fields to this module? Then we can just snap a linky right there in the help text. I just don't know if encouraging users to build their own custom module for editing this form is the right way to go, or whether it should be done in the site's theme.

CommentFileSizeAuthor
#13 168094_theme_signup_user_form_comment.13.patch1.03 KBdww
#9 signup_theme_modified_instructions.patch2.84 KBsenpai
#3 signup_theme_modified_instructions.patch1.65 KBsenpai

Comments

dww’s picture

dww
we/he/they
commented
Title: Signup.theme has some really bad instructions in it. » Improve comments in signup.theme
Component: Code » Documentation
Category: bug » task
Priority: Critical » Normal

Good grief, this is hardly a "critical bug"... Please don't abuse the issue queue or maintainers will lose respect for you and will start ignoring your comments.

The "best practice" is to define this theme function in your own theme, just like any other theme function you want to override in your theme. Hardly seems worthy of an entire handbook page about this. All this needs is a small patch to the comment in the code...

BTW, this whole practice of using a theme function was a hack done under time pressure when this module was first written. Some day, it'd be good to replace it. See http://drupal.org/node/29568 and http://groups.drupal.org/node/3430 for more.

senpai’s picture

senpai commented

@dww, I only marked this as a critical because of an IRC discussion with two other well respected community members who felt that what I'd discovered was too bad to ignore. This text makes it seem like editing the module directly is the preferred option, rather than the drupaly hook_ approach.

I just wanted to point out that since it fooled me, and I already know about the dangers of editing modules directly, it should be changed asap so others don't follow the wrong path to enlightenment.

It's really the .theme file that lives in some of these newer modules that doesn't seem to have any Handbook documentation. I couldn't find any references to the .theme files, and while it looked like it's purpose was to remove presentation from logic, it also seemed like the module was encouraging the direct editing of it's contents, shich obviously would not be good when version 5.x-2.2 hits the mean streets.

I read the links you provided for possible changes to signup.mod, and node/3430 sounds really cool.

senpai’s picture

senpai commented
Assigned: Unassigned » senpai
Status: Active » Needs review
StatusFileSize
new signup_theme_modified_instructions.patch1.65 KB

Here's a patch that changes the explanatory text for the signup.theme file. Also, I corrected a couple of Coder fixes at the same time, but they're minor.

Still ToDo: Explain what happens to the data if someone form_alters the user signup form and adds more fields to the form.

[No functional code was changed during the creation of this patch.]

senpai’s picture

senpai commented
Version: 5.x-2.1 » 5.x-2.x-dev

I forgot to change the issue's Version. The patch in #3 should be applied to 5.x-2.x-dev

dww’s picture

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

please use diff -up

dww’s picture

dww
we/he/they
commented

Further, these instructions are pure wrong. ;) You do *not* use hook_form_alter(). You define the form elements directly in your theme function. It's weird, I know, but that's how it works.

senpai’s picture

senpai commented

Woops! I rolled that one against HEAD, but used TextMate rather than the command line. I didn't even catch that the patch looked weird before I uploaded. I'll do it again.

What do you mean, you define them in your theme function? How are you gonna change the values, defaults, or number of available fields in a form at the templating stage? Or maybe you're suggesting that users should write a new theme function in their custom module instead of a form_alter ? I mean, I successfully did it with a form_alter, but then again, I had dmitri's help to do it ;-)

dww’s picture

dww
we/he/they
commented

I've never tried using form_alter() for this, and have no idea if it works. The way this has been supported in signup since the 4.6.x days was to define your own form elements in that theme function.

senpai’s picture

senpai commented
StatusFileSize
new signup_theme_modified_instructions.patch2.84 KB

I rerolled the patch from #3 to be -up compliant. Still have to figure out what these instructions should say.

senpai’s picture

senpai commented

Ok then. Let's assume that a developer was to begin defining new form elements for the signup.module. Where would they put them? One can't edit the signup.theme file directly, or changes to the site will be lost at the next version upgrade. Can a custom module be created that will change or affect the theme_signup_user_form() theme function (the one in signup.theme) ?

I don't know of any way to do this, so if someone knows, please enlighten me. I know an override could be done in template.php, but that doesn't allow a developer to add or alter the elements of a form, only to change the way the existing elements are rendered to the page.

dww’s picture

dww
we/he/they
commented

@Senpai: The existing comment isn't that far off. Read it. The "best" way to alter this for your site is to put [themename]_signup_user_form() in your theme's template.php file, and define exactly what form elements you want. To quote the existing, accurate documentation:

* You can alter this user signup form to suit your needs -- feel free
* to alter any elements in this section, remove them, or add any
* others.

I repeat: THIS IS WEIRD. It's not the "normal" usage of theme functions. But it works, it's how signup was originally written (long before my time), and that's what the documentation should say.

Cheers,
-Derek

senpai’s picture

senpai commented
Status: Needs work » Postponed

Oh my. Oh wow. No wonder this never made any sense to me!

I had tried using form_alter in my template.php, which, as we all know, would never have done anything but sit there and look like pretty code. I never once thought that I could copy a theme function into a template.php and then affect the number of fields in a form. Wow!

Ok, lemme actually try this in a real life sandbox so I'm sure I understand how it works, and then I'll take a stab at attempting to explain this theme_function_form_alteration process a bit better for the kiddies.

dww’s picture

dww
we/he/they
commented
Assigned: senpai » dww
Status: Postponed » Needs review
StatusFileSize
new 168094_theme_signup_user_form_comment.13.patch1.03 KB

How's this?

dww’s picture

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

Committed to HEAD and DRUPAL-5.

senpai’s picture

senpai commented

That will totally work. Good job consolidating the descriptive text.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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