User account form not consistent with the API [#2460949]

Problem/Motivation

The module is adding a roles_change new form element to the user_account_form(). By doing so, it breaks all other validations that other modules are trying to do against the official $form['account']['roles'] form element. For example an additional validation callback should check $form_state['values']['roles_change'] instead of $form_state['values']['roles'] which a 3rd party module would expect.

Moreover, even if a module tries to integrate with Role Delegation by applying validations against $form_state['values']['roles_change'] he might fail because even this is not consistent. When a user has administer permissions acces, he will get the original $form_state['values']['roles'] and no $form_state['values']['roles_change'].

Proposed resolution

Make the form consistent as keys with the original user_account_form(). Use roles key for roles in form.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Comment	File	Size	Author
#2	user_account_form_not-2460949-2.patch	4.72 KB	claudiu.cristea
#2	7.x-1.x: PHP 5.3 & MySQL 5.5, D7 18 pass
#2	interdiff.txt	893 bytes	claudiu.cristea
#1	user_account_form_not-2460949-1.patch	4.59 KB	claudiu.cristea
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

claudiu.cristea

Romanian

Arad 🇷🇴

CreditAttribution: claudiu.cristea at Webikon commented 27 March 2015 at 16:58

Status:

Active

» Needs review

File	Size
user_account_form_not-2460949-1.patch	4.59 KB

Patch. Note that a test fail but fails also without the patch so the failure has not been introduced now.

Comment #2

claudiu.cristea

Romanian

Arad 🇷🇴

CreditAttribution: claudiu.cristea at Webikon commented 30 March 2015 at 15:25

File	Size
interdiff.txt	893 bytes
user_account_form_not-2460949-2.patch	4.72 KB
7.x-1.x: PHP 5.3 & MySQL 5.5, D7 18 pass

1 file was hidden/shown/deleted

File	Size
user_account_form_not-2460949-1.patch	4.59 KB

Fixed the case when the form is for new an account.

Comment #3

skwashd CreditAttribution: skwashd at Dave Hall Consulting for Pfizer, Inc. commented 26 July 2015 at 16:57

Overall the code looks sane to me. I haven't tested it locally to confirm that it works. One thing I noticed was this:

+++ b/role_delegation.module
@@ -92,13 +92,33 @@ function _role_delegation_add_roles_to_form(&$form, $account) {
+function role_delegation_user_account_submit($form, &$form_state) {
+  $account = !empty($form_state['user']) ? $form_state['user'] : (empty($form_state['values']['account']) ? NULL : $form_state['values']['account']);
+  $existing_roles = $account ? array_keys($account->roles) : array(DRUPAL_AUTHENTICATED_RID);
+  $form_state['values']['roles'] += drupal_map_assoc($existing_roles);

The multiple ternary operators here make the code difficult to follow. Have you considered doing something like this?:

$account = NULL;
if (!empty($form_state['user'])) {
  $account = $form_state['user'];
}
elseif (!empty($form_state['values']['account'])) {
  $account = $form_state['values']['account'];
}

$existing_roles array(DRUPAL_AUTHENTICATED_RID);
if ($account) {
  $existing_roles =  array_keys($account->roles);
}