Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Branching off from #1981644: Figure out how to deal with 'title/title callback'
Local actions in Drupal 8 that are based on routes cannot handle dynamic titles or paths (e.g. an action link with the current node title or path)
The proposed solution is to convert them to plugins which have getTitle and getPath methods.
Comment | File | Size | Author |
---|---|---|---|
#28 | menu-local-action-2031473-28.patch | 22.15 KB | tim.plunkett |
#28 | interdiff.txt | 22.5 KB | tim.plunkett |
#23 | local-actions-2031473-23.patch | 21.68 KB | dawehner |
#23 | interdiff.txt | 998 bytes | dawehner |
#22 | local-actions-2031473-22.patch | 21.46 KB | tim.plunkett |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedHere's the patch from #52 at #1981644: Figure out how to deal with 'title/title callback', which should be the last local action patch that was posted there.
Comment #2
pwolanin CreditAttribution: pwolanin commentedWith doxygen cleanups and minor changes suggested in the review & IRC by dawehner
Comment #3
dawehnerAn array of route names...
empty line
let's remove that, we seem to not do that anymore.
empty lines between the two.
Needs some docs
Let's try to fill up until 80 chars.
empty line between of them.
Typo in "Implmenetations"
What about making a new comment block, and document out. So it looks like "*/ \n // public function getTitle"
Let's try to fill up the 80 chars.
Missing starting "\"
+1 for document that kind of stuff
+1 for throw an exception. it is just a better DX.
Comment #4
pwolanin CreditAttribution: pwolanin commentedThanks @dawehner. Here's a revised one with doc fixes.
Comment #6
tim.plunkettDiscussion ongoing about the interface and the thrown exception, but here's a nitpick.
Gets
Typo, but I think we agreed to remove this comment?
Gets, Finds, etc
Needs a sentence
Missing a blank line, here and a couple places
Can we just call this @MenuLocalAction?
Typo
Comment #7
pwolanin CreditAttribution: pwolanin commentedFixes per Tim's suggestions
Comment #8
pwolanin CreditAttribution: pwolanin commentedAdds methods in the interface - apparently adding optional params works with both the interface and the ControllerResolver.
Comment #9
tim.plunkettThis is the best possible outcome I think we'll see. This will suit all contrib needs.
#famouslastwords
Comment #10
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #11
fubhy CreditAttribution: fubhy commentedJust some small fixes and some remarks (@see below).
I'd even go as far as just naming that "LocalAction". Seems verbose enough for me, especially since the annotation discovery is in /Menu for this. I did not touch that though.
Either we have an interface, or check for is_callable(). Doing both does not make sense. I would vote for not having an interface as I find it rather weird to pretend that the arguments of these methods are optional. They are not (depending the local action plugin). And yes, they are different. So we got the controller pattern right there -> no interface (imo).
Yeah. We should try to compile this into each route so that every route knows which local actions appear on it.
Comment #12
dawehner/me sighs ... Thanks for removing a solution we already found.
Comment #13
fubhy CreditAttribution: fubhy commentedIt's not a solution. We are treating these methods as controllers and we are calling them through controller resolver. We don't have interfaces for other controllers. Why do we want one for this? And if we do want one for this then why are we checking for is_callable() in the plugin manager? Having both doesn't make sense. And having an interface that does not have any arguments on a method and then explicitly saying "oh, but you should definitely add your own arguments there, but make them optional" is totally weird. It's a lie to say that those arguments are optional or have a default value. If your plugin depends on them, it depends on them.
Comment #14
dawehnerShould have a "\"
Comment #15
fubhy CreditAttribution: fubhy commentedOne last thing before I shut up: Why are we putting this in system module instead of Drupal\Core? I feel it really belongs in Drupal\Core instead.
Comment #16
tim.plunkettPlease drop the exceptions and go back to interfaces. We already spent several hours hashing this out...
Comment #17
fubhy CreditAttribution: fubhy commentedWhy would we want to introduce a new pattern here? It's a total lie to have the implements go with $foo = NULL even though they actually require $foo. It's impossible to provide interfaces for controller-like methods as they are dynamic. Period. The interface is a contract. If the implementations can't live up to that promise then that's code-smell. Hence, we can't have an interface implementation for these methods. It makes no sense.
Comment #18
tim.plunkettWe already do this in FormInterface in dozens, eventually hundreds of places.
Comment #19
fubhy CreditAttribution: fubhy commentedOkay, we agreed on IRC that we will do the interface/no interface thing in a follow-up. Daniel is going to come back with a re-roll (I gotta run in a few, office closes).
Comment #20
dawehnerThis just reverts the interface bits.
Comment #21
fubhy CreditAttribution: fubhy commentedBack to RTBC then
Comment #22
tim.plunkett#1978952: Convert shortcut_set_add to a Controller went in, so rerolling for that one.
My only worry here is that everywhere else, each plugin type gets its own subdirectory. But both this and #2004334: Separate Tabs (MENU_LOCAL_TASK) from hook_menu() use \Drupal\$provider\Plugin\Menu, not \Drupal\$provider\Plugin\MenuLocalAction and \Drupal\$provider\Plugin\MenuLocalTask.
Is that a problem? Have we tried combining the two patches?
Comment #23
dawehnerThanks for the reroll.
To avoid confusion we should really switch to a different directory (I went with MenuLocalAction for now)
Comment #24
fubhy CreditAttribution: fubhy commentedNow that we are moving it anyways, can't we just move it to Drupal\Core?
Comment #25
pwolanin CreditAttribution: pwolanin commentedGiven that we have Drupal\system\Annotation\MenuLocalAction, I think MenuLocalAction is sensible as the directory name.
Comment #26
tstoecklerWe could also do Menu\LocalAction and Menu\LocalTask
Comment #27
fubhy CreditAttribution: fubhy commentedOr just leave the "Menu" part out entirely. ;) Bikeshed inc... But please let's not push this back further. Only thing I want to know is whether it's okay to move this to Drupal\Core after committing this version here that has been RTBCed already.
Comment #28
tim.plunkettAfter discussion with Eclipse, fubhy, and alexpott in IRC, we decided to do the namespace move now.
I also, per our discussion, added an alter.
Comment #29
tim.plunkettugh, the second set is rebased on HEAD. I meant to delete the first set. I'll cancel that test...
Comment #30
dawehnerGood try with this interdiff ...
+1
Comment #31
pwolanin CreditAttribution: pwolanin commentedI'm happy with the namespace change here.
Comment #32
Dries CreditAttribution: Dries commentedDespite the empty classes, this makes sense. Committed to 8.x. Thanks!
Comment #33
xjmNeeds a change notice.
Comment #34
catchNot sure this should've gone in with this particular @todo. Was this profiled? Is there a follow-up issue to add that compiling caching?
Comment #35
tim.plunkettIt used to say
// @todo Consider storing the results of hook_local_actions() in a static.
I think this was just an updated version of that pre-existing @todo.
Comment #36
catchThat's swapping a hook invocation to a cache get though, is a problem for any info hook -> plugin conversion though I guess.
Comment #37
pwolanin CreditAttribution: pwolanin commentedSince the final version is extending DefaultPluginManager, getDefinitions() is already cached - I think that changed in #1903346: Establish a new DefaultPluginManager to encapsulate best practices
So we probably just need to remove that @todo
Comment #38
pwolanin CreditAttribution: pwolanin commentedActually - instead of that @todo, we need a follow-up issue to flag routes that have a local action (or local task) appear when we do the router rebuild so that we have an easy way to skip calling this code when it's a no-op.
Comment #39
fubhy CreditAttribution: fubhy commentedI don't think flagging the routes is going to work as that basically requires re-running the route builder entirely every time you configure a local action/task (for example in views). I don't think we want that. Clearing the cached task/action definitions is much simpler. And with cached definitions we won't run into performance problems either. Yes, let's build set's on a per-route basis so we don't have to build up the list on run-time, but let's not write that into the route definitions.
Comment #40
pwolanin CreditAttribution: pwolanin commentedyes, maybe that's what we need - to cache via the manager the actions/tabs that appear per route to avoid having to iterate to find them. I agree flagging the routes might not be the right option.
Comment #41
catchCaching per route sounds good to me.
Comment #42
pwolanin CreditAttribution: pwolanin commentedupdated the change notice at https://drupal.org/node/2007444
Comment #43
pwolanin CreditAttribution: pwolanin commentedComment #44
pwolanin CreditAttribution: pwolanin commentedfollow-up for caching #2046565: Cache the local action plugins that appear per route