Entity API provides a mechanism for viewing entities and an admin UI.

If both of these are in play, it would be nice if this support contextual links. Patch coming :)

Comments

joachim’s picture

Status: Active » Needs review
StatusFileSize
new2.12 KB
fago’s picture

Status: Needs review » Needs work

Sounds good.

+++ b/entity.tpl.php
@@ -31,6 +31,7 @@
+    <?php print render($title_prefix); ?>

We need to document both variables at the template's docblock.

+++ b/includes/entity.controller.inc
@@ -500,6 +500,20 @@ class EntityAPIController extends DrupalDefaultEntityController implements Entit
+      // If $page is TRUE, the title won't be output and contextual links can't
+      // be output.

I think the point is more we don't need the contextual links if page == is TRUE, as usually we've tabs then.

joachim’s picture

Status: Needs work » Needs review
StatusFileSize
new2.73 KB

New patch with those included.

fago’s picture

Status: Needs review » Postponed (maintainer needs more info)
StatusFileSize
new2.7 KB

Thanks, I've tried to improve the inline comment a bit as well. See updated patch.
However, I realized there might be a problem: What if the contextual links module is disabled, get those variables set then? Maybe we have to ensure they are set in the template preprocessor?

joachim’s picture

Core modules such as block just add the contextual stuff regardless, eg in _block_get_renderable_array().

AFAICT the title prefix/suffix variables are general purpose arrays for anything to add to. They seem to get added as defaults in _template_preprocess_default_variables().

fago’s picture

Status: Postponed (maintainer needs more info) » Needs review

I see, thanks for clarifying.

fago’s picture

Status: Needs review » Needs work
StatusFileSize
new1.11 KB

hm, I tried to get this going with profile2 but was not successful.

The problem is that this only works if you use an admin ui for your main entity type, but for that purpose we provide no admin ui (yet). In the case of profile2, this UI is built manually. So maybe we can support those manual UIs too by making use of the entity_uri() function instead? I'm not sure what exactly the contextual module expects here though.

Also attached the template fix patch for profiles which is necessary for testing.

joachim’s picture

Doesn't entity_uri() provides the path for viewing an entity, rather than editing it? We could assume, I suppose, that you can edit an entity at $uri/edit...

> I'm not sure what exactly the contextual module expects here though.

I'm not 100% sure myself, this is the first time I've got it working!!!

fmizzell’s picture

It would be great if the patch that adds the printing of title perfix/suffix was commited, that way if other modules implement the support for contextual links, entities displayed with the default view will show the contextual links

BarisW’s picture

Agree with #9, yes please! Please commit, and alter later if needed.

BarisW’s picture

Status: Needs work » Reviewed & tested by the community

I've tested #7 and this works perfectly. I see the links to edit the entity AND even the Manage display link :) Wow!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, profile2_contextual_tpl_fix.patch, failed testing.

fubhy’s picture

+++ b/includes/entity.controller.incundefined
@@ -500,6 +500,19 @@ class EntityAPIController extends DrupalDefaultEntityController implements Entit
+        $key = isset($entity->{$this->idKey}) ? $entity->{$this->idKey} : NULL;

I am not 100% sure about this line. Shouldn't that go into the if() statement above? If the entity does not have a id key what are we going to show in the contextual links? What would be the use case?

+++ b/includes/entity.controller.incundefined
@@ -500,6 +500,19 @@ class EntityAPIController extends DrupalDefaultEntityController implements Entit
+            $this->entityInfo['admin ui']['path'] . '/manage',

So I am a little bit reserved about this one. This basically ties the availability of the contextual links to the UI Controller. For me this is fine (I use it most of the time anyways). However, I don't quite agree with the 'manage' path suffix that it uses for all 'operational' tasks. I am not 100% sure why we actually have that. This means that, everytime someone overrides hook_menu() in the UI Controller to provide custom pathes (without the 'manage' -- I do that all the time) that person would also have to override this method to adjust the path given here. So I would propose either of this: a) Remove the 'manage' entirely (really, I don't know why we even have that) or b) make it a second [optional] property in the 'admin ui' array 'manage path'). This would happen in a separate issue, obviously.

+++ b/includes/entity.controller.incundefined
@@ -500,6 +500,19 @@ class EntityAPIController extends DrupalDefaultEntityController implements Entit
+          'fragment' => array(

I am usually using the module/entity type for the array key... What do you think about that?

---------

@joachim is right with the title prefix and suffix though, they should always be part of the template. So far I've always done that manually by overriding the entity template for my custom entities (as well as overriding the ->view() method for exact same reasons).

So I definitely +1 this issue apart from my afforementioned concerns. I am doing the exact same thing for my custom implementations.

damienmckenna’s picture

Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new3.2 KB

This is the patch from #4 rerolled due to the tpl file being moved to a subdirectory.

Status: Needs review » Needs work

The last submitted patch, 14: entity-n1598652-13.patch, failed testing.

damienmckenna’s picture

Status: Needs work » Needs review
StatusFileSize
new2.72 KB

Doh! Sorry about that.

danepowell’s picture

Status: Needs review » Reviewed & tested by the community

#16 has been working well for a year now.

zmove’s picture

+1 for a commit. contextual links are a great idea, but underexploited by most modules that doesn't implement it and finally, it's useless because it apply only on nodes/blocks/views...

kristiaanvandeneynde’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/includes/entity.controller.inc
@@ -677,6 +677,19 @@ class EntityAPIController extends DrupalDefaultEntityController implements Entit
+        $build['#contextual_links'] = array(
+          'fragment' => array(
+            $this->entityInfo['admin ui']['path'] . '/manage',
+            array($key),
+          ),

The key 'fragment' should be the implementing module's name. See contextual_pre_render_links()

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
StatusFileSize
new4.68 KB

Try this one: should work all the same, but follow the contextual links' module rules.

kristiaanvandeneynde’s picture

StatusFileSize
new3.2 KB

My bad, another patch snuck in my git format-patch. Correct one attached.

donquixote’s picture

I tried this with profile2.
Unfortunately, ['admin ui']['path'] is only registered for 'profile2_type' entity type, but not for 'profile2' itself.
Is this a common pattern we should address?

EDIT: Sorry, I should read before I post. See comment #7 by fago.

donquixote’s picture

I tried to research how other modules do it, but did not really find a lot of "'admin ui' => " with grep.
Maybe this needs to be explained more?

donquixote’s picture

I think I get it now :)
profile2 has no paths of its own. It uses the user/% paths instead. To edit a user's profile, you visit the user edit page, or a sub-page of it. The url argument is the uid, not the profile2 id.

I would like to see contextual links on profile2 for "Manage display". To manage the display of the profile2 type, not the user.
The two options for this are:

  1. Add additional tabs (local actions) on user/%, that link to the profile2 display settings.
    This requires more than one local action, because the user could have multiple profiles.
    The profile2 "Manage display" links would then be part of the user entity contextual links.
  2. Register dedicated menu router items in profile2/%/* with hook_menu(), with no real pages but only contextual links.

Either of this would be the job of profile2.
So I think profile2 is a bad example for a generic solution in entity API.

donquixote’s picture

From the patch in #21:

+++ b/includes/entity.controller.inc
@@ -672,6 +672,19 @@ class EntityAPIController extends DrupalDefaultEntityController implements Entit
+      if (!$page && isset($this->entityInfo['admin ui']['path'])) {
+        $key = isset($entity->{$this->idKey}) ? $entity->{$this->idKey} : NULL;
+        $build['#contextual_links'] = array(
+          $this->entityInfo['module'] => array(
+            $this->entityInfo['admin ui']['path'] . '/manage',
+            array($key),
+          ),
+        );

I think this is wrong.
If $entity is a regular entity (node, profile2, user, commerce_product, etc), then $key will contain the etid, typically numeric.
But any path with */manage/* usually does not take the etid as an argument.
contextual_pre_render_links() would try to append the etid to */manage/*, so we end up with something like */manage/123. This is wrong most of the time.

Any positive effect that this patch might have is likely a side effect.
By putting anything non-empty into #contextual_links, we trigger hook_contextual_links_view_alter(). This allows display suite and other modules to add their own contextual links.

What we could do:


  /**
   * Implements EntityAPIControllerInterface.
   */
  public function view($entities, $view_mode = 'full', $langcode = NULL, $page = NULL) {
    [..]
    foreach ($entities as $etid => $entity) {
      [..]
      // Add in contextual links, but only if we are not at the entity's page as
      // tabs are usually available there.
      if (!$page) {
        $contextual_links_info = $this->entityBuildContextualLinksInfo($entity);
        if (array() !== $contextual_links_info) {
          $build['#contextual_links']['entity-' . $this->entityType] = $contextual_links_info;
        }
      }
      [..]
    }
    return $view;
  }

  /**
   * @param object $entity
   *
   * @return array
   */
  private function entityBuildContextualLinksInfo($entity) {
    if (NULL !== $entity_uri = entity_uri($this->entityType, $entity)) {
      if (preg_match('@^(.*)/(\d+($|/.+$))@', $entity_uri['path'], $m)) {
        list(, $parent_path, $args_str) = $m;
        $args = explode('/', $args_str);
        return array($parent_path, $args);
      }
    }
    return array();
  }

This even works for profile2, where the url argument is different from the etid.
It will not actually add any user/%/edit links, because these all have 'context' === 1 in the menu_router table. So menu_contextual_links() will not add them. See the db query in menu_contextual_links().
But other modules will get the chance to add their own things.

I think $module does not really need to be a module. So it is good enough to use 'entity-' . $entity_type as a key.

An alternative would be to add this stuff in hook_contextual_links_view_alter().

chris matthews’s picture

The 3 year old patch in #21 applied cleanly to the latest entity 7.x-1.x-dev, but the comments on this thread are mixed as to whether or not this issue is ready for RTBC status.