Updated: Comment #N

Problem/Motivation

The re-use of the {menu_links} table by book module has always been a bit of an awkward implementation. I (pwolanin) wanted to build book on tops of the menu link hierarchy handling and by the end of the 6.x cycle, there wasn't time left to do it better than this.

The coupling now between the book module and menu link code is blocking needed finalization of menu link entity since like removing ArrayAccess

Proposed resolution

Copy helper function from menu.inc into BookManager.php

Remove book_page_alter() as its functionality (fixing the book breadcrumb) is already taken care of in the BookBreadcrumbBuilder

Remaining tasks

User interface changes

None

API changes

The schema will change, but the book module API itself should not change significantly. Possible enhancements to the menu helper functions.

Original report by @kgoel

Follow-up at #2084421: Phase 2 - Decouple book module schema from menu links.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Status: Active » Needs work
FileSize
24.27 KB

Progress, but getting a fatal during the BookTest.

sidharthap’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, book-p1-2100577-1.patch, failed testing.

pwolanin’s picture

@sidharthap - it NEEDS WORK - it will never work now, it's just a starting patch.

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

Issue summary: View changes
dawehner’s picture

Status: Needs work » Needs review
FileSize
24.98 KB
24.98 KB

Green is enough for now.

pwolanin’s picture

pwolanin’s picture

Issue summary: View changes
Issue tags: +beta blocker

marking this as a beta blocker due to the needed API and schema changes, and the fact it's needed to allow menu link entity to be finalized

discussed some with alexpott and updating the issue summary too

The last submitted patch, 6: book-2100577.patch, failed testing.

xjm’s picture

dawehner’s picture

6: book-2100577.patch queued for re-testing.

The last submitted patch, 6: book-2100577.patch, failed testing.

pwolanin’s picture

@xjm - responded there. I do think this is an important part of the menu system decoupling work.

pwolanin’s picture

FileSize
31.95 KB
9.32 KB

First off, resolved some conflicts with HEAD.

Next, I was seeing the printer-friendly test fails locally. THe key problem was in bookTreeCollectNodeLinks(), the route_parameters were still serialized.

This patch fixes that and moves more code into the book manager to decouple from menu.inc and avoid errors from sports where an array was passed as an entity.

I'm also seeing some bad regressions on the add child page functionality, but I think that may be pre-existing in HEAD.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Let's get it in, in a way that the amount of changes is as less as possible.

xjm’s picture

Issue tags: -beta blocker +beta target

While I agree this is a good change to make and would be good to have in by beta 1, we could conceivably ship D8 without this change, and therefore it doesn't block beta. Tagging "beta target" instead.

pwolanin’s picture

@xjm - I disagree. We won't finish the menu links work without this decoupling.

moshe weitzman’s picture

alexpott’s picture

+++ b/core/modules/book/book.module
@@ -474,20 +474,6 @@ function book_node_view(EntityInterface $node, EntityDisplay $display, $view_mod
-function book_page_alter(&$page) {
-  if (($node = menu_get_object()) && !empty($node->book['bid'])) {
-    $active_menus = menu_get_active_menu_names();
-    $active_menus[] = $node->book['menu_name'];
-    menu_set_active_menu_names($active_menus);
-  }
-}

How come we can get rid of this with no obvious replacement?

dawehner’s picture

@alexpott
This code was used only for providing a breadcrumb for books. git blame returns #444920: Book breadcrumbs are broken
On the other hand though books now have their own book breadcrumb builder, so we don't have to deal with that code anymore. Once they are no menu links anymore this piece of code does nothing anyway.

dawehner’s picture

Issue summary: View changes
xjm’s picture

If the issue is actually blocking a critical (as a "hard blocker" as opposed to a "soft blocker" i.e. the code will conflict) then it should be critical as well. I don't see any indication here as to whether or why this is blocking #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit -- can we document that on-issue please, and bump the priority if appropriate?

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 44b1f64 and pushed to 8.x. Thanks!

#2084421: Phase 2 - Decouple book module schema from menu links should responsible for the change notice.

pwolanin’s picture

Yes, Phase 2 will have the major schema and other changes.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.