Disclaimer: this is definitely an obscure bug, I hope this report makes sense. ;)

As you probably know, I'm working on a project (with Earl) that's making heavy use of field_collection (yay!). I've been trying to come to grips with all the field_collection-specific menu items available on the site, since not all users should be able to access all of these menu items directly. I've been studying field_collection_item_access() and I think I see the best way to solve those problems.

However, in testing this stuff and poking around, I noticed something fairly weird about these menu items. For example, on my dev site, field_collection entity #101 is a "field_activity_log" field_collection item attached to uid #28. This user can access this entity directly at:

field-collection/field-activity-log/101

However, it also works at any URL where the 2nd item in the path happens to be a valid field_collection field name on the site. For example, all of these also work:

field-collection/field-messages/101
field-collection/field-employment-history/101
field-collection/.../101

But, if you use a 2nd path element that's not a field_collection field name, you get a 404:
field-collection/field-foo/101

The reason is that you're registering every possible field_collection field name as a separate menu item in field_collection_menu():

  foreach (field_info_fields() as $field) {
    if ($field['type'] == 'field_collection') {
      $path = field_collection_field_get_path($field);
      $count = count(explode('/', $path));

      $items[$path . '/%field_collection_item'] = array(
        'page callback' => 'field_collection_item_page_view',
        'page arguments' => array($count),
        'access callback' => 'field_collection_item_access',
        'access arguments' => array('view', $count),
        'file' => 'field_collection.pages.inc',
      );
      ...

However, the access callback doesn't know anything about the field name. You only pass the operation and the entity ID. Furthermore, the menu item loader function doesn't care about the field name, either:

function field_collection_item_load($item_id, $reset = FALSE) {
  $result = field_collection_item_load_multiple(array($item_id), array(), $reset);
  return $result ? reset($result) : FALSE;
}

Given all of the above, I have no idea why you even bother with the complication of trying to embed the field name in these menu paths. Nothing actually enforces that the field collection entity matches the field names in the paths. It'd probably be simpler in a lot of places if you just went with paths like these:

field-collection/101
field-collection/101/edit
field-collection/101/delete
...

I don't really see what embedding the host entity field name buys you, and how it works now gives you a bunch of additional menu items with duplicate content. Makes it harder to lock this stuff down, adds needless work for the menu router, etc.

Thoughts?

Thanks!
-Derek

p.s. Now that I'm done writing this issue, a better title might be "Embedding host entity field names in field_collection entity menu paths is a waste of effort" or something. ;)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

30equals’s picture

I was having some issues today as wel because of this. For example, the registration module adds some menu items based on a pattern 'entitytype/entityid' - i don't know for certain to what extend other modules rely on this pattern, but it seems to me that i would be a logical path to apply. eg. 'field-collection-item/fieldcollectionid'.

Also, the basepath for the field collections are constructed with the following function

/**
 * Returns the base path to use for field collection items.
 */
function field_collection_field_get_path($field) {
  if (empty($field['settings']['path'])) {
    return 'field-collection/' . strtr($field['field_name'], array('_' => '-'));
  }
  return $field['settings']['path'];
}

But there's no way to set the path yourself right ? (Or maybe i looked over something), so i'm not sure what the use of the function is...?

Anyway, i'm with dww here ;)

30equals’s picture

Status: Active » Needs review
FileSize
3.54 KB

Soooo, i haven't looked into it in detail, but what i'm suggesting is something like what's changed in the attached patch.

Status: Needs review » Needs work

The last submitted patch, field-collection_alter_menu_items-1388156-2.patch, failed testing.

jmuzz’s picture

Title: Menu items to view, edit and delete field_collection entities are too permissive since they don't take field names into account » Remove bundle/field names from view, edit and delete paths for field collection items.

I like this idea but it wouldn't be good to change paths that people may be relying on now for their site functionality.

jmuzz’s picture