Currently FormAPI and the rendering system use the #access property of array elements to control visibility. TRUE is the assumed default behavior, FALSE can be explicitly set, and a FALSE cascades downward. This behavior is enforced in both form.inc's form building code and in drupal_render. The attached patch centralizes element access checking in a single helper function for greater maintainability.
This patch also allows #access to contain a function name in addition to a boolean FALSE. If a function name is specified, it will be called at runtime to determine whether the current user has rights for the current operation. ('edit' is used inside form.inc, and 'view' is used inside drupal_render() -- potentially allowing fields to be blocked from editing in the future but not from being viewed.) If there are objections to this construct, it's easy enough to eliminate the $op parameter and allow it to use a simple callback mechanism -- just seems like a shame not to put the plumbing in for it.
It's a bit rough but also very useful. It would allow us to add access callbacks to renderable elements, caching the data to eliminate unecessary build-time when it's displayed to different users.
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | access_11-D7.patch | 2.64 KB | swentel |
| #14 | access_10-D7.patch | 2.56 KB | swentel |
| #10 | access_10.patch | 2.29 KB | lilou |
| #6 | access.patch | 2.07 KB | chx |
| #5 | access.patch | 2.71 KB | eaton |
Comments
Comment #1
gábor hojtsySeems to be logical to me. It would be nice to get some other form API guru opinion on this before going to RTBC.
Comment #2
eaton commentedFrom a code perspective, it does four things:
1) Abstracts out the access checking to a dedicated function, which eliminates redundant code.
2) if a non-FALSE value is set for the #access property, it checks to see if a function by that name exists. if so, it calls it and passes in the element, expecting a TRUE or FALSE in return.
3) Also allows modules using the _element_access() function to pass in an explicit operation (ie, 'edit' or 'view') for checking.
4) When walking through the form array and building it, ripple down *the same access check* to child elements if they do not have one explicitly set. Currently, we force a 'false' down the chain. Today, it functions the same but in the future, if developers treat the 'view' and 'edit' access checks differently, it gives an opportunity to change the form element to disabled, or some other visual cue, if the current user has 'view' but not 'edit' perms.
Comment #3
mlncn commentedA big plus one on the concept. I only just found this page looking for a way to make a form element viewable but not editable. Can this still make it into Drupal 6, as a security improvement and code optimization?
Comment #4
moshe weitzman commentedlooks reasonable. will the callback function want any more parameters passed to it?
also, this doesn't look right:
call_user_func($element, $op);how about
call_user_func($element['access'], $op);Comment #5
eaton commentedRe-rolled against the latest head. I'm working on CCK at the moment, and something like this would definitely simplify the current work on field-level CRUD permissions considerably. (We'd be able to embed the callbacks directly in a renderable array and cache IT rather than caching on a per-user basis.) It's probably too late for something like this, but I wanted to keep it up to date for when it can be resurrected.
Comment #6
chx commentedI disagree with this logic -- if the function specified in #access does not exist we should deny not allow. Also, I simplified a tad bit the patch.
Comment #7
mlncn commentedThis applies and doesn't break anything! Didn't try anything that relies on the new options, so I guess I can't mark RTBC. I agree with chx's deny if function not found, especially as it will make development mistakes immediately apparent.
benjamin, Agaric Design Collective
Comment #8
birdmanx35 commentedStill applies cleanly a month later; I didn't test this though other than applying it. But it isn't broken.
Comment #9
gábor hojtsyNew features and API changes go to 7.
Comment #10
lilou commentedReroll chx patch #6
Comment #11
moshe weitzman commentedlooks good. needs to use drupal_function_exists() instead of function_exists()
Comment #12
Anonymous (not verified) commentedSo it needs some work.
Comment #13
sunThis is exactly what I was thinking of in #91663: Permission of text format is not checked when editing an entity and instead reset to something a user can use.. However, if access is denied, access callbacks should be able to return alternative contents - so in #91663's use-case, we would want to insert a notice that the user does not have sufficient (input format) permissions to edit a node's body.
Comment #14
swentel commentedreroll and using drupal_function_exists
Comment #16
mlncn commentedYesterday it said it passed– HEAD was broken for a time, so this patch is still good and testable.
Comment #17
swentel commentedChasing HEAD, this might help us fixing #91663: Permission of text format is not checked when editing an entity and instead reset to something a user can use.
Comment #18
webchickLet's flesh this out a bit more. Remember that future readers of this code will not have the context of this issue. How is this determination made?
Comment #19
pasqualleComment #20
catchThis is generically handled by #post_render_cache now, and there are other issues like #2351015: URL generation does not bubble cache contexts working towards making form tokens and etc. use that, which is similar enough to #access and blocks render caching of any form.