Updated: Comment 0
Problem/Motivation
Currently you can't use local tasks with dynamic values, like node/{}/value without extending the base plugin class. This is kind of problematic
for things like the config translation module or just the field UI
Additional like on the link generator issue, the access checking is currently doing way too much in order to check access.
Proposed resolution
Instead of generating the path the plugins should return just the route name and parameters. These parameters could be pulled from either the current page
or from the route definitions (yml probably in most cases).
If we have the route name/parameters checking access is way easier.
Remaining tasks
User interface changes
API changes
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#52 | duplicate_variables.patch | 721 bytes | tstoeckler |
#47 | drupal8.routing-system.2076283-47.patch | 37.66 KB | neclimdul |
#43 | drupal8.routing-system.2076283-42.patch | 37.67 KB | neclimdul |
#43 | interdiff.txt | 1.21 KB | neclimdul |
#42 | local_task-2076283-42.patch | 37.67 KB | dawehner |
Comments
Comment #1
dawehnerLet's see what the testbot thinks about that.
Comment #3
dawehner.
Comment #5
dawehnerReworked the patch to now use checkNamedRoute etc.
Comment #7
dawehnerRerolled.
Comment #9
dawehnerYet another rerole error.
Comment #10
dawehnerSome fixes here and there.
Comment #11
dawehnerLet's wait until #2050919: Replace local task plugin discovery with YamlDiscovery is in
Comment #12
pwolanin CreditAttribution: pwolanin commentedHere's a new version that takes the route parameters from the Request.
Comment #14
pwolanin CreditAttribution: pwolanin commentedA couple of those look like sporadic fails (don't et a fail locally for PageNotFoundTest) and the LocalTasksTest is actually looking for the active class on the wrong element - the A instead of the LI.
So, this has been a bogus test for a while. however, the change to the link generator should not have affected that.
This patch adds comment local tasks - though really we want the YAML implementation in so we don't have to re-convert them.
Bug opened here: #2084125: Bug in active detection in LinkGenerator - need to extract array from _raw_variables ParameterBag
Comment #15
dawehnerMerged in many of my ideas from the other issue: #2045267: LocalTaskBase and LocalTaskInterface has to work with routes containing parameters and fixed some types etc.
Comment #16
pwolanin CreditAttribution: pwolanin commentedI'm not sure this is correct or needed?
The ParamConverterManager puts the raw values of all the route variables into _raw_variables regardless of whether or not they are later upcast.
Comment #17
dawehnerWell, I personally believe in decoupling of code, so from my perspective it would be cool to not couple the link generator onto the way our the paramconverterManager does stuff,
at least support the normal symfony way of doing it.
Comment #18
pwolanin CreditAttribution: pwolanin commentedImprove code comments including an actual route and less ambiguous example
Comment #19
dawehner+1
Comment #21
dawehner#19: local_task-2076283-19.patch queued for re-testing.
Comment #23
dawehnerNot really surprising to have failures if you have local file missing to the patch.
Comment #24
pwolanin CreditAttribution: pwolanin commentedFixes local fails for me by removing the weights on the test plugins (by default the root one gets -10)
Also, remove unused use statements.
Comment #26
pwolanin CreditAttribution: pwolanin commented#24: local_task-2076283-24.patch queued for re-testing.
Comment #27
pwolanin CreditAttribution: pwolanin commentedNeeds to be re-rolled for #2050919: Replace local task plugin discovery with YamlDiscovery
Comment #28
pwolanin CreditAttribution: pwolanin commentedissue tag weirdness...
Comment #29
pwolanin CreditAttribution: pwolanin commentedand more such
Comment #30
pwolanin CreditAttribution: pwolanin commentedworking on the re-roll now...
Comment #31
pwolanin CreditAttribution: pwolanin commentedSince files are renamed, etc I don't think an interdiff is meaningful here. BAsically the same as #24 with conversion to YAML files.
Comment #32
dawehnerThis should be using RouteObjectInterface::ROUTE_NAME
Let's put some @inheritdoc on there.
I just realized that #2076283-18: Allow local task plugins on paths with a dynamic value (like a node) dropped the unit tests again. Let's fix that.
Comment #34
dawehnerThis is it.
Comment #35
dawehner#34: local_task-2076283-34.patch queued for re-testing.
Comment #36
dawehnerAdded a usecase mentioned by neclimdul: Define the route parameters in the plugin definition.
Comment #37
neclimdulloving this. Think there might be a bug in trailing parameters though. Working on a test case.
Comment #38
neclimdulRerolled after routing key rename and with tests for the bug I'd found and a fix from dawehner. Will need another reroll shortly for the pattern->path change but want get this up and out of the way.
Comment #39
dawehnerNitpick alarm: There should be an empty line + @file
Comment #40
neclimdulreroll after #2053489: Standardize on \Drupal throughout core and space fix.
Comment #42
dawehnerThis should be it.
Comment #43
neclimduloh right, and pattern change.
Comment #44
neclimdulComment #45
pwolanin CreditAttribution: pwolanin commentedGreat work with the added test coverage and use cases.
Comment #46
tim.plunkettThis is the only part of the patch I thought was questionable, but it isn't added here, just moved. +1 for RTBC
Comment #47
neclimduljust a reroll to fix a conflict. no changes.
Comment #48
catchYes this looks good. Committed/pushed to 8.x.
Will need a change notice.
Comment #49
pwolanin CreditAttribution: pwolanin commentedHere's a short change notice: https://drupal.org/node/2094027
Comment #51
tstoecklerRe-opening this for the following problem:
This is overriding the $level variable that is being passed into the function. I'm not 100% through with debugging, but I think this might be the cause of some missing tabs in config_translation that I'm having a problem with.
Comment #52
tstoecklerHere we go. The tab funkiness I experienced was in fact not related to this at all. I still think this should be fixed, though. I'm fine with moving this into a follow-up, but I wanted to get some attention from some people who understand what's going on here, first.
Comment #53
pwolanin CreditAttribution: pwolanin commentedLet's move this to a follow-up, since this issue was marked closed/fixed.
Looks indeed like ti would be better not to whack the variable.
Comment #54
dawehnerOpened a follow up as it is simple to do that.
#2106967: menu_local_tasks $level gets overridden