Creating a duplicated role triggers SQL error

Confirmed in HEAD.

Patch checks if role name exists in two instances:

1. When adding new role - if role name exists, display error.
2. When editing existing role - if role name exist, but is not the original name, display error

Patch from http://drupal.org/node/58860 also applied to show edit role form if error occurs during role editing.

Status change

Patch was abit broken, saving of edited role with new name was not working. New patch fixes this.

Patch rerolled to latest HEAD and changed 'exist' to 'exists'.

Rerolled from root to latest HEAD.

quick review- shouldn't this new code be in a separate validate function instead?

Actually, it looks like this entire function has an unfortuante mixing of validate and submit code.

Also, I think this query may be faster (more expert comments welcome), since there is an index on the name column:

db_query("SELECT COUNT(*) FROM {role} WHERE name = '%s'", $edit['name']))

rather than the one you use:

db_num_rows(db_query("SELECT rid FROM {role} WHERE name = '%s'", $edit['name']))

oops, should be:

db_result(db_query("SELECT COUNT(*) FROM {role} WHERE name = '%s'", $edit['name'])));

Thanks for taking a look at it.

Yeah, I think using COUNT(*) would be more efficient, but only can be used when adding roles. The rid would need to be returned when editing a role to ensure the same role name could be saved. In that sense, if seperating to a new function, it would be two different functions for the different purposes. IMO it is not necessary as the functions are not used anywhere else.

I also agree that the role adding/deleting workflow could also be improved (eg using _submit and _validate hooks) but that is for another issue.

This patch uses COUNT(*) in the sql query to check for duplicate role names when adding a role as suggested. db_query returns resource regardless the query is empty or not. Hence, db_result was used to fetch the result. Is there a better way to do this?

I think the first part of the patch could be simplified a bit- you have duplicated code there that's unnecessary. something like:

  if (db_result(db_query("SELECT COUNT(*) FROM {role} WHERE name = '%s' AND rid != $id", $edit['name']))) {
     form_set_error('name', t('The role name %name already exists. Please choose another role name.', array('%name' => $edit['name'])));
       
      }
      else {
        db_query("UPDATE {role} SET name = '%s' WHERE rid = %d", $edit['name'], $id);
        drupal_set_message(t('The changes have been saved.'));
        drupal_goto('admin/user/roles');
      }

patch attached for 5.0/HEAD. Some quick testing seems to show that it fixes the bug. Please review.

The function user_admin_role is clearly an ugly relic of pre-4.7 pre-FPAI code. But, as you said, fixing that is an another issue. Obviously this code should be broken out into form, validate, and submit functions.

pwolanin's patch achieved the same results with less code :)

Keeping up with HEAD. When saving an edited role, status message is now 'The role has been renamed.' instead of 'The changes have been saved.'. Another change was passing $id as a separate parameter when checking if the role name exists.

makes sense to me and is apparently tested by the author.

IMO, this kind of validation should happen in the validate functions. If not, it can still go wrong with programatically submittable forms. Not?

@Dries- I agree that this function badly needs to be refactored into _validate, _submit, etc. functions. I'd be happy to take that on as a separate issue (for 5.x/HEAD), but we should quash this actual bug in 4.7.x and 5.x first.

pwolanin raised the issue up in #7 and we agreed that separating it to the submit and validate patch should be tackled in another issue. But since Dries raised the matter up, I thought we could just fix it altogether. Underlying codes are the same as the previous patch, just changes here and there to make it work with the functions. Please review

Yes, probably better to do this real refactoring, though I think the prior patch should be applied to 4.7 since I'm guessing killes won't want to commit such substantial changes when a simpler bug fix will do.

Also, I think in function user_admin_role_submit, you shouldn't call drupal_goto directly, but rather return the path following these instructions from the Forms API guide:

The return value of the _submit function will be the target of a drupal_goto; every form is redirected after a submit. If you return nothing, the form will simply be redirected to itself after a submit. It is polite to use drupal_set_message() to explain to the user that the submission was successful.

Yeah, probably the bugfix would be more appropriate for 4.7.

Oh yeah, missed out the drupal_goto. Thanks, any other issues?

Issue still applies to beta1, rerolling patch.

Committed to CVS HEAD. Thanks guys!

needs to be backported to 4.7, either the initial (minor) fix or the more substantial one that went into 5.x

Backported to 4.7. Patch is based on minor bug fix as in #12. If refactored code is preferred instead, I don't mind backporting it.

a properly doen FAPI form woudl be preferable, I think.

New 4.7 patch. All done in FAPI glory as much as 4.7 allows :)

thanks! Since it is a rather large change, i'd like a second person to test this patch before committing it.

Sure :) We'll set this back to the review queue then.

tested myself: Works, applied.