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.
Comment | File | Size | Author |
---|---|---|---|
#11 | drupal.contextual-links-alter.11.patch | 5.95 KB | sun |
#9 | drupal.contextual-links-alter.9.patch | 5.85 KB | sun |
#3 | alterable-contextual-links-627288-3.patch | 2.33 KB | David_Rothstein |
alterable-contextual-links.patch | 2.57 KB | David_Rothstein | |
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #2
sunWe want to keep this @see, because the functions run together.
The #contextual_links property is not required and not used for the #type and can be removed.
Comment #3
David_Rothstein CreditAttribution: David_Rothstein commentedOK, 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...
Comment #4
sunThe properties are a bit weird now.
As can be seen from the old code, we only want to apply the cl-region class if there are any contextual links rendered.
So what is this for?
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.
Comment #5
David_Rothstein CreditAttribution: David_Rothstein commentedI'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.
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.
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?
Comment #6
sunCan we please do #626286: Make contextual links a module first?
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']).
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.
Comment #7
sunComment #8
casey CreditAttribution: casey commentedSubscribing. Needs a reroll at least.
Comment #9
sunRe-rolled against HEAD. Attached patch should be ready to fly.
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commentedLooks 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.Comment #11
sunoh thanks! - didn't know that; must have been added in the meantime.
Comment #12
moshe weitzman CreditAttribution: moshe weitzman commentedcode looks good, and fixes a serious WTF
Comment #13
lpalgarvio CreditAttribution: lpalgarvio commentedwas 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
Comment #14
sun@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.
Comment #15
Frando CreditAttribution: Frando commentedThe 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.
Comment #16
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.