Phase 2 - Decouple book module schema from menu links

A thousand times yes! I am (err.. was) doing the same thing with shortcuts over at #2021779-16: Decouple shortcuts from menu links.

Cleaning this up sounds awesome, this stuff drove me crazy in the Node BC removal issue. And it might help in the menu link NG issue too.

That's the route I took with shortcuts as well, so +1 :)

Updated title

Would be nice to have this cleaned up, as we'd like to overhaul the menu system as well in #1777332: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template. This would save us from making sure books can use the logics in menu.module.

Given that this isn't blocking a critical bug/task, and instead something that's really probably a feature (#2100575: [META] - Split book module from menu link), I'm not sure I agree with beta blocker on this.

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.

Rerolled and started to actually try things out.
The current state is that you can save books though the navigation is not displayed properly yet.

missed the getInfo method on the book manager test.

Some more work, this will produce more failures, as now the tests don't fatal anymore.

Some work.

+1 for using the entity checking. We are not longer bind our code to the menu/routing system but just offer some tree on top of nodes.

Just a bit more work.

@pwolanin suggested again that this should be a beta blocker for its impact on #1842858: [PP-1] Convert menu links to the new Entity Field API. Can anyone confirm whether or not this is a hard blocker for that issue? If so; it should be bumped to critical and a beta blocker; if not, it can remain a beta target (though a case could be made for it being major).

Please correct me if I'm wrong, but I don't see how this can be a hard blocker to that issue. However, it may turn out that the patch needed in that issue could be smaller/better if this issue got done first.

Agreed with #35, that issue got a green patch without this and only performance related problems stoppped it from getting in. So this is definitely not a hard blocker, but beta target and major priority makes sense indeed for the nice cleanup that it brings, and it does make that other patch easier.

Yes, we were able to make that patch green without this, but that only works because we let book.module continue to work around the menu link entity APIs, which is going to conflict with at least two other criticals (using EFQ and making the menu_link storage translatable) and will prevent us from removing the array access BC layer, which need to remove.

I think #37 is a good justification for this being critical. And in that case, since this is a schema change, also a beta blocker.

There we go, this should make it green now.

This fixes the test, though opens a new one in the book tests.

Fixed the test failures and reintroduced the book manager unit test.

Fixed issue summary

ahh, by the time the patch is picked up from the testbot queue, it won't apply anymore.

I have found two issues with this patch, through manual testing.

/core/modules/book/lib/Drupal/book/Form/BookOutlineForm.php
@@ 116 @@ public function submit(array $form, array &$form_state) {
     $book_link['menu_name'] = $this->bookManager->createMenuName($book_link['bid']);

createMenuName() is removed in this patch.

second, if a book page is created without adding to any book (- None -), then it is edited and " - Create a new book - " is selected from the outline form, it doesn't work and nothing is changed. this is because in BookManager->updateOutline(), the condition for creating a new book is inadequate:

    $new = empty($node->book['nid']);

here, after creating the initial book page, the 'nid' will have value, and the check wouldn't be correct anymore.

Thank you for the manual test, this really uncovered an not-tested usecase.

This fixes the actual bug and introduced a new test for that bit.

$ patch -p1 < book-2084421-46.patch
patching file core/modules/book/book.admin.inc
patching file core/modules/book/book.install
patching file core/modules/book/book.module
patching file core/modules/book/lib/Drupal/book/BookBreadcrumbBuilder.php
patching file core/modules/book/lib/Drupal/book/BookManager.php
Hunk #1 FAILED at 6.
Hunk #17 FAILED at 580.
Hunk #25 FAILED at 773.
3 out of 35 hunks FAILED -- saving rejects to file core/modules/book/lib/Drupal/book/BookManager.php.rej
patching file core/modules/book/lib/Drupal/book/Controller/BookController.php
patching file core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
patching file core/modules/book/lib/Drupal/book/Form/BookRemoveForm.php
patching file core/modules/book/lib/Drupal/book/Plugin/Block/BookNavigationBlock.php
patching file core/modules/book/lib/Drupal/book/Tests/BookTest.php
patching file core/modules/book/tests/Drupal/book/Tests/BookManagerTest.php

+++ b/core/modules/book/lib/Drupal/book/BookManager.php
@@ -151,16 +146,16 @@ public function getLinkDefaults($nid) {
+    return MENU_MAX_DEPTH - 1 - (($book_link['bid'] && $book_link['has_children']) ? $this->findChildrenRelativeDepth($book_link) : 0);

should be static::MENU_MAX_DEPTH

There are still several others.

BookManager.php:  const MENU_MAX_DEPTH = 9;
BookManager.php:    return MENU_MAX_DEPTH - 1 - (($book_link['bid'] && $book_link['has_children']) ? $this->findChildrenRelativeDepth($book_link) : 0);
BookManager.php:    while ($i <= MENU_MAX_DEPTH && $entity[$p]) {
BookManager.php:    for ($i = 1; $i <= MENU_MAX_DEPTH && $book_link["p$i"]; $i++) {
BookManager.php:        '#description' => $this->t('The parent page in the book. The maximum depth for a book and all child pages is !maxdepth. Some pages in the selected book may not be available as parents if selecting them would exceed this limit.', array('!maxdepth' => MENU_MAX_DEPTH)),
BookManager.php:          for ($i = 1; $i < MENU_MAX_DEPTH; $i++) {
BookManager.php:      for ($i = 1; $i <= MENU_MAX_DEPTH; $i++) {
BookManager.php:        for ($i = 1; $i <= MENU_MAX_DEPTH && $link["p$i"]; ++$i) {
BookManager.php:        for ($i = 1; $i <= MENU_MAX_DEPTH; ++$i) {

It maybe best to rename this right away in BOOK_MAX_DEPTH to make the distinction even clearer.

As I guess #2084421-46: Phase 2 - Decouple book module schema from menu links will fail too needs work. See my previous (somewhat fuzzy) comments.

Yeah that is indeed a good point, lets fix that as well as the rerole.

When outlining an article I get two fatals

PHP Fatal error:  Class 'Drupal\\book\\Unicode' not found in /Users/clemens/Sites/drupal/d8/www/core/modules/book/lib/Drupal/book/BookManager.php on line 474, referer: http://drupal.d8/node/1/outline

PHP Fatal error:  Call to undefined method Drupal\\book\\BookManager::createMenuName() in /Users/clemens/Sites/drupal/d8/www/core/modules/book/lib/Drupal/book/Form/BookOutlineForm.php on line 116, referer: http://drupal.d8/node/1/outline

\Drupal\book\BookManager misses import

use Drupal\Component\Utility\Unicode;

Is it possible to name the patch including the comment # please :)

No idea what happened in the meantime.

I (still?) get an error when outlining at least a parent book itself

There was an error adding the post to the book.

 * Contains \Drupal\book\Form\BookOutlineForm.
...
  public function submit(array $form, array &$form_state) {
...
    if ($this->bookManager->updateOutline($this->entity)) {

only gets a FALSE and never a TRUE value from

   * @return bool
   *   TRUE if the menu link was saved; FALSE otherwise.
   */
  public function updateOutline(NodeInterface $node) {

and this does not produce a return value at all.

   * @return array
   *   The book data of that node.
   */
  public function saveBookLink(array $link, $new) {

Attached patch fixes the errors around saving an outline manually. Let's see what the testbot thinks

(sigh)

+++ b/core/modules/book/lib/Drupal/book/Form/BookOutlineForm.php
@@ -115,7 +115,7 @@ class BookOutlineForm extends ContentEntityFormController {
-      if ($this->entity->book['parent_mismatch']) {

Is parent_mismatch used at all? I think that it is not used anymore

Just wanted to point out that my last patch missed the BookManagerTest again. The next person who creates a patch should merge that in

Added BookManagerTest from #52

Rerolled.

• Failed on rerolling
• The last patch from clemens contained some other issue, reverted these bits from the patch.

The interdiff from #66 contains

--- a/core/includes/file.inc
+++ b/core/includes/file.inc

I don't think these belong in here. The patch does not have these.

The test failure is cause by

Undefined index: options

.

Well, file.inc magically landed in a previous patch so I just removed it.

There was an error adding the post to the book.

The error from #58 is back again.

So either I never fixed it or we have slippery patch handovers :)

(I cannot set this issues status to needs work ... aka https://drupal.org/node/2084421/edit seems broken)

@clemens.tolboom
Please better describe how to reproduce it. I really tried hard now to reproduce it, added quite some books and figured out some problems (not sure whether they are the same as you saw).

The failure "There was an error adding the post to the book." is reproduced by

1. Create a book node but do not create a book.
2. Save book node
3. Visit outline
4. Select 'Create new book'
5. Save

attached patch fixes the missing return TRUE needed for this to succeed. As we have not yet a test for this I tried to extend testBookOutline which unfortunately still fail.

Attached the full patch (including failing test as that's needed), inter-diff and failing test snippet.

There has been various problems on the test like accessing the wrong variable and expected behavior of JS in a php only enviroment.

Let's fix the tests.

+++ b/core/modules/book/lib/Drupal/book/BookManager.php
@@ -810,6 +814,170 @@ public function bookTreeCollectNodeLinks(&$tree, &$node_links) {
+  /**
+   * @todo: what happens here?
+   *
+   * @param array $original
+   * @return type
+   */
+  protected function updateParentalStatus(array $original) {
+    if ($original['pid'] == 0) {

This function needs documentation filled in.

Furthermore I was wondering whether a Phase 3 is planned?

• Maybe drop the max depth or change the table?
• Allow contrib modules to overtake BookManager we need an interface? (https://drupal.org/project/book_made_simple)

@pwolanin is a book not allowed to be in the outline?!?

Furthermore I was wondering whether a Phase 3 is planned?

Just one more? I would guess that we need at least 2 more one or even more. I would guess we don't have a good plan what exactly to do, beside the fact that we have a lot of things to do.

This function needs documentation filled in.

Filled in the documentation.

Maybe drop the max depth or change the table?

Well, if we want to drop max depth we have to change the table. Happily we can already swap out the book manager so you might be able to do it.

Allow contrib modules to overtake BookManager we need an interface? (https://drupal.org/project/book_made_simple)

Yeah, let's add one now, but let's try to not get crazy on this issue.

81: book-2084421-81.patch queued for re-testing.

Damn you random test failures!

81: book-2084421-81.patch queued for re-testing.

Rerolled #81 patch.

88: book-2084421-87.patch queued for re-testing.

Looks good to me.

book-2084421-87.patch no longer applies.

error: patch failed: core/modules/book/book.module:5
error: core/modules/book/book.module: patch does not apply
error: patch failed: core/modules/book/lib/Drupal/book/BookManager.php:574
error: core/modules/book/lib/Drupal/book/BookManager.php: patch does not apply

Re-roll.

Back to RTBC.

seemed like testbot issue. lots of "Failed to write configuration file", "No such file or directory" errors.
94: drupal_2084421_94.patch queued for re-testing.

It was RTBC before and we could continue and increase the patch size over 100k but let's try to stop at some point (now).

book-2084421-100.patch no longer applies.

error: patch failed: core/modules/book/lib/Drupal/book/BookManager.php:518
error: core/modules/book/lib/Drupal/book/BookManager.php: patch does not apply

Re-roll, only conflict was a Note on bookTreeOutput() that was changed to a @todo and in this patch moved from BookManager to the interface.

Was RTBC before re-roll and tests pass so setting it back.

104: book-2084421-104.patch queued for re-testing.

Didn't do a full review yet.

I was a bit concerned when I saw the mat path stuff copied from menu links, but there's not actually that much extra code copied, and there's the option to factor that into something shareable later. Seems OK in general but not looked in depth enough to be comfortable committing.

One obvious bug in the patch:

+++ b/core/modules/book/book.module
@@ -434,14 +418,14 @@ function book_children($book_link) {
+  if (TRUE || $book_link['has_children']) {

Fixed the remark of @catch #107

[edit]user-1 can do more then others so
but I noticed I can add Page and Article on node/add/page|article having made Article Parent book while http://drupal.d8/admin/structure/book/settings denied those types.
[/edit]

Having added the following book structure

#0    Article for book
#1        Basic Page 1
#2        Page 2 for book
#3        Page 3 for book

It is possible to navigate from #3 to #2 (previous) and #0 (up) but not from #0 to #1 on their node pages.

This looks like a bug to me.

@pwolanin please explain #112

In #110 I set issue to "needs work". Did you fixed that one too?

@clemens.tolboom
Can you please test whether the behavior works in Drupal 8 HEAD?

reading through this.
fixing some api docs gates and some nits.
might have some questions...
patch coming.

1. +++ b/core/modules/book/book.module
   @@ -302,14 +284,14 @@ function book_form_update($form, $form_state) {
   -  if (!isset($flat[$book_link['mlid']])) {
   -    // Call bookTreeAllData() to take advantage of the menu system's caching.
   
   @@ -345,31 +325,34 @@ function _book_flatten_menu($tree, &$flat) {
          return $data['link'];
   
   @@ -746,6 +715,27 @@ function template_preprocess_book_node_export_html(&$variables) {
   + * @param $variables
   + *   An associative array containing:
   + *   - element: Structured array data for a book link.
   

   type, array.

2. +++ b/core/modules/book/book.module
   @@ -746,6 +715,27 @@ function template_preprocess_book_node_export_html(&$variables) {
   + */
   +function theme_book_link(array $variables) {
   +  $element = $variables['element'];
   

   needs public/private specified (though many others in this file are missing, but since this is new code, should be done right).
   [edit: uh, no, this in in .module, not in a class]

3. +++ b/core/modules/book/lib/Drupal/book/Access/BookNodeIsRemovableAccessCheck.php
   @@ -21,17 +22,17 @@ class BookNodeIsRemovableAccessCheck implements AccessInterface {
   -   * @var \Drupal\book\BookManager
   +   * @var BookManagerInterface
   

   classes in comments should be use full namespace path. https://drupal.org/node/1353118

4. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
   @@ -16,10 +18,12 @@
   -/**
   - * Book Manager Service.
   - */
   -class BookManager {
   +class BookManager implements BookManagerInterface {
   

   oops. doc block should remain.
   though maybe a better description than Defines a book manager.

5. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
   @@ -91,23 +89,20 @@ protected function loadBooks() {
   +      // @todo: Sort by weight and translated title.
    
   +      // @todo: use route name for links, not system path.
          foreach ($book_links as $link) {
   

   This issue might still be in an early phase of "needs review", so these might be going to be resolved in this issue. (although it has been rtbc a few times...)

   But, if we dont intend to fix these in this issue, we should open issues for them and link the issues in the comment.

6. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
   @@ -517,104 +424,69 @@ public function getTableOfContents($bid, $depth_limit, array $exclude = array())
   +    // Use $nid as a flag for whether the data being loaded is for the whole tree.
   +    $nid = isset($link['nid']) ? $link['nid'] : 0;
   +    // Generate a cache ID (cid) specific for this $bid, $link, $language, and depth.
   +    $cid = 'book-links:' . $bid . ':all:' . $nid . ':' . $language_interface->id . ':' . (int) $max_depth;
   

   since editing these comments anyway, also wrapping them to 80 chars.

7. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
   @@ -809,13 +674,198 @@ public function bookTreeCollectNodeLinks(&$tree, &$node_links) {
   +
   +  protected function moveChildren(array $link, array $original) {
   +    $query = $this->connection->update('book');
   +
   

   I can't actually figure out what this does. But started the docs anyway. Please correct the details.

     /**
      * Moves children from the original parent to the updated link.
      * 
      * @param array $link
      *   The link being saved.
      * @param array $original
      *   The original parent of $link.
      */
   

8. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
   @@ -809,13 +674,198 @@ public function bookTreeCollectNodeLinks(&$tree, &$node_links) {
   +   * @param array $link
   +   *   The book which parent we want to update.
   ...
   +   * @param array $original
   +   *   The book which parent we want to update.
   

   the argument is not the parent, but the book whose parents we want to upcdate, right?

9. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
   @@ -809,13 +674,198 @@ public function bookTreeCollectNodeLinks(&$tree, &$node_links) {
   +    $query->fields(array('has_children' => 1))
   

   should this be setting it to true?

10. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
    @@ -809,13 +674,198 @@ public function bookTreeCollectNodeLinks(&$tree, &$node_links) {
    +   * This method is mostly called when a book is moved/created etc. so we want
    +   * to update the has_children flag of the parent node.
    

    I don't think this applies to creating. It assumes original is NOT a child any longer of it's original parents.

    This makes sense for a deleted link.. but for a moved link, does it assume the children of the moved link do not move with it?

11. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
    @@ -809,13 +674,198 @@ public function bookTreeCollectNodeLinks(&$tree, &$node_links) {
    +    // Check if at least one child exists in the table.
    +    $parent_has_children = $this->connection->select('book', 'b')
    

    is this checking if $original was the child or at least something else in the book?

    Or if this link in the book *had* children, and the parent of those children needs to be updated?

12. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
    @@ -809,13 +674,198 @@ public function bookTreeCollectNodeLinks(&$tree, &$node_links) {
    +      ->condition('bid', $original['bid'])
    +      ->condition('pid', $original['pid'])
    +      ->condition('nid', $original['nid'], '<>')
    

    I'm not sure this is correct.

    Maybe that should be ->condition('pid', $original['nid'])

    to check if the nid was a parent of anything.

    Does this actually return the number of children?

13. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
    @@ -809,13 +674,198 @@ public function bookTreeCollectNodeLinks(&$tree, &$node_links) {
    +    $parent_has_children = ((bool) $parent_has_children) ? 1 : 0;
    +    $query = $this->connection->update('book');
    +    $query->fields(array('has_children' => $parent_has_children))
    +        ->condition('nid', $original['pid']);
    +    return $query->execute();
    

    if the original had children, the there is no need to execute the query to update the parent, cause the has_children value should be remaining the same (true), right?

14. +++ b/core/modules/book/lib/Drupal/book/BookManagerInterface.php
    @@ -0,0 +1,246 @@
    +   * @return
    +   *   An array of all books.
    

    I made the return type array, but I wonder if we can be more specific, like an array of bookmanagers (interfaces)?

15. +++ b/core/modules/book/lib/Drupal/book/BookManagerInterface.php
    @@ -0,0 +1,246 @@
    +   * @param $link
    +   *   A fully loaded menu link.
    +   *
    +   * @return
    +   *   A subtree of menu links in an array, in the order they should be rendered.
    +   */
    +  public function bookMenuSubtreeData($link);
    

    this was already called bookMenu... before this issue, but since this is about decoupling menu and book, I didn't expect to see menu so much in the api docs on this method.

16. +++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
    @@ -7,6 +7,8 @@
    +use \Drupal\book\BookManager;
    +use Drupal\book\BookManagerInterface;
    

    \ not needed... actually phpstorm says that class is not used. taking it out.

17. +++ b/core/modules/book/lib/Drupal/book/Form/BookOutlineForm.php
    @@ -7,6 +7,7 @@
    +use Drupal\book\BookManagerInterface;
     use Drupal\Core\Entity\ContentEntityFormController;
     use Drupal\Core\Entity\EntityManagerInterface;
     use Drupal\book\BookManager;
    

    dont need the BookManager anymore, since using the Interface.

18. +++ b/core/modules/book/tests/Drupal/book/Tests/BookManagerTest.php
    @@ -0,0 +1,127 @@
    + * Tests the book manager.
    + *
    + * @coversDefaultClass \Drupal\book\BookManager
    

    https://drupal.org/node/2057905
    also recommends @group Drupal and the module

Patch did apply, removing the needs reroll tag.
I read (almost) all the lines.
I feel like I need to read this through again before I can understand it.
I did not test this manually.
I fixed some nits, and some api docs core gates stuff (types).
I changed a variable name to make code clearer.
I added/changed (corrected) some comments.

Before patch:
./vendor/bin/phpunit --group Book --strict
OK (4 tests, 16 assertions)

After patch:
OK (7 tests, 25 assertions)

duh, functions in .module are not methods (public/protected) in a class.

sorry, missed one.

+1 for all the changes. Thank you!

Just another reroll, here is nothing to see.

Committed 0ca87db and pushed to 8.x. Thanks!

Let's get a change record in place.

I found some documentation flaws regarding BookManagerInterface after this patch landed. Should I open up a follow up to fix them (only 2 issues where the full path wasn't used when documenting the interface.)?

@cosmicdreams, yes, please open a followup and link it here.

Add the follow up issue.

Thanks!

To be honest I don't really know what to write about as most changes are sorta internal only ... https://drupal.org/node/2208415

Thanks!

Internal or not, I think the change record needs to explicitly list D7 functions (etc.) removed and their D8 replacements.

Since my commit without a change notice has caused some discussion I'd like to point out that in our policy there is latitude for committer discretion in the case of critical patches. This patch had to be rerolled a number of times and is a beta blocker so I think it passed that test.

I agree with @dawehner, there are no API functions involved that we can document in a useful way, most are hook implementations and lots of changes in the BookManager and forms and form alters. I accidently created a duplicate of @dawehner's change reocrd but it was essentially the same as his (I missed that he created one above).

So, I just fixed the project and issue reference (how did you do that!? ;)) and made the BookManagerInterface a link to the API documentation.

There is no before/after code examples that would be useful IMHO.

I did notice that this patch re-adds a book_library_info(), which should be removed. Created https://drupal.org/node/2208669 for that.

Is there a Phase 3 follow up to come?

Thanks @Berdir and @dawehner. I deleted the duplicate change record. I'm trying to figure out if there were other, earlier BC breaks for the book module that didn't get change records? https://api.drupal.org/api/drupal/7/search/book and https://api.drupal.org/api/drupal/8/search/book are very different.

Do we need to update https://drupal.org/node/1914008 as well, last section? https://drupal.org/node/1800686 has a lot of book examples but on a quick skim none of them looked relevant.

A couple of changes:

• https://drupal.org/node/1914008/revisions/view/2825295/6979785
• https://drupal.org/node/1800686/revisions/view/6974991/6979803

Discussed with @dawehner and @longwave. Unlike Taxonomy, Book in D7 didn't really have a public API to speak of... just lots of array manipulation and re-use of the menu API, which is why we have this issue in the first place. In general, people will need to re-do alters to render arrays all over the place anyway because we don't really document these with change records. So with those change record updates I think we can call this fixed. Phew. :) Thanks everyone!