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.
Related Issues
Original report by @kgoel
Follow-up at #2084421: Phase 2 - Decouple book module schema from menu links.
Comment | File | Size | Author |
---|---|---|---|
#14 | 2100577-6-14.increment.txt | 9.32 KB | pwolanin |
#14 | book-p1-2100577-14.patch | 31.95 KB | pwolanin |
#6 | interdiff.txt | 24.98 KB | dawehner |
#6 | book-2100577.patch | 24.98 KB | dawehner |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedProgress, but getting a fatal during the BookTest.
Comment #2
sidharthapComment #4
pwolanin CreditAttribution: pwolanin commented@sidharthap - it NEEDS WORK - it will never work now, it's just a starting patch.
Comment #4.0
pwolanin CreditAttribution: pwolanin commentedUpdated issue summary.
Comment #5
dawehnerComment #6
dawehnerGreen is enough for now.
Comment #7
pwolanin CreditAttribution: pwolanin commentedComment #8
pwolanin CreditAttribution: pwolanin commentedmarking 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
Comment #10
xjmSee @webchick's comment on #2084421: Phase 2 - Decouple book module schema from menu links #12.
Comment #11
dawehner6: book-2100577.patch queued for re-testing.
Comment #13
pwolanin CreditAttribution: pwolanin commented@xjm - responded there. I do think this is an important part of the menu system decoupling work.
Comment #14
pwolanin CreditAttribution: pwolanin commentedFirst 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.
Comment #15
dawehnerLet's get it in, in a way that the amount of changes is as less as possible.
Comment #16
xjmWhile 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.
Comment #17
pwolanin CreditAttribution: pwolanin commented@xjm - I disagree. We won't finish the menu links work without this decoupling.
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedRight - this issue is blocking a Critical - #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit.
Comment #19
alexpottHow come we can get rid of this with no obvious replacement?
Comment #20
dawehner@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.
Comment #21
dawehnerComment #22
xjmIf 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?
Comment #23
alexpottCommitted 44b1f64 and pushed to 8.x. Thanks!
#2084421: Phase 2 - Decouple book module schema from menu links should responsible for the change notice.
Comment #24
pwolanin CreditAttribution: pwolanin commentedYes, Phase 2 will have the major schema and other changes.