Convert book_admin_overview and book_render to a new-style Controller

Attached is a patch that migrates book_admin_overview to new-style Controller.

route -> route_name

removing database dependency in controller

adding return of static() to create method.

moved book_get_books() to a service injected. All book tests passed locally.

Note: I can't remove the book_get_books() method (yet) because it's in use in other callbacks that would fail.

absolutely no idea how tests passed locally... Attached fixes the syntax error in the controller.

added an arguments with a Reference to the database in BookBundle.php. Also, tests are passing, because test coverage doesn't cover this callback. It just does a GET on it, but never verifies that the page is okay, so an error page passes.

Added use PDO to BookManager class.

first reroll! That's right, we got one committed a few minutes ago!

#19: drupal-book_admin_overview-1938296-19.patch queued for re-testing.

Attached patch merges issue: #1938312: Convert book_render to a new-style Controller. Credit goes to robmc for this!

Attached addresses comments in #24.

Fixed doctags to have full class instead of object.

attached removes static drupal function from BookManager.

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.phpundefined
@@ -0,0 +1,82 @@
+   *  Returns an administrative overview of all books.

There is a space to much

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.phpundefined
@@ -0,0 +1,82 @@
+  public function adminOverview() {
+  $rows = array();

Intendation is a bit off here. Makes it hard to read.

We added a "book_get_books" service but we didn't remove the original code?

Btw, the code is a perfect example how to create services and how to use them in combination with other services.
Great reading material.

:D

#28: drupal-book_admin_overview-1938296-28.patch queued for re-testing.

i got this error when i tried to apply the patch:
$ git apply --index drupal-book_admin_overview-1938296-28.patch
error: patch failed: core/modules/book/book.admin.inc:8
error: core/modules/book/book.admin.inc: patch does not apply

Rerolled and fixed doc tags and indentation aspilicious pointed out. There's still too many other references to book_get_books at this time to all roll into this one patch, so I left the method there temporarily:
core/modules/book/book.module: foreach (book_get_books() as $book) {
core/modules/book/book.module: drupal_static_reset('book_get_books');
core/modules/book/book.module: drupal_static_reset('book_get_books');
core/modules/book/book.module: drupal_static_reset('book_get_books');
core/modules/book/lib/Drupal/book/Plugin/block/block/BookNavigationBlock.php: foreach (book_get_books() as $book_id => $book) {

I also added a TODO to the doctag for the book_get_books() to remove it.

+++ b/core/modules/book/lib/Drupal/book/BookBundle.phpundefined
@@ -0,0 +1,27 @@
+ * Definition of \Drupal\book\BookBundle.

this is Contains now
http://drupal.org/node/1354#file

+++ b/core/modules/book/lib/Drupal/book/BookBundle.phpundefined
@@ -0,0 +1,27 @@
+   * Overrides Symfony\Component\HttpKernel\Bundle\Bundle::build().

I think these start with \ in comments.

http://drupal.org/node/1353118

$ grep -R "[*] Overrides Drupal" * | wc -l
     310
$ grep -R "[*] Overrides [\]Drupal" * | wc -l
     455

So I guess there are some either way.

Symfony:

$ grep -R "[*] Overrides Symfony" * | wc -l
      19
$ grep -R "[*] Overrides [\]Symfony" * | wc -l
       9

We can leave this for now I think.

+++ b/core/modules/book/lib/Drupal/book/BookManager.phpundefined
@@ -0,0 +1,76 @@
+<?php
+namespace Drupal\book;

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.phpundefined
@@ -0,0 +1,82 @@
+<?php
+namespace Drupal\book\Controller;

missing @file doc block.
See http://drupal.org/node/1353118
and http://drupal.org/node/1354#file

+++ b/core/modules/book/lib/Drupal/book/BookManager.phpundefined
@@ -0,0 +1,76 @@
+    $this->database = $database;
+  }
+  /**
+   * Returns an array of all books.

missing newline?

+++ b/core/modules/book/lib/Drupal/book/BookManager.phpundefined
@@ -0,0 +1,76 @@
+  function getAllBooks() {

function does not specify public or protected. which should it be?

+++ b/core/modules/book/lib/Drupal/book/BookManager.phpundefined
@@ -0,0 +1,76 @@
+  }
+}

class should have a newline before the final closing bracket.

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.phpundefined
@@ -0,0 +1,82 @@
+  }
+  /**

missing newline.

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.phpundefined
@@ -0,0 +1,82 @@
+    return theme('item_list', array('items' => $book_list));
+  }
+}

missing newline at end of class.

------
some of this is non-blocking
but the @file is a core gate: http://drupal.org/core-gates#documentation-block-requirements

ill make a new patch for those concerns

Attached is a patch fixing coding standards. I guess I need to reinstall drupalcs on this dev box ;-)

forgot to rebase. here's the real patch.

#40: drupal-book_admin_overview-1938296-40.patch queued for re-testing.

+++ b/core/modules/book/lib/Drupal/book/BookManager.phpundefined
@@ -0,0 +1,83 @@
+use PDO;
...
+      $query = $this->database->select('book', 'b', array('fetch' => PDO::FETCH_ASSOC));

I am not gonna unRTBC this but we stopped doing this.
Instead remove use statement and turn constant to \PDO::FETCH_ASSOC

Yep.

Attached patch makes suggested change in 44.

+++ b/core/modules/book/book.moduleundefined
@@ -274,6 +270,8 @@ function book_entity_view_mode_info() {
  * Returns an array of all books.
  *
+ * TODO: Remove in favor of BookManager Service.
+ *

Looks like this patch has the book manager service, so can we just take care of this todo now?

here is the issue: #1963894: remove function book_entity_view_mode_info in favor of BookManager Service

and here is the patch which fixes the TODO -> @todo standard and links to that issue.
http://drupal.org/node/1354#todo

also updating issue summary.

+++ b/core/modules/book/lib/Drupal/book/BookManager.phpundefined
@@ -0,0 +1,82 @@
+  protected function loadBooks() {
+    $this->books = array();
+    $nids = $this->database->query("SELECT DISTINCT(bid) FROM {book}")->fetchCol();
+    if ($nids) {
+      $query = $this->database->select('book', 'b', array('fetch' => \PDO::FETCH_ASSOC));
+      $query->join('node', 'n', 'b.nid = n.nid');
+      $query->join('menu_links', 'ml', 'b.mlid = ml.mlid');
+      $query->addField('n', 'type', 'type');
+      $query->addField('n', 'title', 'title');
+      $query->fields('b');
+      $query->fields('ml');
+      $query->condition('n.nid', $nids, 'IN');
+      $query->condition('n.status', 1);
+      $query->orderBy('ml.weight');
+      $query->orderBy('ml.link_title');
+      $query->addTag('node_access');
+      $result2 = $query->execute();
+      foreach ($result2 as $link) {
+        $link['href'] = $link['link_path'];
+        $link['options'] = unserialize($link['options']);
+        $this->books[$link['bid']] = $link;
+      }
+    }
+  }

I know this is a direct copy of book_get_books - but $result2 is a weird variable name when there is no $result1 or $result... how about we change it to $book_links?

Also I think this is an oddly named function since it does not load books at all... it creates an array of book links... but I'm okay with fixes the function names in a follow up.

Wow, that was easier than writing the bundle in the first place!

Attached is a patch and interdiff using the new YAML services syntax.

Attached patch and interdiff renames result2 -> book_links.

+Stalking Crell so if this goes green can be RTBC'd again.

Committed/pushed to 8.x, thanks!

For a followup..

I think the commentary can be improved further...

+++ b/core/modules/book/lib/Drupal/book/BookManager.phpundefined
@@ -0,0 +1,82 @@
+/**
+ * Book Manager Service.
+ */
+class BookManager {

Could be "Manages a list of books."

+++ b/core/modules/book/lib/Drupal/book/BookManager.phpundefined
@@ -0,0 +1,82 @@
+  /**
+   * Database Service Object.
+   *
+   * @var \Drupal\Core\Database\Connection
+   */

Could be "The database connection." see Drupal\Core\Cache\DatabaseBackend

+++ b/core/modules/book/lib/Drupal/book/BookManager.phpundefined
@@ -0,0 +1,82 @@
+  /**
+   * Books Array.
+   *
+   * @var array
+   */

"Books Array" not that helpful. How about "Array of books keyed by book id."

+++ b/core/modules/book/lib/Drupal/book/BookManager.phpundefined
@@ -0,0 +1,82 @@
+  /**
+   * Returns an array of all books.
+   *
+   * This list may be used for generating a list of all the books, or for building
+   * the options for a form select.
+   *
+   * @return
+   *   An array of all books.
+   */
+  public function getAllBooks() {

Missing type on @return could be

@return array
  An array of all books keyed by book id. Each value is an array with the following keys: type, type, all the columns from the {book} table and all the columns from the {menu_links} table.

Here's an example of what this method actually returns... isn't it beautiful?
For everyones information... here is an example of the array returned for a single book from BookManager::getAllBooks()

Array
(
    [1] => Array
        (
            [mlid] => 125
            [nid] => 1
            [bid] => 1
            [menu_name] => book-toc-1
            [uuid] => f48a0b16-43c5-40f8-b704-4a524e6bd01b
            [plid] => 0
            [link_path] => node/1
            [router_path] => node/%
            [langcode] => und
            [link_title] => test
            [options] => Array
                (
                )

            [module] => book
            [hidden] => 0
            [external] => 0
            [has_children] => 0
            [expanded] => 0
            [weight] => 0
            [depth] => 1
            [customized] => 0
            [p1] => 125
            [p2] => 0
            [p3] => 0
            [p4] => 0
            [p5] => 0
            [p6] => 0
            [p7] => 0
            [p8] => 0
            [p9] => 0
            [updated] => 0
            [route_name] =>
            [type] => book
            [title] => test
            [href] => node/1
        )

)

+++ b/core/modules/book/lib/Drupal/book/BookManager.phpundefined
@@ -0,0 +1,82 @@
+  /**
+   * Loads Books Array.
+   */
+  protected function loadBooks() {
+    $this->books = array();

Could be... "Loads books array."

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.phpundefined
@@ -0,0 +1,89 @@
+  /**
+   * Book Manager Service.
+   *
+   * @var \Drupal\book\BookManager
+   */
+  protected $bookManager;

The book manager.

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.phpundefined
@@ -0,0 +1,89 @@
+  /**
+   * Injects BookManager Service.
+   */

Could be "Injects the book manager."
And missing @return - for example:

@return \Drupal\book\Controller\BookController
  The book controller

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.phpundefined
@@ -0,0 +1,89 @@
+  /**
+   * Constructs a BookController object.
+   */
+  public function __construct(BookManager $bookManager) {
+    $this->bookManager = $bookManager;
+  }

missing @param

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.phpundefined
@@ -0,0 +1,89 @@
+  /**
+   * Prints a listing of all books.
+   *
+   * @return string
+   *   A HTML-formatted string with the listing of all books content.
+   */
+  public function bookRender() {  

This method doesn't actually print anything.

added follow-up issues section and issue

Removing tags. Crell doesn't need to be stalked anymore.

trying to unstick the stalking tag.

The code cleanup from #60 is being done here #1971206: Cleanup of Book Manager Service

Here is the follow-up #1971412: Improve docs after Convert book_admin_overview and book_render to a new-style Controller
oops. #1971206: Cleanup of Book Manager Service was already made.