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.
Updated: Comment 13
Problem/Motivation
We have a basic consensus on moving menu links to be plugins.
default (module-provided) menu links should use YAML discover link local tasks and local actions. We want to avoid too many more changes to the DX, so let's get them into YAML files even before the plugin conversion.
Proposed resolution
USe a simple YAML discovery mechanism so the format of declaring module default menu links doesn't change, even if we radically change the underlying API.
Remaining tasks
- Commit
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#35 | interdiff.txt | 1.29 KB | tvn |
#35 | menu-2226903-35.patch | 98.23 KB | tvn |
#32 | menu-2226903-32.patch | 98.22 KB | martin107 |
#32 | interdiff-31.32.txt | 2.62 KB | martin107 |
#30 | menu-2226903-30.patch | 98.21 KB | pwolanin |
Comments
Comment #1
dawehnerFirst bit.
Comment #3
webchickSince this very blatantly affects a very module developer-facing API, tagging as beta blocker. Thanks so much for taking this on!
Comment #4
dawehnerThis could be it.
Comment #6
xjmComment #7
dawehnerA couple of process
Comment #9
pwolanin CreditAttribution: pwolanin commentedre-rollling and looking at test fails
Comment #10
pwolanin CreditAttribution: pwolanin commented1 line test fix was all, plus some minor cleanup.
Comment #11
pwolanin CreditAttribution: pwolanin commentedNR
Comment #12
pwolanin CreditAttribution: pwolanin commentedbah, wrong patch
Comment #13
pwolanin CreditAttribution: pwolanin commentedComment #16
jibranGreat work we only have two fails now.
Comment #17
dawehnerThis should be it.
Comment #18
pwolanin CreditAttribution: pwolanin commentedLooking good.
2 questions:
sure we use "title" instead of "link_title". I'm not sure if consistency with the local tasks and actions is good or confuding?
Also, we should probably support defining an options array in the YAML
Comment #19
jessebeach CreditAttribution: jessebeach commentedI prefer consistency. Let's please use
title
. I've updated the patch to the following pattern in all the YAML files.And yes, we do need options support to do stuff like this:
Question: is there a way in YAML to write the identifier
data_some_thing
asdata-some-thing
so that we don't need to convert underscores to dashes for the attribute name?Comment #21
pwolanin CreditAttribution: pwolanin commentedworking on fixing this - missed the alter hooks and Views
Comment #22
dawehnerHere is a change notice: https://drupal.org/node/2228089
Comment #23
pwolanin CreditAttribution: pwolanin commentedJesse didn't change the alter hooks or Views implementation.
Also need to fix the API docs.
Comment #24
klausiThis function is now empty, why? Please add a comment. @todo remove it?
Wat? Obscure comment radar alarm!
hook_menu() is gone, stop lying to me.
Otherwise this looks good, the approach makes sense and I like it that the menu constants are now gone.
Comment #26
pwolanin CreditAttribution: pwolanin commentedFixes the reset() functionality, menu delete form, and code comments indicated by klausi.
Comment #27
dawehnerJust realized we can just use ->exists('module')
Can't we fetch it directly into an array?
Comment #28
dawehnerHere is also a followup: #2228207: Test reset menu links
Comment #29
pwolanin CreditAttribution: pwolanin commentedDocumentation for exists() is unclear - only excludes NULL it seems.
Comment #30
pwolanin CreditAttribution: pwolanin commentedhere's a change to fetch as array.
Comment #31
klausiTwo ".." in here.
That comment does not make sense, there is no $existing_item here?
Comment over 80 characters.
same here, comment over 80 chars.
Comment #32
martin107 CreditAttribution: martin107 commentedfixed issues listed above
Comment #33
pwolanin CreditAttribution: pwolanin commented1st line comment should not wrap. tvn is working with me to clean up docs.
Comment #34
tvn CreditAttribution: tvn commentedComment #35
tvn CreditAttribution: tvn commentedComment #36
skipyT CreditAttribution: skipyT commentedthis looks ok.
Comment #37
webchickOk, awesome. I enabled a bunch of modules and clicked around on the off-chance I could spot a typo somewhere that lead to a new 404 / 403, but things worked out. :) (Well, apart from I get ( ! ) Fatal error: Maximum function nesting level of '100' reached, aborting! in /Users/webchick/Sites/8.x/core/vendor/composer/ClassLoader.php on line 133
when trying to enable multiple modules at once, but this happens in HEAD, too.)
Reviewing the diff, the vast majority is doing exacty what this issue says, tho there's a bunch of bitmask removal stuff as well that now move to a new StaticMenuLinks service, and a handful of other things. I didn't see anything immediately "red flaggy," and TBH I'd prefer to get this in sooner than later and handle any problems in follow-up issues since this patch is a (*&@# and a half to re-roll and review and a *tremendous* DX improvement.
Ergo...
Committed and pushed to 8.x. YEAH!! Thanks so much for everyone's hard work on this!
Comment #39
jessebeach CreditAttribution: jessebeach commentedFrom my comment in #19:
So I spent a bit of time yesterday exploring what it would mean to include options for a link in the yml file and ultimately, I convinced myself against it. We don't want display and application level datapoints in the link declaration. These can be added on the theming layer in alter and preprocess functions.
Conclusion: I agree with this commit. It gets us unblocked for further refactoring.
Comment #40
dawehner@jessebeach
I am convinced that adding attributes would actually work quite well at the moment, because all things are just
passed along to the content menu links. We can certainly discuss that in the future.
For all the other people: the approach using plugins could indeed actually allow us to let the sitebuilder choose
whether he wants menu links as content or configuration, of which the later one can be deployed. This also solves
a huge problem you had with features in the past, as this was always a pain.
Comment #41
KartagisHi,
Even after looking at the change record, I am stumped. I even read a core module's .menu_links.yml and .routing.yml, but I couldn't get it to work. I then had to post to forum. . Here is the link to it Please advise what to do.
Regards,
K.
Edit: The issue in my queue is #2233369: Update .menu_links.yml and .routing.yml to make settings show in /admin/config/system
Comment #42
BerdirLooks like we forgot to update the main routing change record: #2226903: Step 1: Move static menu links to yml files.
Comment #43
xjm:)
Comment #44
xjmFrom @berdir:
Comment #45
kay_v CreditAttribution: kay_v commentedgetting a handle on the remaining items so e2tha-e is set up to complete things; pasting from IRC chat with @xjm
1:31:18 PM kay_v: xjm: correct that the change record work for e2tha-e on https://drupal.org/node/2226903 would be to consolidate the existing change records?
1:35:19 PM xjm: kay_v: so https://drupal.org/node/2184797 should be unpublished, https://drupal.org/node/2228089 should be updated to indicate that hook_menu_default_links() was an interim name in D8, and we should also maybe work it into the main routing change record
1:35:26 PM xjm: kay_v: sorry for the delay; had that tell sitting awhile
1:35:30 PM xjm: kay_v: let me get the other link
1:38:18 PM xjm: kay_v: https://drupal.org/node/1800686 found it finally. so that one should be updated/integrated
1:38:32 PM xjm: kay_v: and a reference to this issue added to it so they show up on each other
1:38:54 PM kay_v: xjm: thanks - looks like e2tha-e is all over it :)
1:39:00 PM xjm: kay_v: awesome :)
1:39:47 PM xjm: kay_v: e2tha-e: for now put an [OBSOLETE] in the title of https://drupal.org/node/2184797 and I can get rid of it once we're done with the rest :)
Comment #46
e2tha-e CreditAttribution: e2tha-e commentedHere's what I did:
Comment #47
kay_v CreditAttribution: kay_v commentedThis appears to cover the changes that need to be made from what @kay_v and @xjm know (based on a quick chat). Can one of the folks who contributed work to it also take a look at the above change records to confirm?
Comment #48
xjmThanks @e2tha-e, great work!
Comment #49
pwolanin CreditAttribution: pwolanin commentedhttps://drupal.org/node/1800686 seems incomplete or should link to examples for local tasks, local actions, contextual links, menu links, and theme negotiation
Comment #50
xjm@pwolanin, yes, but it seems like we should file a separate issue for that, as it's out of scope for this one, aside from the static links which should be added?
Comment #51
jibranDo we have any follow up for this? We have possible use case for contrib at http://drupalcode.org/project/devel.git/blob/refs/heads/8.x-1.x:/devel_g....
Also can we mention it in change record.
Comment #52
jibranSorry I missed this part
Comment #53
dawehnerThe changes in #2226903-46: Step 1: Move static menu links to yml files are nice! It is certainly a good idea to link together the different change notices.
Comment #54
pwolanin CreditAttribution: pwolanin commentedfixing title
Comment #55
pwolanin CreditAttribution: pwolanin commentedand tag...
Comment #56
Gábor HojtsyAdded potx extraction support issue for this at #2254561: Support string extraction for Drupal 8 menu_links.yml.
Comment #58
YesCT CreditAttribution: YesCT commentedThis issue undid #2095249: Change description text for admin/config/system/site-information to remove "number of posts per page"
See #2409359: Remove "posts per page" from Site Information description in admin/config