Needs review
Project:
Entity API
Version:
7.x-1.x-dev
Component:
Core integration
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
24 May 2012 at 12:23 UTC
Updated:
21 Jan 2019 at 20:12 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
joachim commentedComment #2
fagoSounds good.
We need to document both variables at the template's docblock.
I think the point is more we don't need the contextual links if page == is TRUE, as usually we've tabs then.
Comment #3
joachim commentedNew patch with those included.
Comment #4
fagoThanks, 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?
Comment #5
joachim commentedCore 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().
Comment #6
fagoI see, thanks for clarifying.
Comment #7
fagohm, 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.
Comment #8
joachim commentedDoesn'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!!!
Comment #9
fmizzell commentedIt 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
Comment #10
BarisW commentedAgree with #9, yes please! Please commit, and alter later if needed.
Comment #11
BarisW commentedI've tested #7 and this works perfectly. I see the links to edit the entity AND even the Manage display link :) Wow!
Comment #13
fubhy commentedI 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?
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.
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.
Comment #14
damienmckennaThis is the patch from #4 rerolled due to the tpl file being moved to a subdirectory.
Comment #16
damienmckennaDoh! Sorry about that.
Comment #17
danepowell commented#16 has been working well for a year now.
Comment #18
zmove commented+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...
Comment #19
kristiaanvandeneyndeThe key 'fragment' should be the implementing module's name. See contextual_pre_render_links()
Comment #20
kristiaanvandeneyndeTry this one: should work all the same, but follow the contextual links' module rules.
Comment #21
kristiaanvandeneyndeMy bad, another patch snuck in my git format-patch. Correct one attached.
Comment #22
donquixote commentedI 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.
Comment #23
donquixote commentedI 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?
Comment #24
donquixote commentedI 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:
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.
Either of this would be the job of profile2.
So I think profile2 is a bad example for a generic solution in entity API.
Comment #25
donquixote commentedFrom the patch in #21:
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:
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().
Comment #26
chris matthews commentedThe 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.