Convert the book_admin_overview page callback to a new-style Controller, using the instructions on http://drupal.org/node/1800686

Follow-up Issues

Files: 
CommentFileSizeAuthor
#56 drupal-book_admin_overview-1938296-56.patch8.91 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 54,348 pass(es).
[ View ]
#56 interdiff.txt727 bytesdisasm
#55 drupal-book_admin_overview-1938296-55.patch8.91 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 54,368 pass(es).
[ View ]
#55 interdiff.txt1.17 KBdisasm
#51 drupal-book_admin_overview-1938296-51.patch9.49 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 54,284 pass(es).
[ View ]
#51 interdiff-46-51.txt533 bytesYesCT
#46 drupal-book_admin_overview-1938296-45.patch9.46 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 54,095 pass(es).
[ View ]
#46 interdiff.txt941 bytesdisasm
#40 drupal-book_admin_overview-1938296-40.patch9.47 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 53,360 pass(es).
[ View ]
#40 interdiff.txt2.45 KBdisasm
#39 drupal-book_admin_overview-1938296-39.patch59.94 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#39 interdiff.txt2.45 KBdisasm
#35 drupal-book_admin_overview-1938296-35.patch9.32 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 53,006 pass(es).
[ View ]
#35 interdiff.txt2.52 KBdisasm
#28 drupal-book_admin_overview-1938296-28.patch8.99 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-book_admin_overview-1938296-28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#28 interdiff.txt2.76 KBdisasm
#26 drupal-book_admin_overview-1938296-26.patch8.9 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 53,045 pass(es).
[ View ]
#26 interdiff.txt950 bytesdisasm
#25 drupal-book_admin_overview-1938296-25.patch8.86 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 53,122 pass(es).
[ View ]
#25 interdiff.txt5.3 KBdisasm
#22 drupal-book_admin_overview-1938296-22.patch8.82 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 53,071 pass(es).
[ View ]
#22 interdiff.txt2.7 KBdisasm
#19 drupal-book_admin_overview-1938296-19.patch7.04 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 53,069 pass(es).
[ View ]
#18 drupal-book_admin_overview-1938296-17.patch6.92 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#18 interdiff.txt451 bytesdisasm
#16 drupal-book_admin_overview-1938296-16.patch6.91 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-book_admin_overview-1938296-16.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#16 interdiff.txt1.49 KBdisasm
#15 drupal-book_admin_overview-1938296-15.patch6.83 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 52,693 pass(es).
[ View ]
#15 interdiff.txt591 bytesdisasm
#13 drupal-book_admin_overview-1938296-13.patch6.83 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/book/lib/Drupal/book/Controller/BookController.php.
[ View ]
#13 interdiff.txt4.03 KBdisasm
#11 drupal-book_admin_overview-1938296-11.patch3.83 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 52,587 pass(es).
[ View ]
#11 interdiff.txt517 bytesdisasm
#9 drupal-book_admin_overview-1938296-9.patch3.81 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#9 interdiff.txt1.05 KBdisasm
#8 drupal-book_admin_overview-1938296-8.patch4.15 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/book/lib/Drupal/book/Controller/BookController.php.
[ View ]
#8 interdiff.txt494 bytesdisasm
#2 drupal-book_admin_overview-1938296-1.patch4.14 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Comments

DamienMcKenna’s picture

Issue tags:+SprintWeekend2013
disasm’s picture

Issue tags:-SprintWeekend2013
StatusFileSize
new4.14 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

disasm’s picture

Status:Active» Needs review
DamienMcKenna’s picture

Issue tags:+SprintWeekend2013
Crell’s picture

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.php
@@ -0,0 +1,67 @@
+  public function __construct(Connection $database) {
+    $this->database = $database;

We're not actually using the database in this controller, so we don't need this dependency. (Yes, the example on the change notice has it, because I just needed an example.)

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.php
@@ -0,0 +1,67 @@
+  foreach (book_get_books() as $book) {

However, book_get_books() should turn into a service of its own I suspect. Then *that* can be a dependent service of this controller.

The service that book_get_book() becomes will, I think, have a DB dependency.

Crell’s picture

Status:Needs review» Needs work
Crell’s picture

Also, stupid me, "route_name" is the key, not "route".

disasm’s picture

StatusFileSize
new494 bytes
new4.15 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/book/lib/Drupal/book/Controller/BookController.php.
[ View ]

route -> route_name

disasm’s picture

StatusFileSize
new1.05 KB
new3.81 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

removing database dependency in controller

disasm’s picture

Status:Needs work» Needs review
disasm’s picture

StatusFileSize
new517 bytes
new3.83 KB
PASSED: [[SimpleTest]]: [MySQL] 52,587 pass(es).
[ View ]

adding return of static() to create method.

Crell’s picture

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.php
@@ -0,0 +1,58 @@
+  foreach (book_get_books() as $book) {

book_get_books() should either turn into a protected method on this class or a separate service that we inject, depending on whether it's useful elsewhere. I think it is, which means service.

That should be an easy enough refactor that we can/should go ahead and do it while we're here.

If you disagree :-), then we should remove the constructor as it's not doing anything.

disasm’s picture

Status:Needs work» Needs review
StatusFileSize
new4.03 KB
new6.83 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/book/lib/Drupal/book/Controller/BookController.php.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, drupal-book_admin_overview-1938296-13.patch, failed testing.

disasm’s picture

StatusFileSize
new591 bytes
new6.83 KB
PASSED: [[SimpleTest]]: [MySQL] 52,693 pass(es).
[ View ]

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

disasm’s picture

StatusFileSize
new1.49 KB
new6.91 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-book_admin_overview-1938296-16.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, drupal-book_admin_overview-1938296-16.patch, failed testing.

disasm’s picture

Status:Needs work» Needs review
StatusFileSize
new451 bytes
new6.92 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Added use PDO to BookManager class.

disasm’s picture

StatusFileSize
new7.04 KB
PASSED: [[SimpleTest]]: [MySQL] 53,069 pass(es).
[ View ]

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

Status:Needs review» Needs work
Issue tags:-SprintWeekend2013, -WSCCI-conversion

The last submitted patch, drupal-book_admin_overview-1938296-19.patch, failed testing.

disasm’s picture

Status:Needs work» Needs review
Issue tags:+SprintWeekend2013, +WSCCI-conversion
disasm’s picture

StatusFileSize
new2.7 KB
new8.82 KB
PASSED: [[SimpleTest]]: [MySQL] 53,071 pass(es).
[ View ]

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

disasm’s picture

Title:Convert book_admin_overview to a new-style Controller» Convert book_admin_overview and book_render to a new-style Controller
Crell’s picture

+++ b/core/modules/book/lib/Drupal/book/Service/BookManager.php
@@ -0,0 +1,66 @@
+namespace Drupal\book\Service;

We don't really have a convention for putting services in \Service at this point. Whether we should or not I don't know, but we can probably skip that for now.

+++ b/core/modules/book/lib/Drupal/book/Service/BookManager.php
@@ -0,0 +1,66 @@
+  /**
+   * Database Service Object.
+   */
+  protected $database;

This needs a @var line to specify what the class of the variable is.

disasm’s picture

StatusFileSize
new5.3 KB
new8.86 KB
PASSED: [[SimpleTest]]: [MySQL] 53,122 pass(es).
[ View ]

Attached addresses comments in #24.

disasm’s picture

StatusFileSize
new950 bytes
new8.9 KB
PASSED: [[SimpleTest]]: [MySQL] 53,045 pass(es).
[ View ]

Fixed doctags to have full class instead of object.

Crell’s picture

Status:Needs review» Needs work
+++ b/core/modules/book/lib/Drupal/book/BookManager.php
@@ -0,0 +1,68 @@
+   * Database Service Object.
+   *
+   * @var object
+   */
+  protected $database;

Er, no. :-) The @var needs to be \Drupal\Core\Database\Connection, so that IDEs can auto-complete it properly.

+++ b/core/modules/book/lib/Drupal/book/BookManager.php
@@ -0,0 +1,68 @@
+    $all_books = &drupal_static(__FUNCTION__);

Eep. No. No drupal_static inside of classes. No.

The way to do that in an object is with an object property.

protected $books;

Then have a protected loadBooks() method, which populates that property.

Then in getAllBook():

if (!$this->books) {
$this->loadBooks();
}
return $this->books;

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.php
@@ -0,0 +1,82 @@
+   * @var object
+   */
+  protected $bookManager;

Same comment here about @var.

disasm’s picture

StatusFileSize
new2.76 KB
new8.99 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-book_admin_overview-1938296-28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

attached removes static drupal function from BookManager.

disasm’s picture

Status:Needs work» Needs review
Crell’s picture

Status:Needs review» Reviewed & tested by the community

If the bot's happy, I think we're good here.

Committers: Please be sure to credit both disasm and robmc!

aspilicious’s picture

Status:Reviewed & tested by the community» Needs work
+++ 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

bookmarvel’s picture

Status:Needs work» Needs review
bookmarvel’s picture

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

Status:Needs review» Needs work

The last submitted patch, drupal-book_admin_overview-1938296-28.patch, failed testing.

disasm’s picture

Status:Needs work» Needs review
StatusFileSize
new2.52 KB
new9.32 KB
PASSED: [[SimpleTest]]: [MySQL] 53,006 pass(es).
[ View ]

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.

Crell’s picture

Status:Needs review» Reviewed & tested by the community

Let's add a follow-up issue to finish converting from book_get_books() to BookManager, as this can be a great example of "old to new" conversion.

Committers: Please be sure to credit both disasm and robmc! :-)

YesCT’s picture

Status:Reviewed & tested by the community» Needs work
+++ 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

bookmarvel’s picture

Assigned:Unassigned» bookmarvel

ill make a new patch for those concerns

disasm’s picture

Status:Needs work» Needs review
StatusFileSize
new2.45 KB
new59.94 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

disasm’s picture

StatusFileSize
new2.45 KB
new9.47 KB
PASSED: [[SimpleTest]]: [MySQL] 53,360 pass(es).
[ View ]

forgot to rebase. here's the real patch.

disasm’s picture

Assigned:bookmarvel» disasm
Crell’s picture

Status:Needs review» Reviewed & tested by the community
xjm’s picture

ParisLiakos’s picture

+++ 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

webchick’s picture

Status:Reviewed & tested by the community» Needs work

Yep.

disasm’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new941 bytes
new9.46 KB
PASSED: [[SimpleTest]]: [MySQL] 54,095 pass(es).
[ View ]

Attached patch makes suggested change in 44.

disasm’s picture

Status:Reviewed & tested by the community» Needs review
ParisLiakos’s picture

Status:Needs review» Reviewed & tested by the community
YesCT’s picture

+++ 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?

Crell’s picture

No, because other code that's not in scope for this patch is also calling that function. That would be unwanted scope creep.

YesCT’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new533 bytes
new9.49 KB
PASSED: [[SimpleTest]]: [MySQL] 54,284 pass(es).
[ View ]

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.

Crell’s picture

Status:Needs review» Reviewed & tested by the community
alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ 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.

Crell’s picture

This will need rerolling anyway to use yaml services. *sigh*

disasm’s picture

Status:Needs work» Needs review
StatusFileSize
new1.17 KB
new8.91 KB
PASSED: [[SimpleTest]]: [MySQL] 54,368 pass(es).
[ View ]

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

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

disasm’s picture

StatusFileSize
new727 bytes
new8.91 KB
PASSED: [[SimpleTest]]: [MySQL] 54,348 pass(es).
[ View ]

Attached patch and interdiff renames result2 -> book_links.

disasm’s picture

Issue tags:+Stalking Crell

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

Crell’s picture

Status:Needs review» Reviewed & tested by the community

I have been Stalked!

I'll update the conversion docs, too, since this issue is our example case.

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

alexpott’s picture

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.

alexpott’s picture

Issue summary:View changes

added follow-up issues section and issue

disasm’s picture

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

YesCT’s picture

trying to unstick the stalking tag.

alexpott’s picture

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

YesCT’s picture

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary:View changes

Updated issue summary.