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.
Instead of using hook_menu() views should use the local tasks plugins provided by #2004334: Separate Tabs (MENU_LOCAL_TASK) from hook_menu()
Comment | File | Size | Author |
---|---|---|---|
#44 | vdc-2032309.patch | 33.69 KB | dawehner |
#44 | interdiff.txt | 1.42 KB | dawehner |
#39 | interdiff.txt | 12.62 KB | dawehner |
#39 | vdc-2032309.patch | 33.76 KB | dawehner |
#37 | vdc-2032309.patch | 27.55 KB | dawehner |
Comments
Comment #1
dawehnerStarted some basic work.
Comment #2
Gábor HojtsyI'm really interested by this because #2044737: Provide local tasks as plugins needs a *very* similar solution and that seems to be required at this point to get into core. So any core changes needed to make this work would be good to get in ASAP. Looks like the core change proposed here is more like code reorganization to achieve loading all routes at once instead of one-by-one, so not required to make this work, so we may be safe there.
Looking at how would we provide the annotations from the derivative though.
Comment #4
dawehnerThere is an extra issue regarding the multiple-load: #2032311: Load all routes for local tasks via getRoutesByNames()
Comment #5
Gábor HojtsyLooks like it should not be included here then? :)
Comment #6
dawehnerOkay made some progress, so you could potentially look somehow how it will work, but to be honest, as always, the views example is way too complex.
Some problems I ran into:
Comment #7
dawehnerSome work on it, though still not near to be reviewed.
Comment #8
dawehnerLuckily views comes with an existing webtest for local tasks, so it was just a matter of waiting for them to run and write some unit tests in the meantime.
Comment #9
dawehnerA couple small nitpicks here and there.
Comment #11
Berdir$parent_task is an array here I think, that does not end well.
Also, getting a real mess with local tasks. Somehow view.files.page_1 ends up as a local task of my page (Where I'm trying to add local tasks to a view), which as added benefit need arguments, but that other view is not providing those arguments, so it blows up.
Comment #12
dawehnerOkay, this is a clear sign that the method is named bad. Fixed that.
Here is a bit more work which let's the three tabs (content/files/comments appear again). The general problem is that the local task system is 100% incompatible with the old,
so if something is converted all of the other tasks have to be converted as well.
It would be cool if you could just point me to the repo where TMGMT D8 is developed, so I will have a look.
Comment #14
dawehnerThis fixes at least the comment tests.
The other one seemed to be a random failure.
Comment #15
BerdirDebug left over.
What do you need a class for if it doesn't do anything? :)
Is this also necessary for the contextual links @todo below?
Comment #16
dawehnerYou are totally right. This is kind of a leftover when we did not used yml files for discovery.
It is a bit hard to untackle the code in the current state. What do you think about moving the logic to the same place as the contextual links at the moment?
Comment #18
damiankloip CreditAttribution: damiankloip commentedI guess you want this to go too?
Should they all move onto their own line? this looks weird.
:)
Are we sure we will always have this state set? So the route processing will be first.
//
Where does this marry up to? (ignorance with local tasks)
Should return NULL if there is no string?
We can use the Views:: alternative now.
Comment #19
dawehnerJust approved the weirdness on the unapproved comments.
This seems to be a bug in my editor.
If we don't rebuild the routes we are fucked anyway, as routes might not exist at all yet, but I don't see that as a problem at all.
Local tasks are just requested when you render the page. Before that you always would have to find out the active route, which requires the route system to be rebuild or active.
Thank you for the review.
Well the full idea behind that line is to make it nice for static checkers and IDEs. Technical a single line comment is always ignored, so for example annotations also don't work on them.
All the local tasks with the same tab_root_id appear at the same time. So if you are at node/{node} the local task manager finds the route 'node.view' and finds all local tasks
which are in the same "tab-group" with the current route.
I don't give a shit about it.
Yeah, sadly this does not help with unit tests at all.
Comment #21
dawehnerJust moved back how executeHookMenu worked when the test passed.
Comment #23
dawehnerDoh!
Comment #24
damiankloip CreditAttribution: damiankloip commentedSets
Needs a docblock
Both small, sorry. Apart from that, all the other points i had before are sorted.
Comment #25
dawehnerThere we go.
Comment #26
damiankloip CreditAttribution: damiankloip commentedComment #27
dawehnerEven it is already perfect, this can be perfecter!
Comment #28
damiankloip CreditAttribution: damiankloip commentedNice clean up!
RTBC++
Comment #29
Berdir27: localtasks-2032309-27.patch queued for re-testing.
Comment #30
BerdirWondering, can we clean this up further now that the contextual links etc. stuff went in? Might also require a re-roll now?
Comment #32
dawehnerNot in this issue, please!
Comment #33
BerdirOk, then let's get this in ASAP :)
Comment #34
amateescu CreditAttribution: amateescu commentedI was working on #2111823: Convert field_ui / Entity local tasks to YAML definitions which has the same
getPluginIdFromRoute()
method and made some improvements to the doxygen block which I'd like to bring here as well.Comment #35
Berdir34: vdc-2032309-34.patch queued for re-testing.
Comment #37
dawehnerReroll.
Comment #38
tim.plunkettAwesome tests!
I think comment_count_unpublished() should be moved to CommentManager or something... But if not, at least delete the old function.
Field UI has this method, and content/config_translation have the same thing but called getTaskFromRoute(). Can we move this to a base class? Or at least a @todo.
I think I had a dedicated issue for this (I might be wrong) but let's close that if so.
Comment #39
dawehnerLet's keep comment_count_unpublished() as this is really out of scope of this issue.
Let's do it.
Comment #40
tim.plunkettWonderful!
Comment #41
dawehnerNo way is that normal.
Comment #42
dawehneraka.
Comment #43
catchCouldn't this return $plugin_id directly if found instead of assigning to null + break etc. The function will return null if nothing is found anyway.
Missing full stop.
eww. Nice to see all of these go.
Is this a new coding standard?
Doesn't hook_local_tasks_alter() run every request? This looks potentially very expensive if so.
Could this be stored with the view somehow instead?
Why the helper for this?
Comment #44
dawehnerSure, this works too. The patch was just moving a bit of code around, but it is fine, if we can improve it at the same time.
There are various places in core where this is done already to get support for static tools as well as IDEs.
There are two hooks at the moment:
So this is not a problem, hey!
I try to figure out what you meant with that. The idea here is to get the configured path from the user, which is stored in the display. There is also $executable->getPath() which returns the same in this case, though also contains runtime overrides, which means that it is not really semantical, what we want to do.
Well, let's allow the code to speak here: This is not a simple functionality behind there.
Comment #45
tim.plunkettI think that adequately addresses catch's concerns.
Comment #46
webchickLet's find out! :)
Comment #47
catchWould be good to add it to http://drupal.org/coding-standards.
The hook that's implemented is hook_menu_local_tasks_alter(), so I think it is a proble? If it's expensive/unnecessary to run that every request, which it looks like it is.
I didn't mean getApplicableViews() itself, I meant the one line wrapper around getApplicableViews() that just saves passing the argument to it.
Comment #48
tim.plunkett@dawehner misspoke, the two hooks are hook_local_tasks_alter (for plugins) and hook_menu_local_tasks_alter (for runtime). views_local_tasks_alter is the plugin one. Field UI, Config Translation, and Content Translation use exactly this pattern.
getApplicableMenuViews() is there for mocking, since we can't swap out the Views:: call. This is like any of our wrapper methods for services.
Comment #49
catchCommitted/pushed to 8.x, thanks!
Comment #50
andypostRelated issue for comment module #2111357: Get rid of comment_count_unpublished() in favor of CommentStorage method