Updated: Comment #55
Problem/Motivation
hook_menu() is still responsible for contextual links, so we can't remove the old menu router.
In addition the system for finding contextual links is based on a confusing combination of path hierarchy and hook_menu() type constants.
Proposed resolution
A totally new mechanism for finding contextual links is added using plugins that are very similar to those used for local tasks.
The plugin type which is discovered using YAML files and use routes to render the contextual links.
AJAX is still used to fetch all the rendered contextual links for a page. A minor change is made to the format of the contextual links ID that's rendered in the HTML so that we have named parameters compatible with route variables.
Remaining tasks
Follow-ups to convert remaining contextual links and update Views to use the new system (possible via plugin derivatives).
User interface changes
none
API changes
Define contextual links via grouped plugins (discovered via YAML) instead of via hook_menu.
see change notice: https://drupal.org/node/2165243
Comment | File | Size | Author |
---|---|---|---|
#138 | interdiff.txt | 1.77 KB | dawehner |
#138 | contextual_links-2084463-138.patch | 85.49 KB | dawehner |
#136 | interdiff.txt | 3.27 KB | tim.plunkett |
#136 | contextual-2084463-136.patch | 85.19 KB | tim.plunkett |
#134 | contextual_links-2084463-134.patch | 84.86 KB | tim.plunkett |
Comments
Comment #1
dawehnerLet's see how much do actually break.
Comment #3
XanoLet's call the service plugin.manager.contextual_links (plural) for consistency with everything else.
Is URL path support supposed to be temporary, until everything is made route-based?
Is there a reason why you opted for "title" rather than "label", which is what we use in most places in D8?
We need an annotation class for these plugins and we can make the options definition default to an empty array, so we don't need these checks.
Comment #4
XanoComment #5
neclimdulNo annotation class because you aren't annotating. There's a defaults array in the manager. "options" is actually set in the defaults. weight is missing though.
Comment #6
XanoYup, that's what I found out too. My mind thought "Derivatives? Annotated classes!".
This patch adds a default value for the weight, removes an erroneous copypaste code comment, and some Views-related code that @dawehner told me to get rid of. I have to go somewhere, so I can't look into the test failures just yet, but let's see what the removal of that Views-related code does for them.
Comment #8
Wim Leerss/LinK/Link/
Weird function doc.
That's a weird comment.
Unnecessary whitespace change.
Why is this additional
$path_args
necessary?Without further explanation as to why it is necessary, my first impression is: "this is adding keys just for additional explicitness".
And what's worse: the colon was used to separate each of the main components, but now it is apparently also being used to serialize the values of this component, so I don't see how this can be reliably deserialized. At least, this now seems to have become more complex/confusing.
Comment #9
pwolanin CreditAttribution: pwolanin commentedI agree re: the serialization. Maybe we should just use a standard format like JSON instead?
Comment #10
Wim LeersOr, why not stick to what we already have?
Comment #11
dawehnerThe new routing system uses an parameter array all over the place which basically consists of values + keys with the name of the slug appearing in the route pattern.
This information is needed in order to generate the actual contextual links. so we somehow have to pass that information along. The patch just adds the keys of the parameters additional to the values.
Comment #12
Wim LeersI think Peter has a point: the serialization used to be super simple, but now there's apparently a need for key-value pairs to be serialized as well. Then it might be wiser to just switch to JSON.
Furthermore, it seems that this is excessive now:
It seems we can get rid of
$module
now, since every route must be unique anyway, and we don't need to call a module-specific hook anymore? And shouldn't$parent_path
be renamed to$route_name
now?Also, where/how are the contextual links being defined right now? The combination of
ContextualLinkInterface
being added and then not being implemented by any of the things that provide contextual links is very confusing.Which brings me to my final point: the resulting code is now so opaque that I don't understand anymore what's going on. It'd be useful to explain the code flow in the issue summary.
Comment #13
XanoContextual links are defined through YAML. Any definitions will automatically be applied to a base class that implements ContextualLinkInterface. Any developers who wish to add more logic to their links or override the base class can add a "derivative" key to the YAML definitions which points to a derivative discovery class which can dynamically declare link definitions with their own "class" keys that point to other classes than the base class.
Comment #14
pwolanin CreditAttribution: pwolanin commentedWell, and you can also just directly specify the class key to hae a single custom implementation. This is this sort of thing I'l do for local actions and maybe a contextual link for clone module when I upgrade it.
I aslo agreee with Wim that we can flatten this a bit, but the top level key is really the "group" not the route.
Comment #15
Wim Leers#13: but I don't seen any routing YML file being changed in this patch yet? I'd like an explanation that covers the entire flow, from where developers define contextual links, to how it is rendered.
Comment #16
Wim Leers#14: Yeah, I'm not sure anymore what's what in the current state of thing :) :(
Comment #17
dawehnerRegarding 3: This is the only proper way to typehint variables in code. This makes the code easier to understand as you exactly know what is expected here.
I haven't done the conversion to JSON as the JS somehow can't deal with the different IDs anymore etc.
Comment #18
Wim LeersInteresting, I didn't know that! :)
I'm not surprised that the JS doesn't work as expected anymore, because I don't see how the modified serialization can work reliably.
What changed in #17? (Interdiff is missing.)
Comment #19
dawehner#17 just fixed some of the points you mentioned but not the serialization parts. Sorry.
Do you think you can help on getting the JS side of it working?
Comment #20
dawehnerLet's see how much this fixes.
Comment #22
dawehnerFixed the remaining failures.
Comment #23
dawehner#22: contextual_links-2084463-22.patch queued for re-testing.
Comment #24
pwolanin CreditAttribution: pwolanin commentedOk, I can see why JSON encoding is maybe not the best idea - it uses " chars which breaks out of the attribute, so they'd have to be escaped and then... ?
Here's a simpler approach that's already used in the module - just encode as for a query string (or form POST data) and use the parse_string() built-in.
Also, makes doc fixes and renames the "Base" class to "Default" to align with what we did with the LocalTaskDefault
to get the easy $this->t() for the default plugin we need: #2087231: Add a PluginBase in the Core namespace with t() as a helper method
Comment #25
pwolanin CreditAttribution: pwolanin commentedoops, patch #22 had some test methods commented out. Fixing that and making variable naming more consistent.
Comment #26
pwolanin CreditAttribution: pwolanin commentedFixes some tests I missed
Comment #28
dawehner#26: contextual_links-2084463-26.patch queued for re-testing.
Comment #30
pwolanin CreditAttribution: pwolanin commentedmissed one link ID in MenuTest
Comment #31
dawehnerAdded a unit test.
Comment #32
pwolanin CreditAttribution: pwolanin commentedIs there a module or system whose links we could convert now?
Comment #33
dawehnerComment #35
dawehner#33: contextual_links-2084463-33.patch queued for re-testing.
Comment #36
Wim LeersGreat progress here! I'll try to review soonish.
Comment #38
dawehnerComment #40
dawehnerJust a simple rerole.
Comment #41
pwolanin CreditAttribution: pwolanin commentedLet's get this in - the last few conversions can be done in a follow-up.
Comment #42
pwolanin CreditAttribution: pwolanin commented#40: contextual_links-2084463-49.patch queued for re-testing.
Comment #44
dawehnerJust a rerole.
Comment #46
dawehnerbla.
Comment #47
pwolanin CreditAttribution: pwolanin commentedRe-roll for change to NodeRenderController.php in #1605290: Enable entity render caching with cache tag support
Comment #48
pwolanin CreditAttribution: pwolanin commentedremove bare t() call in response to #2087231: Add a PluginBase in the Core namespace with t() as a helper method
Comment #50
pwolanin CreditAttribution: pwolanin commented#48: contextual_links-2084463-48.patch queued for re-testing.
Comment #52
pwolanin CreditAttribution: pwolanin commentedNeed to use the new Core not existing Component PluginBase to get the t() method.
Comment #54
dawehner#52: contextual_links-2084463-52.patch queued for re-testing.
Comment #55
pwolanin CreditAttribution: pwolanin commentedUpdating the summary.
Comment #55.0
pwolanin CreditAttribution: pwolanin commentedUpdated issue summary.
Comment #56
dawehner#52: contextual_links-2084463-52.patch queued for re-testing.
Comment #56.0
dawehnerUpdated issue summary.
Comment #57
fubhy CreditAttribution: fubhy commentedLooks good architecturally... Just a couple of remarks.
Lacks @return
ContextualLinkManager (singular)
Are we allowed to do this now?
Not introduced here, i know... But: abbreviated variable names--
Is 'title' really appropriate for contextual links? It's rather a 'label' really.
manager manager.
Sucks that we have to repeat this boilerplate code for every plugin manager unit test.
Comment #58
dawehnerMh, I see your point but I prefer consistency over correctness.
We could also extend the class and just add some setters but you know both are bad
Comment #59
fubhy CreditAttribution: fubhy commentedRTBC if green.
Comment #60
Wim LeersReview of ##5, which I started during DrupalCon but was unable to finish. I apologize for the duplicate remarks, but considering I have many more remarks, the duplication should be limited.
Contextual link plugin manager.
Shouldn't this be 0? A weight of NULL seems weird.
s/Links/Link/
I personally find it more legible if there's a newline after a call to the parent class; but that might just be me.
Leading spaces.
Missing trailing periods.
The "which should be shown" part seems inaccurate?
s/theme_links/theme_links()/
Extraneous newline.
Can't we delete all "context" properties of menu items?
I think this should include the language ID? How else will these be translatable? And: how would e.g. French Drupal developers be able to define these in French first, English second?
This seems wrong? However, I can still delete custom blocks … so maybe not?
Please just double-check :)
What does
$res
mean? You're receiving contextual link plugin definitions. In the called function, it's called$contextual_links
. At the very least, this variable should be named more clearly.Why not convert all contextual links? Follow-ups are not really accepted in this phase of D8 anymore AFAIK.
Much, much better — thanks! :)
"path args" are no longer exactly what they used to be before. They appear to be route parameters instead now. Which is fine. But then we should update the variable names as well.
More importantly, at least one of the parts of the pattern is now in fact the group name that is defined in the contextual links YAML files. But it's impossible to tell which: the module name, or the parent path? I think the parent path. If it's the parent path, then that variable name should be updated as well.
This structure change is most peculiar. Due to the values, it used to be very clear/explicit which is the module name and which is the path. Now it's impossible to tell which is which. Furthermore, I can't help but wonder why we need a nested array here?
Comment #61
pwolanin CreditAttribution: pwolanin commentedre: #11 see: #2088371: YAML discovery incompatible with translations
Given that the breadcrumb patch went in, we should be able to delete most or all of the hook_menu entries.
Comment #62
dawehnerNice review!!
NULL was used for local tasks in order to provide a different weight for the default tab, though yeah this is not needed here.
I prefer that as well.
Yeah let's just drop that addition. The context is the function call, that's it.
You are totally right.
Well, it was always the case in drupal that everything which is wrapped with t(), both in the .info file, .module/.php files or whatever is considered as some sort of English. There is not need to special case that here.
Ups ... this was a mistake.
The problem is that we still have non-route menu_router entries.
Well, we need the route parameters as additional array that is clear.
I agree that this nested structure is odd. I guess the reason is to easily add new links to an existing module but yeah what about adding a follow up to improve that, so we don't have to touch existing non-converted code?
Comment #63
pwolanin CreditAttribution: pwolanin commentedNULL as the default weight is consistent with FAPI and also allows us to distinguish the plugin setting it to 0 from it being the default.
We should go back to that - it's the correct default.
Comment #64
pwolanin CreditAttribution: pwolanin commentedComment #65
Wim LeersThis sounds to me like this patch should (sadly :() be blocked on having completed routing system conversions? Only then will we be able to make all this very explicit, very clear, non-ambiguous?
Comment #66
dawehnerI am against the approach of doing everything at once. You know, what really matters is progress and this can be achieved with this patch. Ripping out things once it is done is damn simple.
Comment #67
pwolanin CreditAttribution: pwolanin commentedI'd agree - we are surely going to have to iterate a bit on this as we actually do the conversions, but we need something in place we can start working with.
Comment #68
dawehnerGiven that, let's put it back to RTBC.
Comment #69
David_Rothstein CreditAttribution: David_Rothstein commentedI'm not particularly religious about this (especially because I was originally on the other side of the debate in Drupal 7) but this is adding extra work for developers; if they want their contextual links to have the same title and weight as regular links, they now need to duplicate that information in two places.
See for example @sun's comments in #473268-139: D7UX: Put edit links on everything and elsewhere.
It's quite possible the extra flexibility is worth it, but it's surprising that this issue has managed to sidestep that whole debate, and adding extra work for developers is something we should always be careful with. It's not actually obvious to me that contextual link definitions should be separated from regular link definitions (i.e. a large part of the premise of this issue), since they aren't really separate links so much as they are different presentations of the same links.
Why are you turning "Delete" into a tab? Shouldn't this entry just be removed?
If I'm reading this right, this is a totally new and different hook compared to
drupal_alter('menu_contextual_links', $links, $router_item, $root_path)
in menu_contextual_links().Seems like this needs to be documented.
It's also cached (whereas hook_menu_contextual_links_alter() isn't) so if you want to use it to e.g. dynamically change the titles or otherwise dynamically edit the links, you'd be out of luck?
Comment #70
dawehnerThank for your feedback, highly appreciated.
Added the functionality and a test.
I totally see the point that coupling paths, menu links and routes into a single syntax helps to keep the amount of typing as low as possible. On the other hand though
we need to get rid of the old routing system. One additional point is that we already moved local actions and local tasks to its own file, so we should try to make things consistent and as simple as possible. Therefore
we sadly have to port many examples in order to find out common problems. (we did that for example for local actions which really helped).
On this was not on purpose. Good catch, really!
We are caching the result of the contextual links by group:
which means that we end up with really a few cache entries (there are like ~5 groups in core at the moment). This could be a potential improvement for big sites can cache this information in memcache.
I think a static cache would totally make sense given that contextual links, on node for example, appear multiple times per page. Do you think we should get in the static cache already in this patch or open a follow up?
You are right, this is not about the actual rendering contextual links, but about the definitions of them. This is needed for some advanced usecases like for example views. I renamed the actual hook here.
The amount of alter hooks is a bit confusing. Is it enough to have one alter information for the static information and one for the links determined before returning to the rendering?
Comment #71
pwolanin CreditAttribution: pwolanin commentedDo we really even need a run-time alter hook? These are plugins so if you need a dynamic title just define your class to provide it.
Comment #73
David_Rothstein CreditAttribution: David_Rothstein commentedReading through the new patch those code changes look good overall.
Normally I'd say a followup (in the spirit of not adding static caches unless they're known to be necessary) but since there was already one in the existing menu_contextual_links() code I think it would be good to add here. And I think it was there for the reasons you mention above.
I would say the second one (the dynamic one) is important to have; it's not just about dynamic titles but also about things like removing links based on certain conditions, etc. Given the second, perhaps the first (the static one) isn't necessary though.
Are both of these necessary? If so, it could be stored in a variable rather than determined twice.
Not sure offhand if this example is correct, either security-wise or translation-wise...
Comment #74
dawehnerThe second one was a leftover from some of my experiments but I decided to not at the contextual link in the first place, as the old menu based system did the same.
Can you think of a better idea what to provide as example?
I would like to keep the static one, as every plugin type in core basically has this alter hook.
For local tasks really special usecases like views have to provide local tasks based on other ones,
so you can use a static alter method.
This fixes the existing test errors ... aka. I have just ran the unit tests, shame on me.
Comment #76
dawehner#74: contextual_links-2084463-74.patch queued for re-testing.
Comment #78
dawehnerRerolled.
Comment #79
Wim Leers#70: thank you so much for that review, it very clearly shows that this patch was definitely inappropriately RTBC'd earlier.
Translation handling is still opaque to me.
I agree that progress matters. But quite often, these kinds of follow-up tasks are not completed timely, which results in people not knowing how to make things right.
Furthermore, I still feel that the nested structure is highly problematic. It's very, very hard to understand. Previously, it was implicitly clear because of its values. But now that the values have become so similar, it's a puzzling mess.
Hence I ask again to let this patch wait until all non-route menu_router entries have been converted.
If I can still find problems in this patch so easily, then I don't think this should be RTBC'd unless David Rothstein and maybe another person reviews this patch another time after a reroll for these problems I just found:
s/on/in/?
Also, the concept of a "contextual link group" is not explained anywhere.
And the important fact that it must be *unique* inside all of Drupal should also be mentioned, I think.
s/Plugin/Contextual link plugin/ ?
So the route parameters must be the same for all contextual links. Which makes sense. But I think it's valuable to have that documented/explained explicitly here.
It wasn't necessary before, because you could only do "node/X/OPERATION" things. But now that you can add arbitrary routes, i.e. arbitrary paths, so an additional explanation is warranted IMO.
So much better than before :)
Again, since that still hasn't been fixed:
- parent path -> group
- args_raw -> route_parameters
- metadata_raw -> options
In fact, why was
$metadata
renamed to$options
? That is such a broad term, that it makes things even harder to grok.s/or/and/
s/must at least contain/must contain at least/
The group of contextual links being rendered.
Replace all this with just "e.g."
s/menu edit/menu_edit/
Should be wrapped in
t()
?It says "keyed by plugin ID" here, but AFAICT this is not a plugin ID, it's a "contextual link ID".
"method" missing in this sentence.
s/Setup/Set up/
Accidentally ended up in patch, I think :)
Overall, I still feel this makes for an unnecessarily complex system. Mitigating facts:
Only by rationalizing it this way, I can convince myself that it'd be acceptable for this to go in. So: I'm fine with this if e.g. David Rothstein is fine with it. However, I continue to think that nested array structure is a major WTF.
Comment #80
dawehnerThank you very much for this long review.
Hopefully the new lines match your requirement.
Right, this behavior is assumed by the full new routing system all over the place.
This for example have to be figured out in the generic views implementation of contextual links, which basically requires another bunch of code. This is one of the the reasons why I don't want to drop all
implementations in this patch. It just blocks itself.
Adapted. It seems to be for me that options are used in many more places in core, so people are more familiar with it than metadata.
Regarding 6 and 7, these have been copy and pasted lines of documentation, but nevermind, I just changed it.
Good idea.
Now we are getting really nitpicky, don't we?
Oh damn.
Well, if we have converted all instances in core we should maybe switch to keyed parameters like this:
Comment #81
David_Rothstein CreditAttribution: David_Rothstein commentedYeah, I agree with Wim Leers that this is pretty confusing (the original is a bit confusing too, but at least you can read it left-to-right and understand it is forming the base of the URL).
I think @dawehner's idea in #80 of using some kind of associative array with named parameters would help a lot. But maybe more like this?
I left out the "module" key because it isn't actually used for anything. And yes, since the first "block" (the array key) isn't actually used for anything either, it could just be a numeric key as in #80, but that wouldn't be good for anyone who is trying to alter this. So it should be a meaningful string, and by convention it could just be the same as the group.
Following up on the discussion above, I wonder if that's actually true? I would expect that if you pass all possible route parameters along, multiple links within the same group could still use only a subset of them.
So if you pass
array('node' => $node, 'entity' => $entity)
as the route parameters, and some contextual links within the group point to routes which require $node whereas others point to routes which require $entity, maybe it just works?By the way, (minor) in the above "This route parameters" should be "These route parameters".
Doesn't work currently, since $group should be $group_name.
I discovered that when looking into the next item....
So I tried it out and actually it looks like we don't need to sanitize this. When I did it led to double-escaping (since ultimately this title gets passed into theme_links() which sanitizes the title). So perhaps we want !label in this example.
I assume the translation is needed here, of course. Though I'm not sure I understand where regular (non-altered) link titles from the .yml file are getting translated, it does seem to be happening somewhere.
Comment #82
dawehnerAs far as I remember this is used for some parts of the CSS but I agree we can drop it.
I do really like your alternative syntax.
Sadly this is not the case. The symfony routing system has indeed some flaws like here. Every parameter which does not appear in the url pattern would be added as a query parameter, so you end up in
node/123?entity=123
Extended the unit test.
These ones are translated in the getTitle() method of ContextualLinkDefault, see the following piece of code:
Note: I haven't changed the syntax yet, as the examples of views and content_translation as not that easy to convert.
Comment #84
dawehnerThere we go.
Comment #86
tim.plunkett#84: contextual_links-2084463-84.patch queued for re-testing.
Comment #87
pwolanin CreditAttribution: pwolanin commented@David_Rothstein re: #81 - you say the array key is not used, but why don't we map that to the group name instead of making you also list the group?
so it could be as simple as:
or we could give the group a more distinct name:
Comment #88
dawehnerI went we keeping the module name so you have
'node.node' => array('route_parameters' ...)
to keep the changes as small as possible.Additional this patch converts the content_translation as well as views_ui contextual links. There is now just a single place where the old
contextual links api is used. The remaining one is the dynamic support of configured views to provide a contextual link. I think we have to
change the UI in order to support group based contextual links.
Comment #89
aspilicious CreditAttribution: aspilicious commentedWhile porting ds to drupal8. We had serious problems to insert a contextual links based on the 'view_mode', 'entity_type', ... and so on, on each entity. I hope this patch doesn't make it worse...
Didn't had the time yet to test it.
Comment #91
dawehnerForgot to add some new files.
Comment #93
dawehnerFixed the test failures and whitespace.
Comment #94
tim.plunkettLet's link this to #2102777: Allow theme_links to use routes as well as href directly, and remember to indent multiline @todos
Weird double comment
The class is changing from views-ui-edit to views-uiedit, is there any reason not to fix this?
Why only kill one flag? I see this is done in other places, is it planned to also kill MENU_CONTEXT_PAGE in this issue or in a follow-up?
double space before extends
Why the extra class? Doesn't seem to be needed anymore.
Alter the plugin (not Alters in hook docs)
Comment #95
dawehnerThank you very much for the review!
The rule is simple: The more documentation the better :P
Let's keep it for now.
MENU_CONTEXT_PAGE has its own dedicated issue: #2102125: Big Local Task Conversion
Good catch!
We are really good in being inconsistent.
Comment #96
tstoecklerWow, this is a nice piece of work. I have a bunch of comments, but they are mostly minor. They do warrant a needs work in total, I think.
Super minor, but could be just
I had a bunch of separated comments, but instead I'm just proposing an improved version of this comment. Feel free to take over whichever parts you like:
It would be cool if we could be more explicit about this. Unique among what? Per page? per route?
"based to" sounds weird. Maybe just "Returns an array of link options" as the @return already specifies what is meant. Maybe also add a @see to LinkGeneratorInterface::generate (with the FQNS, of course).
As the weight, I assume, is relevant only within a single group, might be nice to add another sentence like "The contextual links in one group are sorted by weight for display." or something.
For extra credit, this would extend ContextualLinkManagerInterface which defines the added functions here, i.e. getContextualLinkPluginsByGroup() and
getContextualLinksArrayByGroup().
Since that would be a tad more work, and this is a rather pressing issue, I think it would be fine to do that in a follow-up.
Just for clarity I think it would make sense to put 'node' and 'entity' in quotes.
"links as array" sounds strange. Maybe: "An array of link information, keyed by the plugin ID. Each entry..."
Is there a reason for this high weight? I think 'Translate' is one use-case for showing up between 'Edit' and 'Delete' in fact.
This is missing documentation for the array value. I.e. that is in array of 'route_parameters' and 'metadata'
So should this be instead?
Also 'weight' I think and 'options'?
This can't be correct, I think.
Comment #97
dawehnerGreat review!
Just took that bit as it is.
Well, i just wanted to make clear that all contextual links with the same group are rendered in the group. This though should be also somehow covered by the previous block of docs.
Just replaced it with "passed to"
Added the interface.
No there is not. Set back to 2 as it is in the hook_menu() entry.
Fixed.
Added weight. Options is localized_options, ... we should clean that up later.
Good catch.
Comment #98
tstoecklerThanks for the quick fixes.
Only one minor thing left to complain.
Missing an empty newline.
For me this is RTBC, but I don't feel comfortable enough with the menu system and with contextual links to RTBC this myself.
Comment #99
dawehnerThere we go, and hey someone can take the chance and RTBC it on comment 100.
Comment #100
tim.plunkettI reviewed this in #94, and reviewed the interdiffs since, this is good to go.
#100 woooooo!
Comment #101
David_Rothstein CreditAttribution: David_Rothstein commentedI'm going to say "block.block" is basically equivalent to the confusion that Wim Leers was worried about above...
This gets used in
list($module, $group) = explode('.', $module_group)
but I don't see how $module is ever used in a meaningful way? I would expect the group to always be enough to identify the group of links, so either like #81 (where 'group' is explicitly defined) or #87 (where the group is the array key) and not bothering with $module at all seem like they would be a lot simpler for module developers and with no apparent loss in functionality.Also:
I'm not seeing how the changes in the examples match the changes in the pattern.
Comment #102
dawehnerWell, module is for example used in the code which still exists (menu_contextual_links()) which we cannot drop yet.
Battle-plan: Get this patch in, convert the last usage of hook_menu() based contextual links and then drop the $module. Would that be acceptable?
I adapted so that the views_ui example also has the new syntax.
Comment #103
xjmComment #104
pwolanin CreditAttribution: pwolanin commentedI agree with both David and Daniel - we need to get this in, but also need to clean it up as a follow-up to be only based on group.
Comment #105
webchickSorry, but why can't the group thing be done in this patch? In theory we are in the phase of the release where we're trying not to actively break peoples' modules. This is already going to be a huge break, why cause another one a couple of weeks later?
Comment #106
webchickOh, ok. #102 explains the why. Can I counter-ask then why we can't leave this postponed until those things are fixed, so we can do this conversion once?
(Not moving from RTBC, this is just a question.)
Comment #107
dawehnerAs explained in IRC the views bit is not simple to port.
Comment #108
webchickRight. So I think basically the premise is, "This is holding up a critical [remove the old router system] and so we're asking to commit this as an interim step."
I think that's probably a fair request, but I think either David or Wim should probably be the ones to RTBC it, rather than a co-author of the patch.
That also by definition means this won't hit alpha4, but by holding it until alpha5, we have a chance to complete the views/contextual links follow-ups and remove the module names by then, so to module devs chasing alphas, it's just one API change, not two.
Comment #109
tim.plunkettI RTBC'd this in #100.
@David_Rothstein bumped it in #101 because the $module/$group thing was confusing, but also because the example was wrong.
@dawehner fixed the example in #102 and explained why we need this in before fixing anything else
So the re-RTBC in #104 is just a hold-over from mine.
I would mark this RTBC again, but @webchick specifically asked for Wim or David to do that, so I'll sit on my hands for a bit :)
Comment #110
David_Rothstein CreditAttribution: David_Rothstein commentedI'll take a look tomorrow.
Comment #111
David_Rothstein CreditAttribution: David_Rothstein commentedI looked this over and I agree with @webchick in #105. It's not clear to me why we can't just change the API a single time now. Introducing $module as part of the new API only to remove it later shouldn't be necessary.
So I tried getting rid of it in the attached patch. Let's see how it goes with the testbot.
Also, the menu_contextual_links() call in #102 doesn't look like it would have ever worked? From the patch there:
But menu_contextual_links() expects the second parameter to be a path, so I don't see how $group could have been the right thing.
Given that the tests still passed in #102, that makes me wonder if we can't just go ahead and remove that whole menu_contextual_links() function now too... But I left it in this patch (and fixed it) for the time being.
I made a couple other tiny fixes in the attached patch too, but the vast majority of it is just replacing $module.$group with $group.
One other thing I noticed:
That looks like an unfortunate change (and there are a few others like it elsewhere). It wasn't obvious to me where this comes from (something to do with the plugin ID being used to build up the array of contextual links?).
Comment #113
David_Rothstein CreditAttribution: David_Rothstein commentedOK, hm, so it looks like the tests in #102 only passed due to a coincidence. In the case of nodes, the $group is "node", which also happens to be the first part of the path for node URLs ("node/%/edit", etc). That is why the menu_contextual_links() call worked - but only in that specific case. Furthermore, the tests only pass because the test user doesn't have access to other node-related contextual links (besides the one provided by the view)... In real life, the Views functionality for contextual links is still broken.
I can make #111 work the same way - see attached. I think that will get the tests passing. However, I don't see the point of hacking core to make a test pass, if the functionality the test is trying to test doesn't work in practice... I think it's preferable to just remove the broken test.
For clarity, attached interdiffs are compared to #102 and to #111. Note that I restored one code comment ("A list of places where contextual links should be added") that the original patch changed but that shouldn't have been.
Comment #114
David_Rothstein CreditAttribution: David_Rothstein commentedHere's the version that would make more sense to me - since it doesn't seem like we can make the Views stuff work at all in this issue and the only reason menu_contextual_links() was left behind was to try to make it work, just remove menu_contextual_links() instead and then leave a @todo about the broken Views functionality.
That's not great of course (it's a rather large @todo) but it's more honest about the state that this patch leaves Drupal in, plus it means we get the whole API change done at once.
Comment #116
tim.plunkettI'm pretty sure I'd prefer "future API change" to "breaking all of Views contextual links".EDIT: Seems I misunderstood.
Carry on.
Comment #117
dawehnerThe main reason why the contextual link part of views worked without issues on one of my patch was that the group "node" matched the pattern "node", now that I realized that.
The actual problem with removing the feature out of views temporarily is that we don't seem to have a clear idea how to implement
it inside the new system, because otherwise it would be part of the patch itself. I do have some potential implementations though which all require changes in the UI.
This just fixes the exceptions, ...
Comment #118
David_Rothstein CreditAttribution: David_Rothstein commentedYeah, exactly. It seems like the user interface would have to change to allow the administrator to choose a contextual links group?
---
By the way, since this discussion may be confusing to others, the functionality we're talking about is the one that allows you to configure page displays to appear as contextual links. For example:
cp core/modules/node/tests/modules/node_test_views/test_views/views.view.test_contextual_links.yml core/modules/node/config/
(quick way to get a View that has this functionality)As far as I understand, all patches in this issue currently break that functionality.
Comment #119
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, I'm not sure how we left things with the caching discussion, but in testing this I did confirm that the following code:
does seem to be hitting the database cache an awful lot (for example, once for each node on the page, in the case of contextual links on nodes). So it's missing the static caching that menu_contextual_links() had to try to limit this to one database hit for all nodes on the page, and this would be a good thing to add (or at least make sure we have an issue to track adding it).
Comment #120
David_Rothstein CreditAttribution: David_Rothstein commentedAdding back the disappearing tags.
Comment #121
dawehner.
Comment #122
dawehnerHere is also a follow up for the functionality in views: https://drupal.org/node/2117845
Comment #123
dawehnerJust another rerole.
Comment #125
dawehner#123: contextual_links-2084463-123.patch queued for re-testing.
Comment #126
tim.plunkettI think #121 addressed #119, and #2117845: Bring back contextual links support in views or drop it. is the follow-up discussed in #114.
I re-reviewed it, and it is once again RTBC.
Comment #127
David_Rothstein CreditAttribution: David_Rothstein commentedI think so too, although #2117845: Bring back contextual links support in views or drop it. may need to be bumped in priority as a followup (currently the patch leaves the user interface in Drupal, but it doesn't work at all).
I also still find this kind of thing (from #111) confusing:
It could be a followup, although that would be a (minor) API change to fix.
Comment #128
Gábor HojtsyI don't want to hold this down since this was *just* committed, but we'll need yet another followup to add title context support as it was just added to routes, tabs and actions in #2114069: Routing / tabs / actions lack way to attach context to UI strings. Contextual links need to match that.
Comment #129
dawehnerRegarding that ... this is caused by the way how we name a single contextual link. I would prefer this change over manually working around that
by using a different plugin ID.
Let's be fair and just add the context support.
Comment #130
webchickThat's a non-trivial diff, so let's get another set of eyeballs on it.
Comment #131
dawehnerSome problems which came indirectly to my mind.
Comment #132
tim.plunkettI reviewed both interdiffs, and the changes are minimal and the unit tests are perfect.
Comment #133
Gábor HojtsyAgreed, let's get this in :) @dawehner: thanks a lot for covering context right away :)
Comment #134
tim.plunkettReroll for core.services.yml conflict with #2048223: Add $account argument to AccessCheckInterface::access() method and use the current_user service
Comment #136
tim.plunkettAh, needs account now.
Comment #138
dawehnerJust fixing the tests.
Comment #138.0
dawehnerFix typos.
Comment #139
amateescu CreditAttribution: amateescu commentedThe two interdiffs from #136 and #138 look good to me, so back to rtbc.
Comment #140
webchickIt'd be great to get this in for the next alpha release.
Comment #141
catchRead through both the issue and the patch and I can't see major issues. Thanks for adding the static cache back in #121 was looking for that ;)
Committed/pushed to 8.x, thanks!
Will need a change notice.
Comment #142
Gábor HojtsyI'm trying to adapt config translation to this new API in #2130075: Config translation should work with new contextual links API. Honestly I'm having a hard time and I think the DX of the group system is not very good. So what I figured out by trial and error is basically the contextual links system is works with groups. You either establish a new group with links to be collected by altering the build #contextual_links array to include your group or you know an existing group name and add your links to that. Eg. content translation uses existing group names per entity name.
Now for config translation, we want to add contextual links to translate things at different places, eg. Views, blocks, etc. Whenever a contextual operations list may show up. So we don't really know the group names for those routes either (and I don't know if there would be a way for us to figure out, the contextual links are not defined based on their parent routes but rather their actual routes and the grouping system holds them together). We could also define new groups but that would mean we need to alter in all kinds of build arrays, which I'm not sure how is possible generically.
At this point I think I'm probably missing something since before this all we did is we included a key in our hook_menu() and the item fell into the right place being at the right parent-child point for the path. Now that is not an option at all and figuring out existing groups and/or defining new ones with build altering seems much harder.
How would a generic module like ours do this?!
Comment #143
Gábor HojtsyBTW for local tasks the grouping is based on tab root route ID, so while it is harder to do than D7, it can be looked up based on routes. Much more predictable, not a free-to-pick group name. For contextual links seems like the interaction of the build arrays and the groups is much less predictable to extend to me. :/
Comment #144
dawehnerI cannot respond to that at the moment (sorry), though this is the same reason why views could not be ported.
Comment #145
Gábor Hojtsy@dawehner: I worked out a temporary solution for the most important ones in https://drupal.org/comment/8159639#comment-8159639, that should suffice there until we figure out how to make this work again in a general manner :)
Comment #146
thedavidmeister CreditAttribution: thedavidmeister commentedLooks like the patch here conflicts with #2102777: Allow theme_links to use routes as well as href and makes the active class handling in theme_links on the LI element inconsistent between routes and href (whereas the patch in the other issue handles that correctly).
If we could get some extra eyes over there to finish that off and help reconcile the differences, that would be great.
Comment #147
Gábor HojtsyDocumented the current process for contextual links to the best of my knowledge at https://drupal.org/node/2133283. This is NOT a change notice, it does not compare to Drupal 7, it compares to how local tasks / actions are done which are similar :)
Comment #148
Gábor Hojtsy@dawehner: you suggested on #2130075: Config translation should work with new contextual links API to at least unify the contextual link group names for config entities per entity type. (The content entities are already unified like that I believe). I opened #2134841: Contextual link group names are not predictable for that. :)
Comment #149
xjmComment #150
xjmComment #151
webchickThis got committed, so removing the alpha target tag.
Comment #152
pwolanin CreditAttribution: pwolanin commentedChange notice posted: https://drupal.org/node/2165243