Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Problem/Motivation
Child issue of #2178723: [meta-2] Finalize module-facing API of declaring menu links.
Proposed resolution
Wrap all the storage code into its own service.
Remaining tasks
Patch is currently being developed in a sandbox:
http://drupalcode.org/sandbox/dereine/2031809.git/tree/refs/heads/2227441
git clone --branch 2227441 http://git.drupal.org/sandbox/dereine/2031809.git menutree
User interface changes
API changes
Postponed until
Comment | File | Size | Author |
---|
Comments
Comment #1
dawehnerComment #2
xjmPostponed on #2226903: Step 1: Move static menu links to yml files.
Comment #3
xjmComment #4
jessebeach CreditAttribution: jessebeach commentedUnpostponed!
Comment #5
pwolanin CreditAttribution: pwolanin commentedSo, I'm puzzling a bit over how to accomplish steps 2 and 3 without just breaking the current API to hell and building it back again.
Maybe we can split the entity base table off here from the hierarchy table? We might have duplicated data, but we could handle the loading and saving of hierarchy data only via the menu tree service?
Comment #6
dawehnerHere is an initial state.
Comment #8
dawehnerComment #10
dawehnerSome more fixes.
Comment #12
dawehnerMoved the query logic of MenuTree into the MenuTreeStorage and fixed the usage here and there.
Comment #14
larowlanI came across this and found the term 'plugin manager' confusing.
Looking at the patch, it seems to be about MenuTreeStorage - can we get an issue summary update.
Comment #15
jibranTagging as per #14
Comment #16
dawehner@larowlan
Yeah this issue moved to a totally different direction.
Comment #17
dawehnerLet's see.
Comment #19
dawehnerFixes at least a big range of issues but there are still failures I don't get at all. It works really fine if you manually test entries. Maybe the db_merge() is problematic.
Comment #21
dawehnerThat was kinda simple, working on the other ones.
Comment #23
dawehnerFixing a couple of additional failures.
Comment #24
dawehner.
Comment #26
dawehnerFixing even more failures back to basically one failure in two different tests, if you ask me.
Comment #28
dawehnerStill not green, but it is getter better.
Comment #30
dawehnerTried to fix it, here are some attemps.
Comment #31
effulgentsia CreditAttribution: effulgentsia commentedYay!
Will this issue also remove these columns from {menu_link}, or is that being punted to a follow up?
Comment #32
pwolanin CreditAttribution: pwolanin commentedI think we can/should remove those columns now
Comment #33
pwolanin CreditAttribution: pwolanin commentedSo, thinking about how we can move forward here, I'm wondering if we should try to make an intermediate patch that does away with all the optimized storage so we can clearly define the interface for the tree service and let it just do some recursive SQL.
Once we are happy with the interface we can add back the optimized storage to finish the patch.
Looking at the patch now, I don't think we want to be adding all the hierarchy data to a menu link when it's loaded, for example.
Comment #34
pwolanin CreditAttribution: pwolanin commentedSo, if we need methods to operate on a tree element, we should use "vertex" not "node" (please).
Here's an non-working patch and increment showing some renames to vertex, but I'm going to try ripping out a lot of code next.
Comment #35
xjm@dawehner and @pwolanin are currently working on this in a sandbox due to the scope of the change and the collaboration needed:
http://drupalcode.org/sandbox/dereine/2031809.git/tree/refs/heads/2227441
git clone --branch 2227441 http://git.drupal.org/sandbox/dereine/2031809.git menutree
Comment #36
xjmHiding files since they don't represent the current work-in-progress.
Comment #37
tim.plunkettAs far as I understand it, the code in the sandbox is NOT just this issue, but also includes the scope of #2227441: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins
#2227441: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins, which has no consensus, and a good deal of unanswered questions.
Comment #38
pwolanin CreditAttribution: pwolanin commentedYes, as I started digging into the changes needed here to build a working patch, it seems like there would be a tremendous amount of wasted effort.
So, we started in the sandbox to see if going straight to step 3 would be feasible.
I understand we still need discussion, but marking this postponed so we can focus on a reasonably working PoC for the first part of step 3 as the basis for that discussion.
Comment #39
effulgentsia CreditAttribution: effulgentsia commentedDuplicate of #2227441: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins unless/until someone presents a credible roadmap of how to do this without that.
Comment #40
xjmComment #41
effulgentsia CreditAttribution: effulgentsia commentedIt would be great if anyone who has arguments against #2227441: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins, or the larger meta: #2256497: [meta] Menu Links - New Plan for the Homestretch, to post them to the corresponding issue. If you need to wait until a closer to done patch is ready, that's fine, but if you have concerns you'd like new people to the issue (e.g., Dries) to be aware of now, please post them.