Clean up field_collection_item_access() and the calls to it.

Patch. I've only casually tested, so needs a closer look.

Also, if anyone can explain what FieldCollectionItemEntity with no comma after it is doing in the args... looks almost like an accidental paste, but has been in dev releases for a little while.

#1: field_collection-item_access-1688114-1.patch queued for re-testing.

Don't know why this is failing, applies fine and seems to work for me.

Re-rolled for the latest dev.

You know, just editing.

Ah, ok, I learned how to use simpletest. Behold the green!

The main problem I see with the access check as it stands is it's being called with a ternary. You should be able to check field access without doing any logic in the function call.

The way I've set it up allows all possible ops for field_access and entity_access, since we're checking both in field_collection_item_access. If you'd prefer to restrict the values to just add, edit, and delete, I'm into it; I'd just like to see the logic being used in the ternary pulled inside of the function.

Can you explain the idea behind a field_access wrapper function?

Actually, in this patch there's no code specifically targeting anything besides add, edit, and delete. If you want to accept only those, we can just edit the comments.

/**
 * Determines whether the given user has access to a field collection.
 *
 * @param $op
 *   The operation being performed. One of 'view', 'edit', 
 *   'add', or 'delete'.
 * @param $item
 *   Optionally a field collection item. If nothing is given, access for all
 *   items is determined.
 * @param $account
 *   The user to check for. Leave it to NULL to check for the global user.
 * @return boolean
 *   Whether access is allowed or not.
 */
function field_collection_item_access($op, FieldCollectionItemEntity $item = NULL, $account = NULL) {
  if (user_access('administer field collections', $account)) {
    return TRUE;
  }
  if (!isset($item)) {
    return FALSE;
  }
  
  // Op coming from $settings is 'add', 'edit', or 'delete'.
  // Field access only takes 'view' or 'edit' operations.
  $field_op = $op == 'view' ? 'view' : 'edit';

  // Entity access takes 'view', 'update', 'create', or 'delete' operations.
  if ($op = 'edit') {
    $entity_op = 'update';
  }
  elseif ($op = 'add') {
    $entity_op = 'create';
  }
  
  // Access is determined by the entity and field containing the reference.
  $field = field_info_field($item->field_name);

  $entity_access = entity_access($entity_op, $item->hostEntityType(), $item->hostEntity(), $account);
  $field_access = field_access($field_op, $field, $item->hostEntityType(), $item->hostEntity(), $account);

  return $entity_access && $field_access;
}

Thinking about targeting ops a little more, using the ternary turns any op argument besides "view" into "edit". It might be better to change this to:

  if ($op = 'add' || 'edit' || 'delete') {
    $field_op = 'edit';
  }

I don't think it really matters; if you use a random string for the op link, entity access will return false (since it transforms the ops specifically, any non "view-add-edit-delete" string gets through). Just bringing it up for the hell of it.

The patch rewrites field_collection_item_access as a big wrapper function for both entity_access and field_access. I'm not sure when there would be a need to check the field access for a field collection field without also checking the entity access for the field collection item, so I don't know if it's a good idea to add a separate function wrap that by itself.

I think we might be trying to say the same things but with different words. If you can tell me what operations field collection_item_access should take as arguments, I'll code it up.

I should read twice and edit once...