
Regarding the "signup_sign_up_user" function
Currently the function returns a FALSE value if a user is already signed up
It would be great if the $sid value could be returned with a successful signup.
There would only be one line of code to add...
return $sid;
At the bottom of the function, this
$node->signup_total++;
if ($node->signup_close_signup_limit) {
_signup_check_limit($node, 'total');
}
}
else {
drupal_access_denied();
}
}
Would become this
$node->signup_total++;
if ($node->signup_close_signup_limit) {
_signup_check_limit($node, 'total');
}
return $sid;
}
else {
drupal_access_denied();
}
}
I don't believe any current call to the function would be impacted.
Why would this be helpful?
Currently I can call the function from my own module as outlined in the documentatation:
signup_sign_up_user($signup_form);
If I set the functions return value to a variable:
$sid = signup_sign_up_user($signup_form);
I can check the $sid for FALSE to see if the signup was successful, and that is nice.
I have an additional dataset that I need to use that is keyed on the $sid value, so currently I need to do another query to get that $sid value.
If the "signup_sign_up_user" function returned the value it would elimate the additional query and provide a clearer method for determining success.
Then I could call the function such as...
$sid = signup_sign_up_user($signup_form);
if ($sid > 0) {
// success
db_query('INSERT blah blah blah', $sid);
}
And by the way, thanks for this module, it is great!
Comment | File | Size | Author |
---|---|---|---|
#5 | 424338-a.patch | 552 bytes | ezra-g |
#4 | 424338.patch | 2.16 KB | ezra-g |
#2 | signup_sign_up_user.patch | 318 bytes | rjl |
Comments
Comment #1
dwwSure, that's a lovely idea. I only recently introduced the sid to the codebase, so there are plenty of places that should be making more use of it that are not. Feel free to submit a patch for this and it'll go in.
Thanks,
-Derek
Comment #2
rjl commentedAttached is a patch
Just FYI, this is my first patch, so I hope I did it correctly.
Comment #3
dwwYay, thanks. ;) A few things:
A) When you upload a patch you want the maintainer to review, set the status of the issue to "needs review".
B) When creating a patch, use "diff -up" to make it more readable. See http://drupal.org/patch/create for more info.
Setting this to "needs work" for part B. The patch is probably fine, but since it's your first patch, better to teach you how to do it right for the future...
Thanks,
-Derek
Comment #4
ezra-g commentedActually, it looks like this patch needed a little bit more work than simply a re-roll. The attached patch returns the SID whether or not the user (or anonymous) is already signed up for this nid. Also, it changes the $user variable to $account to avoid getting confused with the global $user object. I tested this with an API call for anonymous and authenticated users with and without existing signups for both.
Comment #5
ezra-g commentedOn further thought, perhaps it would be better to only return the sid for a new signup, and return false when a signup already exists. This patch implements this change. @dww: Choose your favorite flavor ;).
Comment #6
dwwWent with a modified version of #5. I like returning FALSE if we didn't actually signup someone. However, #5 had a code style (indentation) bug, and there was no @return PHP doc for this function. Added those, tested a bit (obviously working fine) and committed to HEAD and DRUPAL-6--1. This'll be out in RC4.