Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

xjm’s picture

Status: Active » Postponed
xjm’s picture

jessebeach’s picture

Issue summary: View changes
Status: Postponed » Active

Unpostponed!

pwolanin’s picture

So, 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?

dawehner’s picture

Status: Active » Needs review
FileSize
28.25 KB

Here is an initial state.

Status: Needs review » Needs work

The last submitted patch, 6: menu_tree_storage-2227179-6.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
28.21 KB
1.31 KB

Status: Needs review » Needs work

The last submitted patch, 8: menu_tree_storage-2227179-8.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
28.69 KB
797 bytes

Some more fixes.

Status: Needs review » Needs work

The last submitted patch, 10: menu_tree-2229283-10.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
37.85 KB
10.41 KB

Moved the query logic of MenuTree into the MenuTreeStorage and fixed the usage here and there.

Status: Needs review » Needs work

The last submitted patch, 12: menu_tree-2229283-12.patch, failed testing.

larowlan’s picture

I 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.

jibran’s picture

Tagging as per #14

dawehner’s picture

Title: Step 2: Wrap the existing logic of accessing menu links into a plugin manager » Step 2: Move the menu tree storage to a separate service.
Issue summary: View changes

@larowlan
Yeah this issue moved to a totally different direction.

dawehner’s picture

Status: Needs work » Needs review
FileSize
37.86 KB
1.16 KB

Let's see.

Status: Needs review » Needs work

The last submitted patch, 17: menu_tree-2227179-17.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
42.71 KB
8.26 KB

Fixes 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.

Status: Needs review » Needs work

The last submitted patch, 19: menu_tree-2227179-19.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
43.35 KB
657 bytes

That was kinda simple, working on the other ones.

Status: Needs review » Needs work

The last submitted patch, 21: menu_tree-2227179-21.patch, failed testing.

dawehner’s picture

FileSize
44.25 KB
6.27 KB

Fixing a couple of additional failures.

dawehner’s picture

Status: Needs work » Needs review

.

Status: Needs review » Needs work

The last submitted patch, 23: menu_tree-2227179-22.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
47.14 KB
6.2 KB

Fixing even more failures back to basically one failure in two different tests, if you ask me.

Status: Needs review » Needs work

The last submitted patch, 26: menu_link-2227179-26.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
47.6 KB
1.31 KB

Still not green, but it is getter better.

Status: Needs review » Needs work

The last submitted patch, 28: menu_tree-2227179-28.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.96 KB
956 bytes

Tried to fix it, here are some attemps.

effulgentsia’s picture

+++ b/core/modules/menu_link/menu_link.install
@@ -217,5 +217,126 @@ function menu_link_schema() {
+  $schema['menu_tree'] = array(
+    'description' => 'Contains the menu tree hierarchy.',
+    'fields' => array(
...

Yay!

Will this issue also remove these columns from {menu_link}, or is that being punted to a follow up?

pwolanin’s picture

I think we can/should remove those columns now

pwolanin’s picture

So, 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.

pwolanin’s picture

Status: Needs review » Needs work
FileSize
11.25 KB
50.06 KB

So, 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.

xjm’s picture

Issue summary: View changes

@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

xjm’s picture

Hiding files since they don't represent the current work-in-progress.

tim.plunkett’s picture

As 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.

pwolanin’s picture

Status: Needs work » Postponed

Yes, 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.

effulgentsia’s picture

Status: Postponed » Closed (duplicate)

Duplicate 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.

xjm’s picture

Issue tags: -beta blocker
effulgentsia’s picture

#2227441: New plan, Phase 1: (prior Step 3): Implement menu links as actual plugins, which has no consensus, and a good deal of unanswered questions.

It 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.