Problem/Motivation
Currently, you have to implement an empty class for any local task plugin. This is a bit weird, you are then just using the empty class for the annotation. It makes sense, similar to routes, to have YAML discovery for these, then static local tasks just get added into a $module.localtasks.yml file. This seems like better UX, as people can then actually create a plugin class for dynamic local tasks.
API changes
Annotations for local tasks go away.
Caching is removed and will be handled in a follow since the strategy is being developed in parallel. (related issue below)
Related Issues
#2050227: Add local action plugin deriver to use YAML discovery for static definitions
#2032303: Cache the result of local tasks
Comment | File | Size | Author |
---|---|---|---|
#75 | drupal8.menu-system.2050919-75.patch | 25.76 KB | neclimdul |
#75 | interdiff.txt | 986 bytes | neclimdul |
#74 | interdiff.txt | 2.7 KB | neclimdul |
#74 | drupal8.menu-system.2050919-74.patch | 25.73 KB | neclimdul |
#68 | local_task-2050919-68.patch | 24.42 KB | dawehner |
Comments
Comment #1
larowlan+1
Comment #2
damiankloip CreditAttribution: damiankloip commentedHere's an initial patch for this. I have converted views local tasks and removed all the empty plugin classes.
The main thing that concerns me, is if we use a the standard derivative id pattern of "$base_plugin_id:$derivative_id" then the parent/root tab ids have to also reflect this, see the currently converted views_ui.local_tasks.yml file in the patch.
Comment #4
damiankloip CreditAttribution: damiankloip commented#2: 2050919.patch queued for re-testing.
Comment #5
tim.plunkettThis is a huge WTF. I'm not sure if the derivative system will let us work around this though?
Comment #6
damiankloip CreditAttribution: damiankloip commentedYep, that's exactly what I was saying in #2.
I guess we could implement our own discovery decorator? or just have the static plugin if as 'static' (or something), so the ids could be 'static:views_ui_list_tab' maybe?
Comment #7
dawehnerI talked with neclimdul yesterday and to be honest he wasn't really convinced that local tasks as plugins are a good idea, so we didn't made any progress on that regard.
We could either implement our own (urgs) or allow the default one to be "configured".
Comment #8
neclimdulHere's an alternative. Its a patch based on #2065571: Add YAML Plugin discovery. Its incomplete, notably missing translation and caching. This was mostly because of the @todo about caching already in the Manager which this bespoke Manager could more properly handle. Solving that @todo was a larger task though with bigger picture concerns that need to be discussed.
Comment #10
neclimduldiffed against old branch by accident.
Comment #11
tim.plunkettThis can go on the LocalTask annotation class
Comment #12
neclimdulLocalTask annotation goes away because we aren't using annotations at all. That's not how this system works.
Comment #14
neclimdulapologies to tim for my last comment. it was correct but i needed a snickers. :)
Fix most of the failures. Root test is still failing and I don't see why(
$this->drupalGet('menu-local-task-test/tasks/view');
). Also, added a test case for "dynamic" titles.Comment #15
damiankloip CreditAttribution: damiankloip commentedIMO we should really check that we can make this change instead of just the addition first.
Comment #16
dawehnerAdding an api change tag.
Comment #17
neclimdulPEBKAC
Comment #19
neclimdulI'm the worst. I uploaded the wrong patch but the right interdiff. Sorry for the noise.
Comment #20
neclimdulComment #21
damiankloip CreditAttribution: damiankloip commentedThe patch is looking pretty good but should we not be working on the dependency first? i.e. #2065571: Add YAML Plugin discovery? As that will block this anyway.
For instance, the latest patch over there does not override the provider key, as this could be set on behalf of another module.
I will ask one of the core maintainers to give their opinion on this today.
As a side note, you should not criticise the work others do so much, it's not constructive :/ The derivative pattern was added (yes, it's not ideal) to have the minimum changes to the API (just additions).
Comment #22
pwolanin CreditAttribution: pwolanin commentedSo I think the pattern used with local tasks should be the right one here also - we want a convenient YAML derivative provider, but we need to allow for modules (e.g. Views) to provide other derivatives. Or maybe I'm missing the flow.
in any case, I think this needs a re-roll since the standalone YAML Plugin discovery was committed.
Comment #23
damiankloip CreditAttribution: damiankloip commentedLocal actions?
Well, first we still need confirmation that this API change is allowed. The current patch will render all current plugins for local tasks broken. Which is why local actions currently has the derivative YAML pattern and not this one.
It is more of an issue here though, as local tasks reference other local tasks.
This is why I added a discovery decorator over in #2065571: Add YAML Plugin discovery, as a contingency plan. That got removed whilst I was asleep though.
Comment #24
damiankloip CreditAttribution: damiankloip commentedHere is a reroll anyway.
Comment #25
catchAdding YAML discovery is fine, removing annotation discovery I don't see any reason to do that.
The only reason I can think of would be if the directory scans from both YAML + annotation discovery were too expensive on cache misses but no-one's brought that up on here (or any other real reason for removing it that I can see in this issue).
Comment #26
neclimdulWe're removing annotation discovery from local tasks because we're not using it. Why would we keep it?
Comment #27
neclimdulAnother reason we'd want to lean toward yaml only that catch wanted documented is that mixing the two means we have to scan directories for annotations and a separate set of directories for yaml files. 2 scans is less good.
Comment #28
damiankloip CreditAttribution: damiankloip commentedThis reflects better what we want to do?
Comment #29
pwolanin CreditAttribution: pwolanin commented@neclimdul - whatever we do, we need to have a simple way for modules like Views or Search to add local tasks dynamically. I'm not sure how the derivative decorator interacts with this YAML discovery?
Comment #30
neclimdul@pwolanin is there not a test for this in core already? It should be handled by the derivative decorator being used by all other plugins system.
Comment #31
pwolanin CreditAttribution: pwolanin commented@neclimdul - had some discussion with EclipseGC about this to clarify my understanding - seems indeed like it should be handled, but it would be nice to demonstrate it.
Comment #32
pwolanin CreditAttribution: pwolanin commentedI'm confused as to why are you creating a new LocalTaskDefault and not just using LocalTaskBase?
also, why comment out the alter hook?
Comment #33
neclimdulre LocalTaskDefault I can't decide if it was because I was moving the static discovery from before or if its because Base is designed to be extended by all tasks and default is explicitly the default. I kinda like the way it is but if there's a case for removing it I'm open to hearing the argument.
re: todo because caching, altering, and translations are closely connected I was leaving it up to the follow up to get it right. We can't just uncomment it because we're not inheriting the DefaultPluginManager but we can put some sort of logic in if its needed.
Comment #34
pwolanin CreditAttribution: pwolanin commentedFrom IRC - you can skip calling the parent constructor and still extend DefaultPluginManager. Here's a quick re-roll to does that and removes 2 unneeded classes.
Something that seems to be missing from the committed YAML discovery is required key checking and typecasting (e.g. make sure a parameter is an array)?
Comment #35
damiankloip CreditAttribution: damiankloip commentedSince when does discovery do that?
Possibly a required keys thing could work. I'm not sure about any type casting though. Why would we even want to do that? (I don't see the point of us doing it in the other issue before). If people have not used an array when they should have, they will get an error soon enough anyway. We don't want to do too much babysitting IMO. If something gets cast into an array, that could make DX worse, i.e. harder for a Dev to debug, or thinking that not using an array is ok.
Comment #36
pwolanin CreditAttribution: pwolanin commenteddamiankloip - I'm looking at the existing code for local actions disscovery via YAML
Comment #37
damiankloip CreditAttribution: damiankloip commentedThat's not a plugin discovery class though, that is just custom code to that derivative class.
Comment #38
neclimdulre: skipping the constructor, I don't really like that. I don't know what the concern here is. Is there a problem, bug or concern?
Comment #39
neclimdulI didn't want to have this discussion quite yet but since it's come up several times on IRC as well and it is a change I'm making in the patch so I should have done it earlier. This isn't pointed at anyone I just want to lay it all out here so everyone sees it.
TL;DNR We're taking unnecessary technical debt.
First, conceptually extending the default plugin manager doesn't make sense to me. Really I think its weird that we ever do extend it because it sort of breaks our semantics(base vs default) but that is a different argument. In this case we are not using the default methods for plugin management. We have different methods for discovery addressed in this issue and different methods for caching which affects all the rest of the DefaultPluginManager code minus maybe the cache clearing method. The alterInfo function pwolanin added it back for is so trivial I even wonder what the point is.
Now the academic argument. I think we're taking on technical debt that doesn't serve us well. The DefaultPluginManager is built to be the one true core plugin implementation. The core idea is that we want the implementations extending it to be tightly coupled as a way of being able to maintain them in a single location because other managers are basically only configuring it for each implementation.
The problem is the concerns of this plugin system are fundamentally different. We've got both a different discovery system, and different cache logic and probably additional concerns. We should remove the training wheels and really write the bespoke manager implementation we need and maintain exactly that not a monkey patch of another class we're not really using.
Comment #40
neclimdulUpdated patch with some discussion from IRC.
Summary
1) Because every task isn't making a class, we're really making a default not a base implementation. So instead of removing the default remove the base and move the implementation.
2) Not actually from IRC but what I'd prefer to see happen based on my comment in #39. For now just store the moduleHandler and handle altering as part of cache follow up or as its own follow up.
Also, because it won't be clear from the interdiff, I updated the todo from the base implementation when I moved it.
I was also lazy worked on my local branch and manually applied pwolanin's changes since I was changing some of them. Because of that the interdiff is off of #19/#24 and you'll see a lot of the same changes. Sorry.
Comment #42
neclimdulFailed copying use statements...
Comment #43
neclimdulComment #44
dawehnerAfter quite some talking with chx and pwolanin we just want to use the decorator approach and reduce the amount of changes of the local task manager as much as possible.
An interdiff to #42 does not make that much sense as I started from 0. Well I copied quite some changes for the tests, so thank you for that!
Comment #45
pwolanin CreditAttribution: pwolanin commentedI think if we are backing off of using primary YAML discovery we should align this with local actions and generate derivatives based on the YAML, instead of adding this extra decorator on top?
In any case, the 2 implementations should be in alignment.
Comment #46
dawehnerLet's wait until #2076681: Add a YAML discovery decorator with tests is in.
Comment #47
neclimdulSeriously, no. You're making things more complicated at some false sense of "bigger" changes. Don't.
Comment #48
dawehnerSo you don't like that we build up an alternative chain of constructors?
Great, now we finally found a solution of which all beside one person agrees with, yeah :)
Comment #49
neclimdulwoops.
Comment #50
neclimdulMaybe I'm not understanding what you did because there wasn't an interdiff and I don't have time to compare the patches line by line tonight but there is no possible reason I can think of to move yaml to a decorator. that is by the very definition of how plugins work the job of the discover object.
Comment #51
neclimdulIt seems the real concern here is that people want to continue inheriting from the DefaultPluginManager. I do still disagree but we don't need a decorator for that. I'm out of time for the evening but I'll re-roll 2 patches so we can see what's going on. Also, since this is a concern we want to discuss lets move the cache issue forward so we can include the cache complexities here instead of the @todo.
Comment #52
pwolanin CreditAttribution: pwolanin commented@neclimdul - no, I think the real concern is that custom implementations should use annotation rather than yaml.
Comment #53
neclimdulOk, so if we want to have definitions live in 2 places, have 2 directory scans and have an extra decorator to support it I'm not going to stop this. I do think it is the wrong decision though.
Comment #54
pwolanin CreditAttribution: pwolanin commentedSo... I know it seems the direction is moving around but I think we are all struggling to see if we are making the "right" decision in terms of setting a model for other parts of core and contrib. At least *I'm* struggling.
The advantage of using annotations for custom implementations is that the meta-data is directly on the class so e.g. it could be duplicated as a unit. Is this worth the potential confusing of there being 2 ways devs might need to approach the same general task?
Using some version of yaml discovery (instead of derivates) for the plugins using the default class means we have nicer IDs and an easier DX in the 80%+ case. But I think we'll need to do the kind of checking like #11 that Tim didn't like. Does having the annotation class means that default values are filled in? I'm not sure I see how that would work. At the least, they not that they are validated, so I think we need something like this list from #11 regardless.
Based on discussion w/ msonnabaum, the right place to put the validation might be in a factory class?
Comment #55
pwolanin CreditAttribution: pwolanin commentedMaybe we can setup a hangout or some other way to have a real-time discussion?
I'm going to re-roll a patch that looks more like #34 and #42 (back to pure YAML discovery without a decorator) and add a custom factory class which is where the validation will live.
I think it's very helpful to keep the hook trivially in place by using the DefaultPluginManager since I'm guessing a module like Views would prefer to do that instead of having to set up a derivatives class via referencing it in YAML?
I think that remains an advantage of keeping annotation discovery too - that a derivative class can be located where needed and be totally self-contained?
Comment #56
pwolanin CreditAttribution: pwolanin commentedturn out it needs a serious re-roll anyhow for changes including #2032303: Cache the result of local tasks
Comment #57
pwolanin CreditAttribution: pwolanin commentedHere's a new one that adds a LocalTaskFactory, and translates the title (which was missing before). The title translation is actually a good reason perhaps to stick with one discovery - in the annotated version it was already translated, so we'd have a mix of translated and untranslated strings.
Note - I created the patch with git diff --no-renames, so it can be applied using patch as well.
Comment #58
pwolanin CreditAttribution: pwolanin commentedHere's the same patch with the rename.
Comment #59
pwolanin CreditAttribution: pwolanin commentedSo, further discussion in IRC with msonnabaum and others, suggests that setting defaults would ideally be in the YAML discovery since the annotation-based discovery already sets defaults and it would be good for them to work in parallel.
However, the rest of core already uses the $defaults array from the PluginManagerBase. So... let's go back to that and then we don't need the custom factory right now.
So, this ends up back very close to neclimdul's patch in #42 except that it continues to extend DefaultPluginManager so we don't have to re-implement the alter hook and caching, validates that a route name is present, and translates the plugin title inside the getTitle() method.
This one has the rename in the diff, for easier comparison to neclimdul's
Comment #61
tim.plunkettWhy not just call parent?
This isn't a problem now, but if it becomes a multidimensional array, this will stop working.
$this->t->translate will not work, we need $this->t(). That was decided already, see #2019679: Support for Drupal 8 $this->t()
Wouldn't this need $this->t()?
Comment #62
pwolanin CreditAttribution: pwolanin commented@tim.plunkett I think this->t() vs this->t->translate() doesn't matter since I think the original test will need to be parsed out of the YAML?
It would only matter in the case of fixed strings in the class, but I guess we should provide a $this->t() for that use case?
Patch adds that method and fixes the test fail, and addresses the other comments.
changes:
Comment #63
tim.plunkettThis is the wrong interface, it should be
Drupal\Core\StringTranslation\TranslationInterface;
Per #2018411: Figure out a nice DX when working with injected translation, this should be
Comment #64
pwolanin CreditAttribution: pwolanin commentedThanks. Fixed those.
Also: #2079095: Shorten the doxygen for method t() in ControllerBase to the standard version
Comment #65
dawehnerI think this is fine from my perspective!
Comment #66
damiankloip CreditAttribution: damiankloip commentedSorry to change status, but I'm not sure why array_values() is being used here?! It doesn't really make sense. You're definition will look like:
The local task definitions are not getting tested, only in unit tests that mock everything, so this wouldn't get picked up. This could be a follow up? I don't mind.
Comment #67
damiankloip CreditAttribution: damiankloip commentedComment #68
dawehnerAdded a test to ensure the provider key.
Comment #69
pwolanin CreditAttribution: pwolanin commented@damiankloip - thanks for catching that bug. I think with the added test coverage this is solid.
Comment #70
damiankloip CreditAttribution: damiankloip commentedWas there some other reason array_values() was used in the first place (to fix anything else)? Seems like we have most things covered now though.
Comment #71
pwolanin CreditAttribution: pwolanin commentedIt was in the 1st patch by neclimdul in #8, so I think it was just a mistake or oversight that no one noticed and keep re-using.
Comment #72
damiankloip CreditAttribution: damiankloip commentedOK, great. Looks good then!
Comment #73
neclimdulFound two things that are missing. patch in a few.
Comment #74
neclimdulPeter caught me and corrected the only big concern I had. Just some doc and use cleanups. Added back the defaults that went missing so they are documented.
Comment #75
neclimdul"can you improve the doc name for route? i.e. that it's required and is used to generate the link for the tab?" - pwolanin
Comment #76
pwolanin CreditAttribution: pwolanin commentedthanks for the extra docs
Comment #77
pwolanin CreditAttribution: pwolanin commented#75: drupal8.menu-system.2050919-75.patch queued for re-testing.
Comment #78
dawehnerGiven that this blocks #2076283: Allow local task plugins on paths with a dynamic value (like a node) and #2045267: LocalTaskBase and LocalTaskInterface has to work with routes containing parameters, which itself blocks every advanced conversion like node/{node}/edit and field UI this feels critical, sorry.
Comment #79
pwolanin CreditAttribution: pwolanin commented#75: drupal8.menu-system.2050919-75.patch queued for re-testing.
Comment #80
catchLooks great. Committed/pushed to 8.x.
Will need a change notice.
Comment #81
pwolanin CreditAttribution: pwolanin commentedAdded a short new change notice at : https://drupal.org/node/2085285
and updated the original one at: https://drupal.org/node/2044515
Comment #83
plachRelated issue: #2106349: Comment translation overview has broken local tasks. I need help!
Comment #83.0
plachUpdate to clarify new direction of issue.