Make Book storage independent

#2209025: Phase 3 - Make the BookManager interface more coherent, move more code out of book.module is probably fixing most of what you need. We will improve that class and especially over time, like moving out form logic and other stuff.

This issue sounds more like a meta issue.

I'm going to work on this

I will work on that.

Initial patch. slashsm already did an inital review at drupalcon and gave suggestions.

Let's see now.

Looks good. Few minor comments:

1. +++ b/core/modules/book/src/BookManager.php
   @@ -57,6 +57,13 @@ class BookManager implements BookManagerInterface {
   +   * Book Storage.
   +   *
   +   * @var BookStorageInterface
   +   */
   +  protected $bookStorage;
   +
   

   Provide entire namespace in @var: \Drupal\book\BookStorage

2. +++ b/core/modules/book/src/BookManager.php
   @@ -733,7 +698,7 @@ public function loadBookLink($nid, $translate = TRUE) {
       */
   
   +++ b/core/modules/book/src/BookStorage.php
   @@ -0,0 +1,193 @@
   +/**
   + * Defines a Controller class for books.
   + */
   +class BookStorage implements BookStorageInterface {
   +
   

   Defines storage class.

3. +++ b/core/modules/book/src/BookStorageInterface.php
   @@ -0,0 +1,155 @@
   +/**
   + * Defines a common interface for book controller classes.
   + */
   +interface BookStorageInterface {
   +
   

   Book storage classes.

4. +++ b/core/modules/book/src/BookStorageInterface.php
   @@ -0,0 +1,155 @@
   +   * Get books.
   +   *
   +   * @return array
   +   *   An array of book ids.
   +   */
   +  public function getBooks();
   +
   

   Gets books. + what is the difference with the next one? Maybe "Gets all books."

5. +++ b/core/modules/book/src/BookStorageInterface.php
   @@ -0,0 +1,155 @@
   +  /**
   +   * Load books.
   +   *
   +   * @param array $nids
   +   *   An array of nids.
   +   *
   +   * @return array
   +   *   Array of loaded book items.
   +   */
   +  public function loadMultiple($nids);
   

   Loads books.

6. +++ b/core/modules/book/src/BookStorageInterface.php
   @@ -0,0 +1,155 @@
   +  /**
   +   * Get child relative depth.
   +   *
   +   * @param array $book_link
   +   *   The book link.
   +   *
   +   * @param int|string $max_depth
   +   *   The maximum supported depth of the book tree.
   +   *
   

   Gets....

7. +++ b/core/modules/book/src/BookStorageInterface.php
   @@ -0,0 +1,155 @@
   +  /**
   +   * Delete a book entry.
   +   *
   +   * @param int|string $nid
   +   *   Deletes a book entry.
   +   *
   +   * @return mixed
   +   *   The return value is dependent on the database connection.
   +   */
   +  public function delete($nid);
   

   Deletes....

   (And so on for all comments in this interface)

8. +++ b/core/modules/book/src/BookStorageInterface.php
   @@ -0,0 +1,155 @@
   +  /**
   +   * Update the book id of the book link that it's beeing moved.
   +   *
   +   * @param ing|string $bid
   

   Hm... what about something like this:

   "Updates book reference for links that were moved between books."

9. +++ b/core/modules/book/tests/src/BookManagerTest.php
   @@ -56,6 +56,13 @@ class BookManagerTest extends UnitTestCase {
   +   * Book Storage.
   +   *
   +   * @var BookStorageInterface
   +   */
   +  protected $bookStorage;
   +
   

   @var with namespace.

Thx for the review.

Gets books. + what is the difference with the next one? Maybe "Gets all books."

This one only loads "books", that would be the top most book links.

1. +++ b/core/modules/book/src/BookManager.php
   @@ -57,6 +57,13 @@ class BookManager implements BookManagerInterface {
   +   * Book Storage.
   +   *
   +   * @var: \Drupal\book\BookStorageInteface
   +   */
   +  protected $bookStorage;
   +
   +  /**
       * Stores flattened book trees.
   

   @var without :.

2. +++ b/core/modules/book/tests/src/BookManagerTest.php
   @@ -56,6 +56,13 @@ class BookManagerTest extends UnitTestCase {
   +   * Book Storage.
   +   *
   +   * @var: \Drupal\book\BookStorageInteface
   +   */
   +  protected $bookStorage;
   +
   +  /**
       * {@inheritdoc}
   

   Same as above.

Removed punctuations.

Two other minor comments-related things:

1. +++ b/core/modules/book/src/BookStorageInterface.php
   @@ -0,0 +1,155 @@
   +  /**
   +   * Deletes a book entry.
   +   *
   +   * @param int|string $nid
   +   *   Deletes a book entry.
   +   *
   +   * @return mixed
   +   *   The return value is dependent on the database connection.
   +   */
   +  public function delete($nid);
   

   Return value should not be DB dependent.

2. +++ b/core/modules/book/src/BookStorageInterface.php
   @@ -0,0 +1,155 @@
   +   * @param array $parameters
   +   *   See BoookManager::bookTreeBuild().
   +   * @param int $min_depth
   

   Please explain a bit more than just "see".

Return value should not be DB dependent.

Here we are returning the return from the delete query. So I just copied what the comment for \Drupal\Core\Database\Query\Delete::execute says.

Please explain a bit more than just "see".

Copied stuff from bookTreeBuild. Or did you mean to just elaborate more on why "see" ?

Copied stuff from bookTreeBuild. Or did you mean to just elaborate more on why "see" ?

I'd go with something like: "An associative array of build parameters. For info about individual parameters see BookManager::bookTreeBuild()."

Here we are returning the return from the delete query. So I just copied what the comment for \Drupal\Core\Database\Query\Delete::execute says.

I'd go with something like: "Number of deleted book entires."

Thx for the suggestions.

Some minor nits. I know that the book module doesn;t have the test coverage of some of the other modules - can we check that we've got enough coverage or what we're changing here.

1. +++ b/core/modules/book/src/BookManager.php
   @@ -57,6 +57,13 @@ class BookManager implements BookManagerInterface {
   +   * @var \Drupal\book\BookStorageInteface
   
   +++ b/core/modules/book/src/BookStorage.php
   @@ -0,0 +1,193 @@
   +  /**
   
   +++ b/core/modules/book/tests/src/BookManagerTest.php
   @@ -56,6 +56,13 @@ class BookManagerTest extends UnitTestCase {
   +   * @var \Drupal\book\BookStorageInteface
   

   Mis-spelt

2. +++ b/core/modules/book/src/BookStorage.php
   @@ -0,0 +1,193 @@
   +   * Constructs a BookManager object.
   

   Book storage

3. +++ b/core/modules/book/src/BookStorageInterface.php
   @@ -0,0 +1,156 @@
   +   * Update the book id of the book link that it's beeing moved.
   ...
   +   *   element beeing moved.
   

   Mis-spelt - being

4. +++ b/core/modules/book/src/BookStorageInterface.php
   @@ -0,0 +1,156 @@
   +   * @param ing|string $bid
   +   *   The ID of the book whose children we move.
   

   ing? I'm guessing integer

Fixed review stuff.

As for the test coverage goes, I've went through the tests and as far as I can understand most of it should be there. Except the test already mentioned in #2282959: Fatal error when accessing book hierarchy at /book. But as this is my first core issue it would be perhaps good to give this a second lool.

The patch contains an update for the file sites/sites.php. Maybe I am wrong but I do not think that it belongs to this patch. Below the update to the file sites/sites.php.

+<?php
+
+/**
+ * @file
+ * Configuration file for multi-site support and directory aliasing feature.
+ *
+ * This file is required for multi-site support and also allows you to define a
+ * set of aliases that map hostnames, ports, and pathnames to configuration
+ * directories in the sites directory. These aliases are loaded prior to
+ * scanning for directories, and they are exempt from the normal discovery
+ * rules. See default.settings.php to view how Drupal discovers the
+ * configuration directory when no alias is found.
+ *
+ * Aliases are useful on development servers, where the domain name may not be
+ * the same as the domain of the live server. Since Drupal stores file paths in
+ * the database (files, system table, etc.) this will ensure the paths are
+ * correct when the site is deployed to a live server.
+ *
+ * To activate this feature, copy and rename it such that its path plus
+ * filename is 'sites/sites.php'.
+ *
+ * Aliases are defined in an associative array named $sites. The array is
+ * written in the format: '<port>.<domain>.<path>' => 'directory'. As an
+ * example, to map http://www.drupal.org:8080/mysite/test to the configuration
+ * directory sites/example.com, the array should be defined as:
+ * @code
+ * $sites = array(
+ *   '8080.www.drupal.org.mysite.test' => 'example.com',
+ * );
+ * @endcode
+ * The URL, http://www.drupal.org:8080/mysite/test/, could be a symbolic link or
+ * an Apache Alias directive that points to the Drupal root containing
+ * index.php. An alias could also be created for a subdomain. See the
+ * @link http://drupal.org/documentation/install online Drupal installation guide @endlink
+ * for more information on setting up domains, subdomains, and subdirectories.
+ *
+ * The following examples look for a site configuration in sites/example.com:
+ * @code
+ * URL: http://dev.drupal.org
+ * $sites['dev.drupal.org'] = 'example.com';
+ *
+ * URL: http://localhost/example
+ * $sites['localhost.example'] = 'example.com';
+ *
+ * URL: http://localhost:8080/example
+ * $sites['8080.localhost.example'] = 'example.com';
+ *
+ * URL: http://www.drupal.org:8080/mysite/test/
+ * $sites['8080.www.drupal.org.mysite.test'] = 'example.com';
+ * @endcode
+ *
+ * @see default.settings.php
+ * @see conf_path()
+ * @see http://drupal.org/documentation/install/multi-site
+ */

Ups, sorry for that. Here is the same patch without the accidentally added sites.php.

Found one tiny strageness in comments. Can confirm sites.php are gone though :)

1. +++ b/core/modules/book/src/BookStorageInterface.php
   @@ -0,0 +1,156 @@
   +   *
   +   * @param int|string $max_depth
   +   *   The maximum supported depth of the book tree.
   +   *
   +   * @return int
   

   Isn't this always int?

2. +++ b/core/modules/book/src/BookStorageInterface.php
   @@ -0,0 +1,156 @@
   +   *
   +   * @param int|string $nid
   +   *   Deletes a book entry.
   +   *
   

   Same as above?

3. +++ b/core/modules/book/src/BookStorageInterface.php
   @@ -0,0 +1,156 @@
   +   *
   +   * @param int|string $pid
   +   *   The parent's book nid.
   +   *
   

   as above?

4. +++ b/core/modules/book/src/BookStorageInterface.php
   @@ -0,0 +1,156 @@
   +   *
   +   * @param int|string $bid
   +   *   The ID of the book that we are building the tree for.
   +   * @param array $parameters
   

   again

5. +++ b/core/modules/book/src/BookStorageInterface.php
   @@ -0,0 +1,156 @@
   +   *
   +   * @param int|string $nid
   +   *   The nid of the book entry to be updated.
   +   * @param array $fields
   

   Same here.

6. +++ b/core/modules/book/src/BookStorageInterface.php
   @@ -0,0 +1,156 @@
   +   * @param int|string $bid
   +   *   The ID of the book whose children we move.
   +   * @param array $original
   

   and here

I agree with #27

Looks good. Thanks!

+++ b/core/modules/book/book.services.yml
@@ -6,14 +6,16 @@ services:
+    arguments: ['@database', '@entity.manager', '@string_translation', '@config.factory', '@book.storage']

Looks like the @database argument can be removed no?

Regarding #30 Yep, it is obvious when you see it... snip snip ... stray dependency removed.

BookManagerTest passes locally.

Just thinking about the last change.

BookManaager::__construct TranslationInterface $translation can also be removed as a dependency.

But it is dangerous to cross the streams... so I am opening up another issue for that. I will list it as a child of this one.

It uses StringTranslationTrait, so it is a wanted dependancy!

Feedback got adressed

+++ b/core/modules/book/src/BookStorage.php
@@ -0,0 +1,193 @@
+/**
+ * Defines a storage class for books.
+ */
+class BookStorage implements BookStorageInterface {

+++ b/core/modules/book/src/BookStorageInterface.php
@@ -0,0 +1,156 @@
+/**
+ * Defines a common interface for book storage classes.
+ */
+interface BookStorageInterface {

I think BookStorage is incorrectly named - we're not storing books here we are storing book outline information. Also I think we should have a bit more documentation on what an outline is or at least point to some documentation on what it is.

Here are the fixes I'd make if I was going to commit this:

diff --git a/core/modules/book/src/BookManager.php b/core/modules/book/src/BookManager.php
index 36614bc..de4a158 100644
--- a/core/modules/book/src/BookManager.php
+++ b/core/modules/book/src/BookManager.php
@@ -9,7 +9,6 @@
 
 use Drupal\Component\Utility\Unicode;
 use Drupal\Core\Cache\Cache;
-use Drupal\Core\Database\Connection;
 use Drupal\Core\Entity\EntityManagerInterface;
 use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\Session\AccountInterface;
diff --git a/core/modules/book/src/BookStorageInterface.php b/core/modules/book/src/BookStorageInterface.php
index ea03893..1564912 100644
--- a/core/modules/book/src/BookStorageInterface.php
+++ b/core/modules/book/src/BookStorageInterface.php
@@ -16,7 +16,7 @@
    * Gets books (the highest positioned book links).
    *
    * @return array
-   *   An array of book ids.
+   *   An array of book IDs.
    */
   public function getBooks();
 
@@ -24,7 +24,7 @@ public function getBooks();
    * Loads books.
    *
    * @param array $nids
-   *   An array of nids.
+   *   An array of node IDs.
    *
    * @return array
    *   Array of loaded book items.
@@ -49,18 +49,18 @@ public function getChildRelativeDepth($book_link, $max_depth);
    * Deletes a book entry.
    *
    * @param int $nid
-   *   Deletes a book entry.
+   *   The node ID of the book entry to delete.
    *
    * @return mixed
-   *   Number of deleted book entires.
+   *   Number of deleted book entries.
    */
   public function delete($nid);
 
   /**
-   * Loads book's children using it's parent's id.
+   * Loads a book's children using its parent ID.
    *
    * @param int $pid
-   *   The parent's book nid.
+   *   The book's parent ID.
    *
    * @return array
    *   Array of loaded book items.
@@ -113,7 +113,7 @@ public function insert($link, $parents);
   public function update($nid, $fields);
 
   /**
-   * Update the book id of the book link that it's beeing moved.
+   * Update the book id of the book link that it's being moved.
    *
    * @param int $bid
    *   The ID of the book whose children we move.

So before fixup along the lines of #34. I am re-rolling.

Naming things is hard, just proposing a simple name and trying on a couple of existing methods :-

BookOutlineService::countOriginalLinkChildren()
BookOutlineService::getBookMenuTree()
BookOutlineService::getBookSubtree()
BookOutlineService::loadBookChildren()

For me it does not fit comfortably with all methods, I am not tied to this name, just a first iteration.

BookOutlineService -- it is not the drupal way to append a service with "Service"

Perhaps BookOutllineManager?

BookOutliner?

If I may, I think

Drupal\book\BookStorage -> \Drupal\book\BookOutliner

will cause confusion with an existing class \Drupal\book\BookOutline. which is as follows

/**
 * @file
 * Contains \Drupal\book\BookOutline.
 */

namespace Drupal\book;

/**
 * Provides handling to render the book outline.
 */
class BookOutline {

So my counter is :-

Drupal\book\BookStorage -> \Drupal\book\BookOutliner
Drupal\book\Outline-> \Drupal\book\BookOutlineHandler

Am I using Handler in an acceptable way, that is the question? Or would BookOutlineRender stick in anyone craw.

What about Drupal\book\BookStorage -> Drupal\book\BookOutlineStorage and Drupal\book\BookStorageInterface -> Drupal\book\BookOutlineStorageInterface?

Looks good!

This issue is about making book storage independent and not about fixing existing @todos. We are not changing any logic in this patch. We only re-implement existing logic in a more friendly way when it comes to storage swapability.

I totally support rising follow-ups (which should have been done when @todos were added to the codebase) and addressing things that are mentioned in #41 there. Fixing that stuff is out of this issue's scope and would make patch unnecessarly complex.

+++ b/core/modules/book/book.services.yml
@@ -6,7 +6,7 @@ services:
+    arguments: ['@entity.manager', '@string_translation', '@config.factory', '@book.storage']

@@ -15,7 +15,9 @@ services:
+  book.storage:

+++ b/core/modules/book/src/BookManager.php
@@ -58,6 +50,13 @@ class BookManager implements BookManagerInterface {
+   * Book Storage.
...
+  protected $bookStorage;

@@ -67,11 +66,11 @@ class BookManager implements BookManagerInterface {
+  public function __construct(EntityManagerInterface $entity_manager, TranslationInterface $translation, ConfigFactoryInterface $config_factory, BookOutlineStorageInterface $book_storage) {
...
+    $this->bookStorage = $book_storage;

@@ -89,16 +88,10 @@ public function getAllBooks() {
+    $nids = $this->bookStorage->getBooks();
...
+      $book_links = $this->bookStorage->loadMultiple($nids);

@@ -148,21 +141,7 @@ public function getParentDepthLimit(array $book_link) {
+    $max_depth = $this->bookStorage->getChildRelativeDepth($book_link, static::BOOK_MAX_DEPTH);

@@ -436,14 +415,12 @@ public function getTableOfContents($bid, $depth_limit, array $exclude = array())
+    $this->bookStorage->delete($nid);
...
+      $result = $this->bookStorage->loadBookChildren($nid);

@@ -630,32 +607,11 @@ protected function doBookTreeBuild($bid, array $parameters = array()) {
+      $result = $this->bookStorage->getBookMenuTree($bid, $parameters, $min_depth, static::BOOK_MAX_DEPTH);

@@ -734,7 +690,7 @@ public function loadBookLink($nid, $translate = TRUE) {
+    $result = $this->bookStorage->loadMultiple($nids);

@@ -842,18 +790,8 @@ protected function moveChildren(array $link, array $original) {
+    $this->bookStorage->updateMovedChildren($link['bid'], $original, $expressions, $shift);

@@ -875,10 +813,7 @@ protected function updateParent(array $link) {
+    return $this->bookStorage->update($link['pid'], array('has_children' => 1));

@@ -901,22 +836,13 @@ protected function updateOriginalParent(array $original) {
+    $original_number_of_children = $this->bookStorage->countOriginalLinkChildren($original);
...
+    return $this->bookStorage->update($original['pid'], array('has_children' => $parent_has_children));

@@ -1103,17 +1028,9 @@ public function bookSubtreeData($link) {
+        $result = $this->bookStorage->getBookSubtree($link, static::BOOK_MAX_DEPTH);

+++ b/core/modules/book/tests/src/BookManagerTest.php
@@ -52,16 +45,21 @@ class BookManagerTest extends UnitTestCase {
+  protected $bookStorage;
...
+    $this->bookStorage = $this->getMock('Drupal\book\BookOutlineStorageInterface');
+    $this->bookManager = new BookManager($this->entityManager, $this->translation, $this->configFactory, $this->bookStorage);

We're still calling the service book.storage and the using the variable names book_storage and bookStorage - let's add the make these book.outline_storage, book_outline_storage and bookOutlineStorage respectively.

Ah... my bad. Good catch!

Can someone update the issue summary to be aligned to the solution in the latest patch - also I think https://www.drupal.org/node/2208415 could be fleshed out and linked to this issue.

Done.

Committed ae319dd and pushed to 8.0.x. Thanks!