This is something that I added recently to CCK. Please, see #514452: Add new argument $node to content_access() to enhance the context for hook_field_access()

This is necessary in order to provide field permissions based on context, for example to check for 'edit own field' we need to know the owner of the object.

CommentFileSizeAuthor
#3 field_access_context-597832-3-D7.patch3.48 KBmarkus_petrux

Comments

yched’s picture

yched commented

Not where I can submit a patch myself, but +1 on principle.

moshe weitzman’s picture

moshe weitzman commented
Category: task » bug
Priority: Normal » Critical
Issue tags: +API clean-up

Has to happen before release in order to give upgrade path for many sites. Marking as critical

markus_petrux’s picture

markus_petrux commented
Status: Active » Needs review
StatusFileSize
new field_access_context-597832-3-D7.patch3.48 KB

Patch attached.

In D6, we're using the $node. In D7, we're using $obj_type and $object. Both methods should provide the same feature.

bjaspan’s picture

bjaspan commented

Why is $object optional but $obj_type is not? For that matter, when will you call field_access() without an object available, e.g. why should it be optional at all?

markus_petrux’s picture

markus_petrux commented

In Views for D6, content_access() is used as an access callback for fields exposed to Views (by CCK). This is used by Views to see if the field can be included or it should be discarded while building the query. Here, we do not have a node, but we know this use of content_access() involves a node. If it was Fields in D7, we could tell Views the $obj_type argument (the nature of the field exposed to views is known), but not the $object argument.

Any other place where content_access() is invoked includes the node, so it could include $obj_type and $object. But in Views, this is not possible because the access callback of the field does not have nodes yet.

I tried to describe this in the CCK issue, but also in the hook_field_access() implementation of CCK Private Fields (link). I guess this situation will still remain in D7.

markus_petrux’s picture

markus_petrux commented

Related issues:

- #501404: Field Permissions in Core
- #598924: Port of Field Permissions to D7

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ modules/field/field.api.php	2009-10-07 18:16:16.000000000 +0200
@@ -1429,11 +1429,15 @@ function hook_field_build_modes($obj_typ
-function hook_field_access($op, $field, $account) {
+function hook_field_access($op, $field, $obj_type, $object, $account) {

1) If it's optional, then it should use a default of NULL in the signature, just like in field_access() itself.

2) Is this the natural order of arguments? All other *_access() functions we have take $account right after the primary object/condition to check.

That said, and taking #593522: Upgrade drupal_alter() into account, wouldn't it make sense to pass a $context (which here would contain $obj_type and $object) ?

This review is powered by Dreditor.

bjaspan’s picture

bjaspan commented

re #7:

1. Optional arguments are syntactic sugar, only useful to external API callers. field_access() may have optional arguments, but hook_field_access() will always be invoked with all of its arguments (though $object may be NULL). Thus, the hook argument should not be optional. This is consistent with other Field API hooks.

2. I had the same question, except I was wondering if $obj_type and $object should be first (before $field even) to be consistent with other Field API functions. I decided since I wasn't sure what was right it wasn't a big enough deal to bikeshed. :-)

Not sure what I think about passing $context. I note that field_attach_load() accepts $options.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I just looked into field.module and field.attach.inc once again, and indeed, almost all field functions take $obj_type, $object as first arguments. So we should definitely do as well here for DX++

Let's skip that $context idea... probably only makes sense for drupal_alter() and related stuff.

markus_petrux’s picture

markus_petrux commented

Food for thought: note how this is used as an access callback argument is Views fields (D6 snippet of CCK):

          'access callback' => 'content_access',
          'access arguments' => array('view', $field),

In D7, in the contrib module that implements Views integration, it could do:

          'access callback' => 'content_access',
          'access arguments' => array('view', $field, 'node'),

That's the reason why I opted for ordering the arguments that way.

If we use $context, it could look like this:

          'access callback' => 'content_access',
          'access arguments' => array('view', $field, array('obj_type' => 'node')),

Views is the only context where $object would be optional. The rest of the places where content_access() is invoked by CCK in D6 comes with the $node, so I think this is equal for D7.

Then, if we use $context, we could use it also to pass the $account?

Thoughts? Do we go for $context with members for 'obj_type', 'object', 'account'? Is the patch ok as it is?

[EDIT] Oops! it seems I was writing my follow up while sun already replied.

markus_petrux’s picture

markus_petrux commented

BTW, note that $object is optional, so it should be after $field.

It think the correct prototype would be field_access($op, $field, $obj_type, $object = NULL, $account = NULL), which is what the patch in #3 uses.

bjaspan’s picture

bjaspan commented
Status: Needs review » Reviewed & tested by the community

This discussion has resolved my concerns about the argument order, and answered the question about why $obj_type is mandatory but $object is not.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Looks good. Useful and harmless to pass around some extra information. Committed to CVS HEAD!

Status: Fixed » Closed (fixed)
Issue tags: -API clean-up

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +API clean-up

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.