Updated: Comment ...
Problem/Motivation
Building on
#2065571: Add YAML Plugin discovery
and
#2050919: Replace local task plugin discovery with YamlDiscovery
Local actions are currently using YAMl derivatives on top of annotation-based discovery.
however, the 90%+ use case would be satisfied by the YAMl, and I think it is more DX friendly to just let devs specify the plugin class or derivative class in the YAML in the < 10% case where they need to do something special.
Remaining tasks
User interface changes
API changes
Related Issues
#2091145: Adapt $module.foo pattern for local action plugin IDs
Original report by @pwolanin
Comment | File | Size | Author |
---|---|---|---|
#69 | interdiff.txt | 4.64 KB | dawehner |
#69 | local_actions-2077473-69.patch | 39.76 KB | dawehner |
#64 | local_actions-2077473-64.patch | 36.66 KB | dawehner |
#64 | interdiff.txt | 595 bytes | dawehner |
#60 | 2077473-60.patch | 36.66 KB | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedThis patch essentially combines work from #2050919: Replace local task plugin discovery with YamlDiscovery and #2076283: Allow local task plugins on paths with a dynamic value (like a node)
Comment #3
dawehnerReally nice stuff.
Some of the review points fixed in the patch.
+1 for moving the logic into the local action manager.
Ha, I nearly thought you really did not copied anything by hand :)
Nice!
Comment #5
pwolanin CreditAttribution: pwolanin commented@dawehner - I just lost track of #2046565: Cache the local action plugins that appear per route, though I noticed the @todo in the code. This patch reorganized that method a bit, so not sure if we should combine them some how.
Comment #6
dawehnerComments regarding the fixes:
The url /add got removed, though menu_get_item() always fetches also some parent. This leads the local action to appear on 403/404 pages, so the test still worked
Comment #7
dawehnerSo many new files are missing. (updating a lost of the different patches). (never touch your gitignore file)
Comment #9
pwolanin CreditAttribution: pwolanin commented#7: local_actions-2077473-7.patch queued for re-testing.
Comment #11
pwolanin CreditAttribution: pwolanin commented#7: local_actions-2077473-7.patch queued for re-testing.
Comment #13
dawehnerThere we go.
Comment #15
dawehnerHa, made a copy error.
Comment #16
disasm CreditAttribution: disasm commentedComment #17
dawehnerJust a rerole.
Comment #18
pwolanin CreditAttribution: pwolanin commentedSome cleanup of code comments and clarifies the check in the theme function.
Comment #20
dawehner+1 Just taking the chance to add a tag.
Comment #21
dawehnerJust another rerole on top of #18 and one more little change.
Comment #23
dawehnerThis should be it.
Comment #25
dawehnerCustom_block already fixed what was needed for this patch to work.
Comment #27
pwolanin CreditAttribution: pwolanin commentedthe patch should include a conversion
tim points to menu_link_add goes on menu_menu_edit
Comment #28
dawehnerI am glad that this is already part of the patch.
... let's see how much this fixes.
Comment #30
dawehnerInteresting, there are local actions which neither have a real href, nor a route, but just a title/text.
Comment #32
pwolanin CreditAttribution: pwolanin commentedIs that logic right? Are we setting an empty string by default?
Comment #33
dawehnerYes we do:
Let's just go with this.
Comment #34
pwolanin CreditAttribution: pwolanin commentedIn link with the last LocalTask patch, let's rename LocalActionBase to LocalActionDefault?
Comment #35
dawehnerIs there a clear reason behind why that was done?
Comment #36
pwolanin CreditAttribution: pwolanin commented@dawehner - it's being used as the default. It's not being extended by any other class typically. In contrast, FormBase or ControlerBase classes are never used themselves - they are abstract.
Comment #37
dawehnerAll the rerole due to the route_name changed did not allow me to provide an interdiff.
This fixes especially the point made in #34
Comment #39
dawehnerdoh!
Comment #41
damiankloip CreditAttribution: damiankloip commentedThis should fix those test failures, I think it's just a route naming issue.
Plus:
This could prob be updated now, it's acting more of a default implementation really. EDIT: Hmm, maybe not!
Really?
'The string translation object'
Prob make both of these "...FOR the local task"
plugin.s
Seems like this default should be NULL?
I don't see why we need a ternary here? The logic is: if there are route names, get all the routes by names? So basically, this would be better (and maybe more readable) as
if ($route_names) { $this->routeProvider->getRoutesByName() }
?Comment #42
damiankloip CreditAttribution: damiankloip commentedComment #43
neclimdul"... for the local action."
Comment #44
damiankloip CreditAttribution: damiankloip commentedHa, that's what I said ;)
Comment #45
dawehnerI don't care, as we will throw some exception anyway in the future.
Good points, ... here are some other typos as well.
Comment #47
damiankloip CreditAttribution: damiankloip commentedI think we still need to wrap the call to getRoutesByNames() in an if, so we don't pass nothing in there. I made a couple of other changes, as well as removeing the getPath() method from the interface (we only need route names now) and implementing getWeight() in the default class and in the manager where the local action links are built.
Comment #48
dawehnerSomeone should fill a novice task to replace '' to NULL in the LocalTaskManager.
Comment #49
pwolanin CreditAttribution: pwolanin commentedProbably weight should be NULL by default here also like for local tasks?
Comment #50
damiankloip CreditAttribution: damiankloip commentedI don't get that, 0 is a sensible default for a weight. It should always be an int IMO. id and route_name are required keys, weight isn't.
Comment #51
dawehnerSo damian, the reason why it is NULL on local tasks is that we can apply some logic. If it is NULL but the default local task, change it to -10, otherwise set it to 0.
If it is already 0 you cannot know whether this was intended.
For local actions it doesn't really make sense to not have a 0 as default given that there are no default local actions.
Comment #52
pwolanin CreditAttribution: pwolanin commentedI think NULL could still be useful for the same reason (to distinguish those plugins where it's set in the base definition vs. not set at all), even though we don't have the concept of default. In drupal_render() the weight not being set makes the item sort as 0, but it's never filled in e.g. for form arrays.
Comment #53
damiankloip CreditAttribution: damiankloip commentedOK, I don't really mind either way, let's just change it to NULL and be on our way.
Comment #54
dawehnerWhatever works, back to RTBC
Comment #56
damiankloip CreditAttribution: damiankloip commentedoops, didn't pull/rebase!
Comment #57
neclimdulComment #58
pwolanin CreditAttribution: pwolanin commentedas a result of #2087231: Add a PluginBase in the Core namespace with t() as a helper method, we don't need to add t() to the default class.
This is a very minor change, o still should be RTBC unless the tests fail.
Comment #60
pwolanin CreditAttribution: pwolanin commentedoops, I was sure I deleted that line in create()... maybe I didn't save before rolling the patch.
Comment #62
pwolanin CreditAttribution: pwolanin commented#60: 2077473-60.patch queued for re-testing.
Comment #64
dawehnerThis should be it.
Comment #65
pwolanin CreditAttribution: pwolanin commentedAh, yes, good catch. Core not Component. back to RTBC.
Comment #66
pwolanin CreditAttribution: pwolanin commented#64: local_actions-2077473-64.patch queued for re-testing.
Comment #67
dawehnerGiven that this is needed to remove the menu_router we should mark it as critical.
Comment #68
alexpottShouldn't this be deleted?
Shouldn't this be deleted?
Shouldn't this be deleted?
We need to test overriding this given that we don't appear to have any usages in core.
Comment #69
dawehnerGood points!
Comment #70
pwolanin CreditAttribution: pwolanin commentedLooks good with the added test.
Comment #71
pwolanin CreditAttribution: pwolanin commented[posted to wrong issue]
Comment #72
pwolanin CreditAttribution: pwolanin commentedComment #73
alexpottCommitted 0fc52f1 and pushed to 8.x. Thanks!
Fixed a couple of minor nits during commit.
Comment #74
pwolanin CreditAttribution: pwolanin commentedfirst pass at a change notice: https://drupal.org/node/2102583
Also updated: https://drupal.org/node/2007444
Comment #75
pwolanin CreditAttribution: pwolanin commentedmarkign fixed given the lack of any feedback on the change notices.
Comment #76
dawehnerThese changes are looking great. I do like that the central change notice is updated and the other one just link to it.
Comment #77
pwolanin CreditAttribution: pwolanin commentedbad tag
Comment #78
xjmComment #78.0
xjmadded issue summary.