Problem/Motivation
Following #2084463: Convert contextual links to a plugin system similar to local tasks/actions, contextual links cannot be attached to path parents anymore. They are collected based on group names which come from build array keys. So to attach contextual links to existing groups, you need to know this group name. However group names are somewhat arbitrary, eg. blocks have 'block', menu have 'menu', but views have 'views_ui_edit'.
Proposed resolution
Figure out how to make the names predictable to allow for automation in extending links. At least make the config entity based links use the module name.
Remaining tasks
- Figure out what to do.
- Build it.
- Get it reviewed.
- Test.
- Commit.
User interface changes
None.
API changes
Maybe. At least some existing group names will change.
Related Issues
#2084463: Convert contextual links to a plugin system similar to local tasks/actions
Comment | File | Size | Author |
---|---|---|---|
#28 | 2134841-28.patch | 1.53 KB | immaculatexavier |
#13 | 2134841_13.patch | 1.43 KB | sushilkr |
#1 | contextual-link-entity-type-based.patch | 5.76 KB | Gábor Hojtsy |
Comments
Comment #1
Gábor HojtsyLooking at the original conversion patch, looks like only views_ui and menu_test modules have a mismatch. Here is a patch for views_ui, not sure how best to fix menu_test.
Comment #2
dawehnerThe current changes look fine.
Sadly this does not solve the usecase of views itself, which had path based contextual links before.
Comment #3
Gábor HojtsyWell, its also not a full solution for config translation but at least would make some code less strange :D I'm open to a more general solution as well, but don't really know how that would look like.
Comment #4
Wim LeersShouldn't this be critical?
Comment #5
Gábor HojtsySo far I think only config translation and views are affected, so not sure... Views has a larger problem that it is not possible to add contextual links (which were possible from the views UI before), because you don't really know if there is a group matching and if none, you get route resolve errors :/
Comment #6
Wim LeersBut isn't this a critical regression?
Comment #7
Gábor HojtsyYeah it depends on whether you consider this part of the API critical :D There have been old APIs replaced with entirely new ones where direct 1-1 mapping of features are not present and then it comes down to consideration of what is important.
Comment #8
pwolanin CreditAttribution: pwolanin commentedSince the contextual links relate to the UI, it seems like the group should be "views_ui" not "view"?
e.g. if I made some other UI module (simpleviews) to edit views it shoudl have a clearly distinct group?
Comment #9
Gábor Hojtsy@pwolanin: right, well. The underlying problem is that the only way to figure out which groups are available is to inspect build arrays, which are totally not predictable. We have no idea what generates the render array for *something*. If you don't want to care about existing groups, but want to add a new one, again you need to modify that render array to add a new group. You cannot just add new routes for existing groups even unless all your route placeholders are filled in in the group.
So its multiple levels less predictable / harder to extend vs. the old path based solution :/
The entity name matching would bring some flexibility, eg. the content entities also define it that way.
As for simpleviews, it would provide a UI where the same placeholders are needed (we can assume?!) so it could put links into the same group and possibly remove other links if it wants in alter.
Comment #10
dawehnerWe could also move the contextual link groups to the entity information, so if needed someone could request them from there.
This does cover all the usecases of config translation, though not all of views.
Comment #11
Gábor HojtsySounds like views needs a path based system anyway where we don't need to know what build arrays have what groups. How can we make that work in the new system? :)
Comment #12
jhedstromComment #13
sushilkr CreditAttribution: sushilkr commentedRerolled
Comment #14
jlbellidoRemoved tags
Comment #15
alimac CreditAttribution: alimac commentedWe should all try and use the same sprint tag. According to https://groups.drupal.org/node/447258 it should be SprintWeekend2015 with no #.
Comment #16
pwolanin CreditAttribution: pwolanin commentedSo the conclusion is that the normal/default group name should be the entity type (lower case) or the plugin ID for the entity type?
Looks like the last patch is missing most of the changes from Gabor.
Comment #27
larowlanWe need a reroll from #1 here
Comment #28
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedRerolled Patch against 9.3.x for #1
Removing Reroll tag. Need test
Comment #30
smustgrave CreditAttribution: smustgrave at Mobomo commentedSeems like good house keeping ticket.
Comment #31
catchThere are various changes from #1 (by Gábor Hojtsy) not included in the patch in #28.
Comment #32
smustgrave CreditAttribution: smustgrave at Mobomo commentedLooking at the changes in #1 those files have gone through updates. Can someone confirm this is still an issue?
Comment #33
neclimdulHaven't really reviewed so not touching the status but if it has been fixed, there are some parts of core that should be fixed because they still think its an issue. Might be helpful to reviewing if this is still an issue too.
https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/conf...
https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/conf...
Comment #35
smustgrave CreditAttribution: smustgrave at Mobomo commented@neclimdul should this be moved back to NW?