Currently, it is not possible (or at least not easy) for a module to alter the way contextual links are built. This is because they are built very early in the page rendering process, and their default theme, library, etc is added in a way that is not directly accessible.

In earlier work on the initial contextual links patch at #473268: D7UX: Put edit links on everything we had them as a standard element with a pre-render function. This patch restores that functionality, which allows other modules to alter any aspect of them via hook_element_info_alter(), including the function that is used to build them.

Notes:

  • This slightly changes the input and purpose of system_build_contextual_links(). We should perhaps make it so that function behaves closer to the way it did before - i.e., receives the entire page element which has contextual links attached to it, rather than just the contextual links themselves (even though core only uses the latter).
  • This also slightly changes the meaning of the 'contextual-links-region' CSS class - it is now added to regions of the page even if the current user does not have access to perform any of the particular actions that the contextual links point to. I don't think this is a problem at all, though.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

Status: Needs review » Needs work

I was really surprised when contextual links got in without following the proper page render system. Thanks for restoring that, David. This looks like nice.

In system_preprocess, could we just set the variable such as: $variables['contextual_links'] = $element. This is RTBC after that.

sun’s picture

+++ modules/system/system.module	9 Nov 2009 02:42:42 -0000
@@ -3533,8 +3543,6 @@ function theme_system_settings_form($var
- *
- * @see system_build_contextual_links()

We want to keep this @see, because the functions run together.

+++ modules/system/system.module	9 Nov 2009 02:42:42 -0000
@@ -308,6 +308,16 @@ function system_element_info() {
+  $types['contextual_links'] = array(
...
+    '#contextual_links' => array(),

The #contextual_links property is not required and not used for the #type and can be removed.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
2.33 KB

OK, fixed the issues in #2.

Regarding the $variables['contextual_links'] = $element suggestion in #1, I think it's not that simple because $element in this case is already an entire structure representing the item that the links are attached to (for example, it might be a fully built node), so it already has its own #theme, #attached, etc.

I tried a different solution for that issue in the attached patch - same as before, but now passing the full $element in a special '#parent_element' key. Not sure if that's a good idea or not. It definitely meets the goal of getting the info to any pre-render functions that might need it, but these parent elements can be pretty big, so we'd now be passing around a pretty large array all over the place in $variables['contextual_links'] until it gets rendered...

sun’s picture

Status: Needs review » Needs work
+++ modules/system/system.module	10 Nov 2009 21:10:56 -0000
@@ -308,6 +308,15 @@ function system_element_info() {
+  $types['contextual_links'] = array(
...
+    '#links' => array(),
@@ -3564,10 +3573,15 @@ function system_preprocess(&$variables, 
+    $variables['contextual_links'] = array(
...
+      '#contextual_links' => $element['#contextual_links'],

The properties are a bit weird now.

+++ modules/system/system.module	10 Nov 2009 21:10:56 -0000
@@ -3564,10 +3573,15 @@ function system_preprocess(&$variables, 
-    if (!empty($variables['contextual_links'])) {
-      $variables['classes_array'][] = 'contextual-links-region';
-    }
...
+    // Label this element as having contextual links associated with it.
+    $variables['classes_array'][] = 'contextual-links-region';

As can be seen from the old code, we only want to apply the cl-region class if there are any contextual links rendered.

+++ modules/system/system.module	10 Nov 2009 21:10:56 -0000
@@ -3564,10 +3573,15 @@ function system_preprocess(&$variables, 
+      '#parent_element' => $element,

So what is this for?

+++ modules/system/system.module	10 Nov 2009 21:10:56 -0000
@@ -3619,17 +3633,7 @@ function system_build_contextual_links($
-  $build = array();
-  if ($links) {
-    $build = array(
-      '#theme' => 'links',
-      '#links' => $links,
-      '#attributes' => array('class' => array('contextual-links')),
-      '#attached' => array(
-        'library' => array(array('system', 'contextual-links')),
-      ),
-    );
-  }
-  return $build;
+  $element['#links'] = $links;
+  return $element;

We need to unset #theme and some other properties to skip the rendering of links if there are no contextual links, since there is no break condition like #printed after #pre_render in drupal_render().

This review is powered by Dreditor.

David_Rothstein’s picture

As can be seen from the old code, we only want to apply the cl-region class if there are any contextual links rendered.

I'm not sure we have a way to do that with this approach?

I mentioned above that this would happen, but I'm not sure it's that big of an issue compared to the benefits that this patch would provide. It's still a contextual links region in those cases, anyway, just one that doesn't have any visible links. Certainly for the way core uses this class, it shouldn't to matter, and I'm having trouble imagining a way that a theme would use this where it would matter either.

+ '#parent_element' => $element,

So what is this for?

It allows pre-render functions to have access to the full element if they need that info while building the contextual links. Before the patch, system_build_contextual_links() had this access (although didn't actually use it for anything). If we decide that contrib modules don't need it, that would of course remove my concern in #3 about passing around giant arrays.

We need to unset #theme and some other properties to skip the rendering of links if there are no contextual links, since there is no break condition like #printed after #pre_render in drupal_render().

Well, I guess we could unset #theme and #attached - is that what you meant? But the worse that happens is that theme_links() gets called with an empty array (and returns quickly), and a CSS/JS library gets attached that isn't used. I guess it is a micro-optimization for the case of a user who has overall permission to use contextual links but doesn't actually have access to many contextual links on the page. I'm also not sure what #printed has to do with this - maybe I am misunderstanding?

sun’s picture

Can we please do #626286: Make contextual links a module first?

It allows pre-render functions to have access to the full element if they need that info while building the contextual links. Before the patch, system_build_contextual_links() had this access (although didn't actually use it for anything). If we decide that contrib modules don't need it, that would of course remove my concern in #3 about passing around giant arrays.

It's a copy of $element, so any further customizations in preprocess and process hooks are not contained. In general though, if it would be possible, then I think it would make sense to pass #element => &$element, by reference, and only pass that (because it already contains $element['#contextual_links']).

Well, I guess we could unset #theme and #attached - is that what you meant? But the worse that happens is that theme_links() gets called with an empty array (and returns quickly), and a CSS/JS library gets attached that isn't used. I guess it is a micro-optimization for the case of a user who has overall permission to use contextual links but doesn't actually have access to many contextual links on the page. I'm also not sure what #printed has to do with this - maybe I am misunderstanding?

It's not only about performance, but also about potential markup cruft in the output. Since it is a #type, it can be altered. So any of the following can be set: #pre_render, #theme, #theme_wrappers, #post_render, #states, #attached, #cache, #prefix, and #suffix.

Looking at http://api.drupal.org/api/function/drupal_render/7 once again, we can simply return an empty array if there are no links to prevent anything bad from happening.

sun’s picture

Component: system.module » contextual.module
casey’s picture

Subscribing. Needs a reroll at least.

sun’s picture

Assigned: Unassigned » sun
Priority: Normal » Major
Status: Needs work » Needs review
FileSize
5.85 KB

Re-rolled against HEAD. Attached patch should be ready to fly.

moshe weitzman’s picture

Status: Needs review » Needs work

Looks good. I thought this went in already.

Please skip $destination = &drupal_static(__FUNCTION__); or show benchmarks which prove we need it. This has gotten silly adding caches everywhere. drupal_get_destination() already has static cache.

sun’s picture

Status: Needs work » Needs review
FileSize
5.95 KB

oh thanks! - didn't know that; must have been added in the meantime.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

code looks good, and fixes a serious WTF

lpalgarvio’s picture

was thinking... there's an edit and delete for content in D7 alpha6.
how about per default, adding also the links publish (if it's unpublished), unpublish (if it's published), promote, unpromote, etc... ?

imo there shouldn't be a need for a module to deal with those actions since they are already in core.

also, would be great if any module could add links to the contextual gear, based on what content type, block, etc is being hovered.
a module like workflow could in the future provide extra links for that matter.

and a backport for D6 of this module would be just great :P
i can imagine it already eclipsing away these modules:
* Administer Tool Tip
* Swentel's Contextual links
* Block edit
* Admin links
* Fasttoggle
* Admin:hover

sun’s picture

@LPCA: While I think that the publish/unpublish links are a good idea, it's unfortunately too late for D7 to implement them. Only unpublish would be possible, since each link needs to be connected with an actual page/menu router path, and there is no page to publish a node yet (only one for unpublishing, which is used by mass-operations/actions). Thus, this has to be done in contrib and can only be considered for D8.

Frando’s picture

The patch looks great, this is a much needed cleanup and makes contextual links consistend with our standard render mechanism. RTBC+1.

#13: This is not related to this patch. If you want to add publish and promote links in core, please create a new issue. And contributed modules can already add more contextual links.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)
Issue tags: -Contextual links

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