Posted by agentrickard on January 17, 2010 at 9:10pm
7 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | user.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | tsphethean |
| Status: | needs work |
| Issue tags: | forms api, needs backport to D7, Novice |
Issue Summary
Form _submit and _validate callbacks should not call user_access() checks. Doing so interferes with form processing and can break hook_form_alter().
Looks like the form itself needs to be rewritten to pass appropriate values to trigger certain conditions.
Comments
#1
Moving
#2
First solve and discuss in this issue http://drupal.org/node/687586 and then come back here to apply the same solution.
#3
#4
subscribing
#5
The only place I can find that calls user_cancel_confirm_form_submit() in 8.x is user_multiple_cancel_confirm_submit(). I can't find anything that calls user_multiple_cancel_confirm_submit(). Is this some feature of drupal/php that I'm unaware of where some behind the behind the scenes mojo in user_cancel_confirm_form() calls user_cancel_confirm_form_submit() (or the equivalent for the multiple cancel)? Or are these both just functions that have been deprecated but are still used somewhere I can't find?
#6
@emclaughlin module_formID_submit() is called dynamically when the form generated by module_formID() is submitted successfully.
That's the default submit callback (others can be defined explicitly in
$form['#submit']).#7
I have reviewed the approach in 687586 and applied the same approach here. Let me know if anything additional is required as I see that despite a successful patch the 687586 issue was re-opened.
#8
#9
The last submitted patch, user-cancel_submit_remove-687588-7.patch, failed testing.
#10
Revised patch attached - now checking for null response from user_access call
#11
The last submitted patch, user-cancel_submit_remove-687588-10.patch, failed testing.
#12
A little confused by why this is failing - I'm not getting the same notice when I test locally and stepping through adding debug output at each step has been successful.
I've added an extra isset check in this patch - hopefully that is ok but let me know if I'm missing something!
#13
Looks good.
#14
Looks good, doesn't add new features and don't break anything, so...
#15
Wait!
1) Why use numbers here:
+ '#value' => ($admin_access ? 1 : 0),And then we use a boolean for the same:
+ $admin_access = FALSE;Should be more consistent use TRUE/FALSE in both places.
2) Instead of use this:
+ $admin_access = FALSE;+
+ if (isset($form_state['values']['administer_users'])) {
+ $admin_access = $form_state['values']['administer_users'];
+ }
I think it's better the compact form:
$admin_access = isset($form_state['values']['administer_users']) ? $form_state['values']['administer_users'] : FALSE;#16
I agree with TRUE/FALSE but Drupal coding standards dissuade the use of ternary expressions.
Please don't recommend changes that violate standards.
#17
For TRUE/FALSE and brevity, we can just use:
<?php'#value' => user_access('administer users'),
?>
In the IF, probably simpler to do this:
<?php$admin_access = !empty($form_state['values']['administer users'];
?>
Clear, concise and avoids the ternary operator.
#18
Thanks for the feedback and suggestions. Revised patch attached.
#19
Now there are redundant lines:
<?php-
+
+ $admin_access = !empty($form_state['values']['administer users']);
+
+ if ($admin_access) {
+ $admin_access = $form_state['values']['administer_users'];
+ }
?>
The first !empty() assumes that the 'administer users' form element is set. If not set and TRUE, admin riggers will not fire.
#20
Good point - revised patch attached.
#21
The last submitted patch, user-cancel_submit_remove-687588-20.patch, failed testing.
#22
Gah, sorry - typo on the form state values. Fixed working patch attached.
#23
Gah, sorry - typo on the form state values. Fixed working patch attached.
#24
A while since this has moved so re-rolled against latest D8 build, apply cleanly so seeing what testbot makes of it now.
#25
Sorry, not sure how I thought that last patch would apply... re-rolled now and re-queueing for testing.
#26
#25: user-cancel_submit_remove-687588-25.patch queued for re-testing.
Seeing whether testbot still likes this one
#27
The last submitted patch, user-cancel_submit_remove-687588-25.patch, failed testing.