I've attached a patch that allows other modules to alter signup data before it's rendered via signup_build_signup_data(). This will allow other modules to inject relevant signup data.

Comments

jrbeeman’s picture

StatusFileSize
new1.6 KB

Please ignore that first patch - this one should be much more usable.

jrbeeman’s picture

StatusFileSize
new3.68 KB

Here's another patch rolled against HEAD as of today.

jrbeeman’s picture

There's a related issue for the Signup Status module that depends on this.

csc4’s picture

Status: Active » Needs review

I think the status needs to be changed to show there's a patch to review?

jrbeeman’s picture

Ah, yep - sorry about that!

dww’s picture

Status: Needs review » Needs work

Sorry I didn't really see this issue until recently. Thankfully, the patch still applies with offsets. ;) However, I'm not entirely sure what I think, but I spot a few problems:

A) You changed the API to signup_build_signup_data() but didn't update the phpdoc comment for that function.

B) This seems like it's probably a bug to my eyes:

+    if (!isset($signup->uid)) {
+      global $user;
+      $signup->uid = $user->uid;
+    }

Can you add a comment in the code that explains why you want to do that? Seems like if the $signup object we're passed in doesn't have a uid, it's an anonymous signup, and then the current user's uid is irrelevant. What's going on here?

C) This whole function is only for the serialized custom signup data from the crazy theme_signup_user_form() system. It's not clear to me why other modules need to or should be able to alter this data. Can you explain why this is necessary? That other issue you linked to is just dealing with appending stuff so it shows up in the same places. I'm not sure an alter hook is the right solution to that problem (maybe it is, I haven't thought much about it). Regardless, I'd honestly rather spend effort on #29568: Flexible number and type of fields and a saner solution to this custom data problem in general, before adding something like this which is specific to (and perpetuates) the existing insanity.

Thoughts?

Thanks,
-Derek

dww’s picture

Note, part of this problem is now solved by #51226-22: allow users to edit their own signup, since the user's own signup data would be displayed back via the signup_edit_form. So if other modules alter the signup form correctly (sadly, it had to be two different forms, it was just too crazy to reuse), they'll get their other fields displayed "for free".

However, there's still the (smaller) problem of displaying the signup details via the %user_signup_info token in signup emails, and in the administrative table at node/N/signups/admin. I guess for node/N/signups/admin you can use a view and add other entire columns to the table if you introduce other fields, so maybe that's not so bad. And I suppose it's not really *that* big a deal if the %user_signup_info token only contains the fields from theme_signup_user_form() and not every possible field...

So, we might not need to do this when #51226 lands...

dww’s picture

Status: Needs work » Closed (won't fix)