| Project: | U Create |
| Version: | 6.x-1.0-beta4 |
| Component: | ucreate.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
I have been using this module as a part of Open Atrium, but I have a custom authentication module which modifies the user registration form, as well as requiring additional user validation. Unfortunately, the way that UCreate currently works, the user validation and custom form additions are not used.
The user_register form can't be used directly, because it checks for the "administer users" permission internally. However, it could be used if we built the form by impersonating user 1.
function ucreate_user_form()
{
global $user;
// Save the user
$saved_user = $user;
// Make sure we don't give the user admin permissions if this fails
session_save_session(FALSE);
// Load user 1 and get the user_register form
$user = user_load(array('uid' => 1));
$output = drupal_get_form('user_register');
// Restore the user and return the form
$user = $saved_user;
session_save_session(TRUE);
return $output;
}That is a slightly simplified version of the function I am testing now. The validation and submit functions also need to use impersonation, which I accomplished by adding start and end handlers for both of those. Doing it this way, we get automatic integration with profile fields, custom user modules, and the ucreate_og module becomes unnecessary because OG adds the group checkboxes to the user_register form itself.
We can use hook_form_user_register_alter to remove fields we don't want displayed, and add back in the logic which determines if the user can set roles or not. Maybe move things like "notify user of new account" to the admin ucreate form.
Comments
#1
I was wrong about needing to add validation and submit handlers - they are not not necessary because those are called from drupal_get_form(). Here is a working patch that uses the core user_register form, allowing integration with all modules which modify the user registration process. This simplifies the ucreate module somewhat, because it is no longer necessary to create the form, validate the fields, or send confirmation emails. The core user_register routine takes care of all that.
I didn't test this patch extensively yet. It's build backwards from my own customized version of this module which includes a few other changes that don't make sense in this patch.
The key changes are the three functions:
ucreate_user_form(): This function is modified to get the user_register() form.
function ucreate_user_form() {ucreate_impersonate('start');
$output = drupal_get_form('user_register');
ucreate_impersonate('end');
return $output;
}
ucreate_impersonate(): Impersonate user 1, making sure that the session is not saved. (No elevating the current user to admin unintentionally!) We do this by changing the UID instead of doing a user_load for user 1. This is for two reasons: we don't need the overhead of a user_load call, and we only want the permisisons of user 1, we still want to have the account info of the registered user available.
function ucreate_impersonate($action='check'){static $saved_user;
global $user;
if($action == 'start'){
if(!isset($saved_user)){
session_save_session(FALSE);
$saved_user = clone $user;
$user->uid = 1;
}
}else if ($action == 'end'){
if(isset($saved_user)){
$user = clone $saved_user;
unset($saved_user);
session_save_session(TRUE);
}
}else{
return isset($saved_user) ? $saved_user : false;
}
}
ucreate_form_user_register_alter(): Alter the user_register form to set the default roles, and to hide the "send notification email" and "user status" options.
function ucreate_form_user_register_alter(&$form, &$form_state)
{
// Will return false if no user account has been saved, meaning we know that
// the code within this block will only run if the form is being displayed
// on the ucreate page with an impersonated user 1.
if($account = ucreate_impersonate()){
// If the user (the real user, not the one we are impersonating) has
// permission to assign roles, let them do so. Otherwise set the default
// roles as a hidden field.
if (user_access('assign user roles',$account) || user_access('administer users',$account)){
// Set defaults.
$form['account']['roles']['#default_value'] = variable_get('ucreate_default_roles', array());
// Remove disabled options - they are not needed here
foreach($form['account']['roles'] as $k=>$v){
if(is_numeric($k) && $v['#disabled']){
unset($form['account']['roles'][$k]);
}
}
}else{
$form['account']['roles'] = array(
'#type' => 'value',
'#value' => drupal_map_assoc(variable_get('ucreate_default_roles', array()))
);
}
// Remove the "notify user" and "status" options.
$form['account']['notify']['#type'] = 'value';
$form['account']['notify']['#value'] = 1;
$form['account']['status']['#type'] = 'value';
$form['account']['status']['#value'] = 1;
}
}
#2
Updated the patch to fix a typo.
#3
Always remember to test after clearing caches. Fixed the menu callback.
#4
Clearly I am new at this.
#5
#6
Not seeing a lot of interest here, and that may be because this patch is too aggressive and changes the entire user creation process for the module. I hope I am not stepping on any toes here, it just seemed that by going in this direction, all of the issues related to profile fields and integration with other modules would be handled automatically. But perhaps this approach is best fleshed out in a separate module.
My plans going forward are to allow the administrator to specify which fields from the registration form to include on the ucreate form, as well as provide default values for the fields. This could be done by generalizing the way that default roles are currently handled. In addition, a separate option to generate a password automatically instead of having the ucreate user specify it.
One thing that gets lost with this approach is adding a custom message to the email notification, but that could be addressed with hook_mail_alter(). Personally, I prefer that all account notification emails use the same template.
#7
I'm interested in this. Patch in #5 applied cleanly, but got a fatal error while accessing user/add form as a regular authenticated user.
Fatal error: Cannot unset string offsets in [...]/includes/form.inc on line 496
Going to dig in on this to see if i can fix it....
UPDATE: Always remember to read the entire thread thoroughly before testing. (The cache clear fixed the fatal error :)
#8
So some feedback. First of all, great effort here! I was able to view a relatively pretty user registration form, complete with core profile fields, fieldsets (categories), etc... however I did notice a few things:
1) There was no status message in the UI upon creation.
2) The user was activated automatically even when Drupal was configured to require admin activation (ie this solution still doesn't fix #917766: Respect core user activation settings.).
3) On admin/user/user/list, the user's Last Access was set to the same date as the 'Member for' value, whereas before applying this patch, the Last Access value says 'Never' by default (until the user actually physically logs in).
4) Re ^, The core user settings configuration 'Require e-mail verification when a visitor creates an account' is ignored.
5) The confirmation email that is sent is the Administrator welcome email (ie Welcome, new user created by administrator)
I believe that this module should follow core drupal functionality by adding an additional customizable email message to the core user settings page, to accompany the existing e-mail settings, something of the following sort:
* Welcome, new user created by administrator
* Welcome, new user created by site member.
* Welcome, no approval required
* Welcome, awaiting administrator approval
* Password recovery email
* Account activation email
* Account blocked email
* Account deleted email
Perhaps food for a separate ticket... but, since this patch varies from the existing module functionality so much already, adding this functionality in this patch would really, really, rock.
#9
Good points jrguitar21. Because this patch basically tricks the system into thinking that an administrator submitted the form, the user flow is exactly as if the user was created under admin/user/add. Most of the issues you raise relate to this.
It seems there are two mindsets as to how this module should work. I was going under the assumption that the "create users" permission is essentially delegating the admin user creation functionality. As such, it would be granted to trusted users, and could safely bypass activation, email verification, etc, just as is done for admin-created users.
1) There was no status message in the UI upon creation.
I am not sure why the status message, which gets set in the core submit handler, is not displayed. Perhaps because of the swapping around users? I will look into it.
2) The user was activated automatically even when Drupal was configured to require admin activation (ie this solution still doesn't fix #917766: Respect core user activation settings.).
This would be relatively easy to implement by adding an extra submit handler to set the user status based on the value of that setting.
3) On admin/user/user/list, the user's Last Access was set to the same date as the 'Member for' value, whereas before applying this patch, the Last Access value says 'Never' by default (until the user actually physically logs in).
There is a comment in the core user_save function where this is set, explaining: Consider users edited by an administrator as logged in, if they haven't already, so anonymous users can view the profile (if allowed). I guess this is to prevent users who have never logged in from putting malicious content on their profile and having it publicly visible before they are approved? As with the previous issue, this would not be difficult to reset in a submit handler.
4) Re ^, The core user settings configuration 'Require e-mail verification when a visitor creates an account' is ignored.
I would be slightly concerned with this setting in the context of this module, because I think people would be concerned that someone was attempting to use their credentials or hack their accounts if they got a confirmation email they were not expecting. A new email would be required, explaining that someone has suggested they join.
5) The confirmation email that is sent is the Administrator welcome email (ie Welcome, new user created by administrator)
This is the trickiest issue, because the choice of email is made in the core user_register_submit function. The only thing I can think of off the top of my head is to use the impersonation function not only to change the user, but to temporarily swap the value of the admin email variable. That's kind of risky, though, because the change would not be reverted if something went wrong or PHP crashed.
Of course, it may simply be that my approach of using impersonation introduces too many issues along these lines to make it worthwhile. The advantage is that it keeps the registration process in the core user_register set of forms, so that hooks and alters work as expected. Another possibility would be to impersonate the anonymous user when building the form. I will experiment with that solution and see if it gets closer to how people expect the module to operate.
#10
Though I should note: If impersonating the anonymous user, security checks would not allow roles to be set.
#11
FWIW I'm using your patch now in a production site. I didn't have the use case for needing to automatically set roles and so I thought about impersonating user 0 and actually tried it without any noticeable errors (though I didn't spend much time on it, and reverted back to your patch, before launching).
I've made the modification for the activation status to be pulled from the core settings... here's a modified patch that also contains some code cleanup.
#12
One other thing I've come across is that its impossible to get rules to trigger the event for user creation (which, if it worked correctly, could easily solve the issue 1 in comment #8) Admittedly, though, I don't think even the regular UCreate module without the patch even did this, so technically speaking, nothing gained, nothing lost.
Also, if rules triggered correctly, then someone who needed the role could probably configure a way to turn a cck selection into a role via a Rule (to handle the caveat of impersonating user 0).