Phase 3 - Make the BookManager interface more coherent, move more code out of book.module

continuing the work, still not really a patch.

Let's see.

Fixes.

Right.

Created a change notice: https://drupal.org/node/2216631

+++ b/core/modules/book/lib/Drupal/book/BookManager.php
@@ -15,6 +15,7 @@
+use PDO;

Do we really need this?

Not at all.

1. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
   @@ -1024,19 +1086,19 @@ protected function _menu_tree_data(&$links, $parents, $depth) {
        $tree = &drupal_static(__METHOD__, array());
   ...
   -        $cache = \Drupal::cache('menu')->get($cache->data);
   +        $cache = \Drupal::cache('menu')->get($tree_cid_cache->data);
   

   Out of scope, but it'd be nice to fix this someday

2. +++ b/core/modules/book/lib/Drupal/book/Plugin/Block/BookNavigationBlock.php
   @@ -122,7 +134,7 @@ public function build() {
   -          $book_menus[$book_id] = \Drupal::service('book.manager')->bookTreeOutput($pseudo_tree);
   +          $book_menus[$book_id] = $this->bookManager->bookTreeOutput($pseudo_tree);
   

   Yay!

+++ b/core/modules/book/lib/Drupal/book/BookManager.php
@@ -1024,19 +1086,19 @@ protected function _menu_tree_data(&$links, $parents, $depth) {
     $tree = &drupal_static(__METHOD__, array());
...
-        $cache = \Drupal::cache('menu')->get($cache->data);
+        $cache = \Drupal::cache('menu')->get($tree_cid_cache->data);
Out of scope, but it'd be nice to fix this someday

That's potentially solved by #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.

1. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
   @@ -253,6 +267,14 @@ public function updateOutline(NodeInterface $node) {
   +    if (!empty($node->book['bid'])) {
   

   Why the nested if and not &&?

2. +++ b/core/modules/book/book.module
   @@ -181,21 +181,6 @@ function book_admin_paths() {
   -function book_get_books() {
   

   This isn't in the new change notice, does the previous one need updating for removal?

3. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
   @@ -1024,19 +1086,19 @@ protected function _menu_tree_data(&$links, $parents, $depth) {
   +      if ($tree_cid_cache && isset($tree_cid_cache->data)) {
   

   No need for isset() check if there's also a check for $tree_cid_cache, ->data is always set. I realise this logic is in the old code too.

4. +++ b/core/modules/book/lib/Drupal/book/BookManagerInterface.php
   @@ -54,6 +54,20 @@ public function bookTreeAllData($bid, $link = NULL, $max_depth = NULL);
   +   *   If TRUE, set access, title, and other elements.
   

   I know this comes from the menu system, but $translate setting access always felt like an accident to me and it's being enforced here. What's the use case for not setting $translate? Is that ever used?

5. +++ b/core/modules/book/lib/Drupal/book/BookOutline.php
   @@ -0,0 +1,136 @@
   +  public function childrenLinks($book_link) {
   

   Does this need array type hint?

This isn't in the new change notice, does the previous one need updating for removal?

No idea, just extended https://drupal.org/node/2216631

No need for isset() check if there's also a check for $tree_cid_cache, ->data is always set. I realise this logic is in the old code too.

Yeah let's fix it now.

I know this comes from the menu system, but $translate setting access always felt like an accident to me and it's being enforced here. What's the use case for not setting $translate? Is that ever used?

If you are in an API only change without any output involved you just want to load the book entry and not care about the additional information.
One place we use it at the moment is hook_node_load:

function book_node_load($nodes) {
  /** @var \Drupal\book\BookManagerInterface $book_manager */
  $book_manager = \Drupal::service('book.manager');
  $links = $book_manager->loadBookLinks(array_keys($nodes), FALSE);

Does this need array type hint?

Fixed the other one as well.

.

15: book_manager-2209025-15.patch queued for re-testing.

Reroll

Rerolled.

Rerolled on top of this morning's core.

Forgot to set status.

Back to RTBC

Committed/pushed to 8.x, thanks!