I realized that signup_nodeapi() is doing weird things by relying on global $form_values. This won't work at all in D6, and it's a hack in D5, too. Furthermore, the function is a lot more complicated that it needs to be, and duplicates a fair bit of code. It also generates some PHP warnings in some cases from using uninitialized variables. Finally, by relying on a switch statement nested inside another switch, it throws coder_style into a fit: #328725: coder_format: switch in a switch? ;)

CommentFileSizeAuthor
#5 328822_signup_nodeapi_cleanup_fix.5.patch3.52 KBdww
#1 328822_signup_nodeapi_cleanup.1.patch11.5 KBdww

Comments

dww’s picture

dww
we/he/they
commented
Status: Active » Needs review
StatusFileSize
new 328822_signup_nodeapi_cleanup.1.patch11.5 KB

Try this on for size... it addresses all of the stated problems, shrinks the actual code in the module by about 10 lines, and adds 25 lines of PHPdoc comments to clarify the complicated stuff.

dww’s picture

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

In my testing for the past few days, I haven't seen any problems with this, it makes the code cleaner, and it'll make the D6 port easier, so I committed to HEAD and DRUPAL-5--2. Not user visible so there's no CHANGELOG entry about it.

dww’s picture

dww
we/he/they
commented
Category: task » bug
Status: Fixed » Active

I bet this is what caused: #330900: Signup settings lost when a user edits the node without administer signup permission
That's a case I don't think I tested and I'm not surprised I broke it. ;)

dww’s picture

dww
we/he/they
commented

yup, easy bug, in both DRUPAL-5--2 and HEAD. I just don't have a means to upload a patch right now (on a plane). But I'll commit the fix when I can get my laptop online again.

dww’s picture

dww
we/he/they
commented
Status: Active » Needs review
StatusFileSize
new 328822_signup_nodeapi_cleanup_fix.5.patch3.52 KB

The basic fix was a one line change:

-  elseif (variable_get('signup_node_default_state_'. $node->type, 'disabled') == 'enabled_on') {
+  elseif ($op == 'insert' && variable_get('signup_node_default_state_'. $node->type, 'disabled') == 'enabled_on') {

However, while I was testing this more, I realized there was another hunk of code that didn't need to run unless someone with signup admin perms was editing a node (which generated a PHP warning in D6). So, attached patch fixes all. Applies to both HEAD and DRUPAL-5--2. If anyone wants to further test before I commit, that's lovely. ;)

geodaniel’s picture

geodaniel commented

Subscribing to this, will try to test it out today.

geodaniel’s picture

geodaniel commented

This seems to do the job as expected. Editing a signup-enabled node as someone who doesn't have the correct perms doesn't wipe the signup info out any longer.

dww’s picture

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

Committed to HEAD and DRUPAL-5--2. Setting this back to a task per the original issue.

dww’s picture

dww
we/he/they
commented

FYI: This also broke #335190: Disable signup on node leaves signup enabled (now fixed) ;)
I added a "Known bugs" section to both the 6.x-1.0-rc1 and 5.x-2.6 release notes.

Status: Fixed » Closed (fixed)

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