Updated: Comment #N

Problem/Motivation

Converting book_outline and it's associated function calls to Form and BookManager Service.

Proposed resolution

Convert functions to methods on classes.

Remaining tasks

API changes

The following public methods were added to BookManager Service:
public function getLinkDefaults($nid)
public function getParentDepthLimit(array $book_link)
public function addFormElements(array $form, array &$form_state, NodeInterface $node, AccountInterface $account)
public function checkNodeIsRemovable(NodeInterface $node)
public function updateOutline(NodeInterface $node)
public function createMenuName($id)
public function updateID(array $book_link)
public function getTableOfContents($bid, $depth_limit, array $exclude = array())

These can be accessed using the following:

<?php
Drupal
::service('book.manager')->methodName($args);
?>

The following protected methods were added to BookManager Service:
protected function addParentSelectFormElements(array $book_link)
protected function recurseTableOfContents(array $tree, $indent, array &$toc, array $exclude, $depth_limit)

These are no longer directly accessible and only usable in BookManager. They are both helper functions for public methods above.

Original report by @Crell

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

Files: 
CommentFileSizeAuthor
#108 interdiff.txt438 bytesdisasm
#108 drupal8.book-module.1938316-108.patch42.77 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,905 pass(es).
[ View ]
#101 drupal8.book-module.1938316-101.patch42.78 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 59,177 pass(es).
[ View ]
#101 interdiff.txt946 bytesjibran
#98 drupal8.book-module.1938316-98.patch42.78 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 59,176 pass(es).
[ View ]
#98 interdiff.txt945 bytesdisasm
#96 drupal8.book-module.1938316-96.patch43.44 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,126 pass(es), 8 fail(s), and 4 exception(s).
[ View ]
#90 drupal8.book-module.1938316-90.patch43.39 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,878 pass(es).
[ View ]
#90 interdiff.txt680 bytesdisasm
#88 drupal8.book-module.1938316-88.patch43.45 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,857 pass(es).
[ View ]
#88 interdiff.txt1.31 KBdisasm
#86 drupal8.book-module.1938316-86.patch44.09 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,505 pass(es).
[ View ]
#86 interdiff.txt680 bytesdisasm
#83 drupal8.book-module.1938316-83.patch44.09 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,137 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#83 interdiff.txt7.58 KBdisasm
#80 drupal8.book-module.1938316-80.patch44.18 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,479 pass(es).
[ View ]
#79 drupal8.book-module.1938316-79.patch43.92 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,415 pass(es).
[ View ]
#79 interdiff.txt539 bytesdisasm
#77 drupal8.book-module.1938316-77.patch43.91 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,429 pass(es).
[ View ]
#77 interdiff.txt1.16 KBdisasm
#75 drupal8.book-module.1938316-75.patch43.9 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,329 pass(es).
[ View ]
#75 interdiff.txt1.25 KBdisasm
#73 drupal8.book-module.1938316-73.patch43.91 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 57,429 pass(es), 8 fail(s), and 4 exception(s).
[ View ]
#67 drupal8.book-module.1938316-67.patch43.92 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 57,863 pass(es), 8 fail(s), and 4 exception(s).
[ View ]
#67 interdiff.txt784 bytesdisasm
#66 drupal8.book-module.1938316-66.patch43.91 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#66 interdiff.txt8.39 KBdisasm
#64 drupal8.book-module.1938316-63.patch39.27 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#64 interdiff.txt581 bytesdisasm
#62 interdiff.txt7.67 KBdisasm
#60 drupal8.book-module.1938316-60.patch39.27 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 57,996 pass(es), 8 fail(s), and 4 exception(s).
[ View ]
#60 interdiff.txt1.48 KBdisasm
#57 drupal8.book-module.1938316-57.patch39.23 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 57,735 pass(es), 8 fail(s), and 4 exception(s).
[ View ]
#57 interdiff.txt6.28 KBdisasm
#49 drupal8.book-module.1938316-49.patch34.88 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,582 pass(es).
[ View ]
#49 interdiff.txt3.94 KBdisasm
#46 drupal8.book-module.1938316-46.patch34 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 57,926 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#46 interdiff.txt472 bytesdisasm
#45 drupal8.book-module.1938316-45.patch34 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#45 interdiff.txt10.4 KBdisasm
#42 book-1938316-42.patch25.5 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,279 pass(es).
[ View ]
#42 interdiff.txt18.46 KBtim.plunkett
#39 1938316-book_outline-39.patch21.78 KBscor
PASSED: [[SimpleTest]]: [MySQL] 57,147 pass(es).
[ View ]
#37 1938316-book_outline-37.patch21.67 KBstella
PASSED: [[SimpleTest]]: [MySQL] 58,422 pass(es).
[ View ]
#37 1938316-29-37-interdiff.txt8.49 KBstella
#29 book_outline1938316-29.patch21.72 KBjaskho
PASSED: [[SimpleTest]]: [MySQL] 57,986 pass(es).
[ View ]
#29 book_outline1938316-19-29.interdiff.txt16.46 KBjaskho
#19 drupal-book_outline-1938316-19-do-not-test.patch8.79 KBjaskho
#19 interdiff.txt608 bytesjaskho
#17 drupal-book_outline-1938316-17.patch8.81 KBjaskho
PASSED: [[SimpleTest]]: [MySQL] 54,511 pass(es).
[ View ]
#17 interdiff.txt1.51 KBjaskho
#14 drupal-book_outline-1938316-14.patch8.91 KBjaskho
PASSED: [[SimpleTest]]: [MySQL] 54,123 pass(es).
[ View ]
#12 drupal-book_outline-1938316-12-do-not-test.patch8.91 KBjaskho
#12 interdiff.txt1.19 KBjaskho
#10 drupal-book_outline-1938316-8-do-not-test.patch9.17 KBjaskho
#6 drupal-book_outline-1938316-6.patch8.84 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 53,188 pass(es).
[ View ]
#6 interdiff.txt945 bytesdisasm
#5 drupal-book_outline-1938316-5.patch8.68 KBjaskho
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-book_outline-1938316-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#3 book-1938316.patch0 bytesjaskho
PASSED: [[SimpleTest]]: [MySQL] 53,167 pass(es).
[ View ]

Comments

Issue tags:+#SprintWeekend

Assigned:Unassigned» jaskho

Status:Active» Needs work
StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 53,167 pass(es).
[ View ]

Posting code in progress.
Have routing working, but not menu.
Remaining tasks that I'm aware of:
* Get hook_menu working
* Page title (currently displays "Error")
* Port (I think) book_remove_button_submit() to BookOutlineForm method
* Port docblocks for BookOutlineForm::buildForm and ::submitForm
* Remove unused code from book.pages.inc

Uh oh, patch is empty. :-( Did you forget to commit to your branch before diffing?

StatusFileSize
new8.68 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-book_outline-1938316-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Trying again with patch of code in progress, a few clean-up pieces added.
* hook_menu is working, but had to re-instate access callback key, which I suspect is not correct.
* still no title; not sure how to handle this. Original code used custom callback to drupal_set_title() then hand off to drupal_get_form().
* still not sure what to do with book_remove_button_submit() - leave in module inc file or refactor to BookOutlineForm::removeButtonSubmit() (e.g.)?

StatusFileSize
new945 bytes
new8.84 KB
PASSED: [[SimpleTest]]: [MySQL] 53,188 pass(es).
[ View ]

attached is a reroll that fixes two space issues and removes ?> at end of file.

jaskho: to apply:
git checkout 8.x
git pull
git branch -D
git checkout -b
git apply -v
git add core
git commit -m 'reroll'

Update... I've been dealing with an untimely system breakdown (vs the timely kind??). Still very much "on" this issue, fwiw.

Status:Needs work» Needs review

Status:Needs review» Needs work

+++ b/core/modules/book/lib/Drupal/book/Form/BookOutlineForm.php
@@ -0,0 +1,129 @@
+   * @TODO: update docblock copied from book_outline_form()
+   * Form constructor for the book outline form.

I don't understand this @todo. But at the very least it should be after the shortdesc and longdesc.

+++ b/core/modules/book/lib/Drupal/book/Form/BookOutlineForm.php
@@ -0,0 +1,129 @@
+  /**
+   * [validateForm description]
+   * @param  array  $form       [description]
+   * @param  array  $form_state [description]
+   * @return [type]             [description]
+   *
+   * @TODO: null implementation or declare class as abstract?
+   */
+  public function validateForm(array &$form, array &$form_state) {
+    // ...
+  }

Null implementation. If the class is abstract it cannot be used. :-)

The todos above need to be resolved, and I don't think we can commit this until we sort out the title and tab-access issues. Blargh.

Posting work in progress (review away, but it might be more efficient to wait since there's a known blocker - see below)

A commit-blocking issue remains unresolved:
hook_menu/access as noted in #5. Any pointers appreciated.

Also just as a note, re moving the "remove" submit handler (book_remove_button_submit) to the form Controller class, it appears FAPI is not ready to handle methods as callbacks (see #1454730: Allow OO methods as FAPI / render API #callbacks), so this will have to wait for another day.

+++ b/core/modules/book/book.moduleundefined
@@ -176,13 +176,14 @@ function book_menu() {
+    'route' => 'book_outline',

This should be 'route_name'. I think that's the main cause of your problem

Fixes 'route' -> 'route_name'.
Still pending upstream issues with registration of menu_links (see http://drupal.org/node/1945418).
Apropos of that, the code in this patch has been successfully tested against patch in http://drupal.org/node/1945418#comment-7211562.

I'm about to disappear into vacation land till April 1, with limited web access. If this is still pending when I return I'll be happy to pick it back up, but between now and then I won't be able to work on it much.

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

This is the patch from #12, renamed so it will test now that the menu/router integration is fixed.

+++ b/core/modules/book/book.pages.incundefined
@@ -101,111 +101,17 @@ function book_export_html(EntityInterface $node) {
+ * @see BookOutlineForm::buildForm()

This needs the full namespace
eg @see \Drupal\book\Form\BookOutlineForm::buildForm()

+++ b/core/modules/book/lib/Drupal/book/Form/BookOutlineForm.phpundefined
@@ -0,0 +1,113 @@
+   * Implements \Drupal\Core\Form\FormInterface::getFormID().
...
+   * Implements \Drupal\Core\Form\FormInterface::buildForm().
...
+   * Implements \Drupal\Core\Form\FormInterface:submitForm()

No need to do this anymore:)
use inheritdocs:
#1906474: [policy adopted] Stop documenting methods with "Overrides …"

+++ b/core/modules/book/lib/Drupal/book/Form/BookOutlineForm.phpundefined
@@ -0,0 +1,113 @@
+  public function validateForm(array &$form, array &$form_state) {
+    // ...
+  }

I think we can just remove this?

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new1.51 KB
new8.81 KB
PASSED: [[SimpleTest]]: [MySQL] 54,511 pass(es).
[ View ]

Includes the recommended changes EXCEPT "remove validateForm()" (per comment #9)

ah i see, it directly implements FormInterface..it should be
public function validateForm(array &$form, array &$form_state) {}
no need for the dots comment:P

StatusFileSize
new608 bytes
new8.79 KB

With null comment removed per #18

+++ b/core/modules/book/book.moduleundefined
@@ -176,13 +176,9 @@ function book_menu() {
-    'access callback' => '_book_outline_access',
-    'access arguments' => array(1),
+++ b/core/modules/book/book.routing.ymlundefined
@@ -4,3 +4,10 @@ book_settings:
+  requirements:
+    _permission: 'administer book outlines'

this is not the same

+++ b/core/modules/book/book.pages.incundefined
@@ -101,111 +101,17 @@ function book_export_html(EntityInterface $node) {
function book_remove_button_submit($form, &$form_state) {

any reason why dont we move this inside the class?

+++ b/core/modules/book/lib/Drupal/book/Form/BookOutlineForm.phpundefined
@@ -0,0 +1,111 @@
+  protected $database;
...
+  public static function create(ContainerInterface $container) {
+    return new static($container->get('database'));
+  }

those are not needed and should be removed

Status:Needs review» Needs work

Thanks rootatwc, good catches. +1 for rootatwc for code reviewer chops:)
Re. moving book_remove_button_submit() into the class, see #10. My analysis may not be correct, but that's what I've been able to come to.

ah i see, sorry did not go through all comments..yes seems FAPI does not accept methods as callbacks:/ meh

jaskho, great job on this patch. It's very clean. However; I think we can move some more functions into this class and/or the BookManager service (if the functions are used across multiple classes).

+++ b/core/modules/book/lib/Drupal/book/Form/BookOutlineForm.phpundefined
@@ -0,0 +1,111 @@
+      $node->book = _book_link_defaults($node->nid);

If this function is specific to this callback put it in this class. Otherwise, add it to BookManager service.

+++ b/core/modules/book/lib/Drupal/book/Form/BookOutlineForm.phpundefined
@@ -0,0 +1,111 @@
+      $node->book['parent_depth_limit'] = _book_parent_depth_limit($node->book);

If this function is specific to this callback put it in this class. Otherwise, add it to BookManager service.

+++ b/core/modules/book/lib/Drupal/book/Form/BookOutlineForm.phpundefined
@@ -0,0 +1,111 @@
+    _book_add_form_elements($form, $form_state, $node);

If this function is specific to this callback put it in this class. Otherwise, add it to BookManager service.

+++ b/core/modules/book/lib/Drupal/book/Form/BookOutlineForm.phpundefined
@@ -0,0 +1,111 @@
+      '#access' => _book_node_is_removable($node),

If this function is specific to this callback put it in this class. Otherwise, add it to BookManager service.

Will do

jasko, are you still working on this?

Yes, I should have a patch today. Thanks for asking.

Still working. I really think I'll have it very soon. I have a good block of time tomorrow, so fingers crossed. I'd like the chance to see this through to the end, but I don't want to be holding anything up. If it would be best for me to let it go, I will.

working... getting close...

Status:Needs work» Needs review
StatusFileSize
new16.46 KB
new21.72 KB
PASSED: [[SimpleTest]]: [MySQL] 57,986 pass(es).
[ View ]

Refactored the following functions from book_manager.module to methods on the BookManager service (per #18):

  • _book_link_defaults() >> BookManager::bookLinkDefaults()
  • _book_parent_depth_limit() >> BookManager::bookParentDepthLimit()
  • _book_add_form_elements() >> BookManager::bookAddFormElements()
  • _book_node_is_removable() >> BookManager::bookNodeIsRemovable()

Updated code in book_form_book_outline_alter() to accommodate API changes.

EDIT:
I'm not confident the below is the best/right way to grab the node object from the $form_state array:
$node = reset($form_state['build_info']['args']);.
As a matter of fact, I'm not confident about setting the title in x_form_alter(). I was originally following another form controller's example, but I don't see any great reason not to set the title in BookOutlineForm::buildForm().

The last submitted patch, book_outline1938316-29.patch, failed testing.

Status:Needs review» Needs work

The failed test was Drupal\user\Tests\UserPasswordResetTest. I have to assume this is an extraneous issue.

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

#29: book_outline1938316-29.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+#SprintWeekend, +WSCCI-conversion

The last submitted patch, book_outline1938316-29.patch, failed testing.

Status:Needs work» Needs review

Passes that test for me locally, resubmitting

#29: book_outline1938316-29.patch queued for re-testing.

+++ b/core/modules/book/lib/Drupal/book/BookManager.phpundefined
@@ -94,4 +95,129 @@ protected function loadBooks() {
+  public function bookLinkDefaults($nid) {
...
+  public function bookParentDepthLimit($book_link) {
...
+  public function bookAddFormElements(&$form, &$form_state, EntityInterface $node) {
...
+  public function bookNodeIsRemovable(EntityInterface $node) {

I think no need to add book in methods name we know these are in book manger so all the methods are related to book.

StatusFileSize
new8.49 KB
new21.67 KB
PASSED: [[SimpleTest]]: [MySQL] 58,422 pass(es).
[ View ]

Patch reroll to change those 4 function names to be without the "book" prefix.

Status:Needs review» Needs work

Nice work. Here's a couple more comments. This is real close though. Thanks for the hard work!

+++ b/core/modules/book/book.moduleundefined
@@ -250,20 +246,8 @@ function _book_outline_access(EntityInterface $node) {
function _book_outline_remove_access(EntityInterface $node) {
-  return _book_node_is_removable($node) && _book_outline_access($node);
-}
-
-/**
- * Determines if a node can be removed from the book.
- *
- * A node can be removed from a book if it is actually in a book and it either
- * is not a top-level page or is a top-level page with no children.
- *
- * @param \Drupal\Core\Entity\EntityInterface $node
- *   The node to remove from the outline.
- */
-function _book_node_is_removable(EntityInterface $node) {
-  return (!empty($node->book['bid']) && (($node->book['bid'] != $node->nid) || !$node->book['has_children']));
+  return Drupal::service('book.manager')->nodeIsRemovable($node)
+    && _book_outline_access($node);

Unless I'm missing something, I think this should be an access checker rather than function.

+++ b/core/modules/book/book.moduleundefined
@@ -877,6 +786,9 @@ function book_node_predelete(EntityInterface $node) {
function book_node_prepare(EntityInterface $node) {
+  // Get BookManager service
+  $book_manager = Drupal::service('book.manager');
+
   // Prepare defaults for the add/edit form.
   if (empty($node->book) && (user_access('add content to books') || user_access('administer book outlines'))) {
     $node->book = array();
@@ -893,7 +805,8 @@ function book_node_prepare(EntityInterface $node) {
@@ -893,7 +805,8 @@ function book_node_prepare(EntityInterface $node) {
       }
     }
     // Set defaults.
-    $node->book += _book_link_defaults(!empty($node->nid) ? $node->nid : 'new');
+    $node_ref = !empty($node->nid) ? $node->nid : 'new';
+    $node->book += $book_manager->linkDefaults($node_ref);
   }
   else {
     if (isset($node->book['bid']) && !isset($node->book['original_bid'])) {
@@ -902,24 +815,11 @@ function book_node_prepare(EntityInterface $node) {
@@ -902,24 +815,11 @@ function book_node_prepare(EntityInterface $node) {
   }
   // Find the depth limit for the parent select.
   if (isset($node->book['bid']) && !isset($node->book['parent_depth_limit'])) {
-    $node->book['parent_depth_limit'] = _book_parent_depth_limit($node->book);
+    $node->book['parent_depth_limit'] = $book_manager->parentDepthLimit($node->book);

Is there a reason this can't be a method on book manager service? If there is, then lets move the meat of this to the book manager service, and just return the output of that function.

+++ b/core/modules/book/lib/Drupal/book/BookManager.phpundefined
@@ -94,4 +95,129 @@ protected function loadBooks() {
+  public function nodeIsRemovable(EntityInterface $node) {

Since a book is always a node, why not just isRemovable() for the function name?

StatusFileSize
new21.78 KB
PASSED: [[SimpleTest]]: [MySQL] 57,147 pass(es).
[ View ]

#37 no longer applied, rerolled.

Status:Needs work» Needs review

Changing status to run tests.

Status:Needs review» Needs work
  1. @@ -94,4 +95,129 @@ protected function loadBooks() {
    +    return MENU_MAX_DEPTH - 1 - (($book_link['mlid'] && $book_link['has_children']) ? entity_get_controller('menu_link')->findChildrenRelativeDepth($book_link) : 0);

    entity_get_controller() should be replaced with an injected service.

  2. @@ -94,4 +95,129 @@ protected function loadBooks() {
    +      '#title' => t('Book outline'),

    The Translation service needs to be injected.

Otherwise I think this is pretty close.

Status:Needs work» Needs review
StatusFileSize
new18.46 KB
new25.5 KB
PASSED: [[SimpleTest]]: [MySQL] 58,279 pass(es).
[ View ]

Here is a major overhaul.

+++ b/core/modules/book/book.moduleundefined
@@ -288,19 +295,18 @@ function book_get_books() {
+  $user = Drupal::currentUser();
   $node = $form_state['controller']->getEntity();
-  $access = user_access('administer book outlines');
+  $access = $user->hasPermission('administer book outlines');
   if (!$access) {
-    if (user_access('add content to books') && ((!empty($node->book['mlid']) && !$node->isNew()) || book_type_is_allowed($node->getType()))) {

This is awesome! Major improvement getting rid of user_access everywhere.

+++ b/core/modules/book/lib/Drupal/book/BookManager.phpundefined
@@ -175,28 +204,28 @@ public function addFormElements(&$form, &$form_state, EntityInterface $node) {
-      foreach (book_get_books() as $book) {
+      foreach ($this->getAllBooks() as $book) {

Not sure how this book_get_books stayed in there as long as it did. Much improved.

+++ b/core/modules/book/lib/Drupal/book/BookManager.phpundefined
@@ -214,10 +244,53 @@ public function addFormElements(&$form, &$form_state, EntityInterface $node) {
+  public function updateBookOutline(NodeInterface $node) {
+    return _book_update_outline($node);

Is this _book_update_outline getting converted in another issue? If so, an @todo would be in place here.

+++ b/core/modules/book/lib/Drupal/book/Form/BookOutlineForm.phpundefined
@@ -88,37 +80,49 @@ public function buildForm(array $form, array &$form_state, EntityBCDecorator $no
+  public function submit(array $form, array &$form_state) {

Finally! I found it odd that we had submitForm as the method name on a form class.

submitForm is actually correct, that's just working around #2022875: Resolve difference between submitForm(), submit(), and save() in EntityFormController

I know of no issue for _book_update_outline().

StatusFileSize
new10.4 KB
new34 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

No reason not to move that method in the book manager service here then. Here's a patch. Also moves book_create_menu and book_update_bid into the service.

StatusFileSize
new472 bytes
new34 KB
FAILED: [[SimpleTest]]: [MySQL] 57,926 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

Foobar... I looked at that interdiff 10 times before uploading, but saw it right away in dreditor. changing book->manager to book_manager.

Status:Needs review» Needs work

The last submitted patch, drupal8.book-module.1938316-46.patch, failed testing.

  1. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
    @@ -39,9 +50,10 @@ class BookManager {
    -  public function __construct(Connection $database, EntityManager $entityManager) {
    +  public function __construct(Connection $database, EntityManager $entityManager, TranslationInterface $translation) {
         $this->database = $database;

    Can we make this $this->connection please?

  2. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
    @@ -94,4 +106,285 @@ protected function loadBooks() {
    +        $parent = $database->query("SELECT * FROM {book} WHERE mlid = :mlid", array(
    ...
    +        $node->book['plid'] = $database->query("SELECT mlid FROM {book} WHERE nid = :nid", array(
    ...
    +        $database->insert('book')
    ...
    +        if ($node->book['bid'] != $database->query("SELECT bid FROM {book} WHERE nid = :nid", array(
    ...
    +    $query = $database->select('menu_links');
    ...
    +      $database->update('book')

    Needs to be $this->database (or $this->connection, ideally)

Status:Needs work» Needs review
StatusFileSize
new3.94 KB
new34.88 KB
PASSED: [[SimpleTest]]: [MySQL] 58,582 pass(es).
[ View ]

thanks for the speedy review tim.plunkett. Attached is a patch addressing comments in #48.

Status:Needs review» Needs work

I think we need two new AccessController.

  1. +++ b/core/modules/book/book.module
    @@ -252,20 +255,8 @@ function _book_outline_access(EntityInterface $node) {
    function _book_outline_remove_access(EntityInterface $node) {

    We should create AccessController and deprecate _book_outline_remove_access.

  2. +++ b/core/modules/book/book.module
    @@ -252,20 +255,8 @@ function _book_outline_access(EntityInterface $node) {
    +    && _book_outline_access($node);

    We should create AccessController and deprecate _book_outline_access.

Status:Needs work» Needs review

book_admin_edit is using that currently, and it's already converted in that patch: #1945390: Convert book_admin_edit to a new-style FormInterface implementation. Switching back to needs review because that's out of the scope of this issue.

Status:Needs review» Needs work

Both are out of scope:

_book_outline_remove_access() will be handled in #1938318: Convert book_remove_form to a new-style Form object.

_book_outline_access() needs to be handled in #1945390: Convert book_admin_edit to a new-style FormInterface implementation, after this issue is in.

Status:Needs work» Needs review

Status crosspost.

Assigned:jaskho» disasm
Status:Needs review» Needs work

Status:Needs work» Needs review

Status:Needs review» Needs work

Ok I take back #50 :). I really want to combine all three in one issue but anyways I have nothing to complain here.

  1. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
    @@ -94,4 +106,285 @@ protected function loadBooks() {
    +  public function addFormElements(array $form, array &$form_state, NodeInterface $node, AccountInterface $account) {

    $form is passed by reference in _book_add_form_elements why not here?

  2. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
    @@ -94,4 +106,285 @@ protected function loadBooks() {
    +    $form['book']['plid'] = _book_parent_select($node->book);

    _book_parent_select should also be the part of bookManger because only _book_add_form_elements and now bookManger::addFormElements calls it.

  3. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
    @@ -94,4 +106,285 @@ protected function loadBooks() {
    +  /**
    +   * Translates a string to the current language or to a given language.
    +   *
    +   * @param string $string
    +   *   A string containing the English string to translate.
    +   * @param array $args
    +   *   An associative array of replacements to make after translation. Based
    +   *   on the first character of the key, the value is escaped and/or themed.
    +   *   See \Drupal\Core\Utility\String::format() for details.
    +   * @param array $options
    +   *   An associative array of additional options, with the following elements:
    +   *   - 'langcode': The language code to translate to a language other than
    +   *      what is used to display the page.
    +   *   - 'context': The context the source string belongs to.
    +   *
    +   * @return string
    +   *   The translated string.
    +   */
    +  protected function t($string, array $args = array(), array $options = array()) {
    +    return $this->translation->translate($string, $args, $options);
    +  }

    Can we add @todo here to remove it when #2018411: Figure out a nice DX when working with injected translation is in.

  4. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
    @@ -94,4 +106,285 @@ protected function loadBooks() {
    +   * @param $book_link
    ...
    +  function updateID($book_link) {

    Typehint missing.

  5. +++ b/core/modules/book/lib/Drupal/book/Form/BookOutlineForm.php
    @@ -0,0 +1,128 @@
    +   * @var \Drupal\node\NodeInterface
    ...
    +  protected $entity;

    NodeInterface and $entity why not $node?

  6. +++ b/core/modules/book/lib/Drupal/book/Form/BookOutlineForm.php
    @@ -0,0 +1,128 @@
    +    $form = $this->bookManager->addFormElements($form, $form_state, $this->entity, $this->getCurrentUser());

    We can remove $form= now we are passing $form by reference to addFormElement see 1.

Status:Needs work» Needs review
StatusFileSize
new6.28 KB
new39.23 KB
FAILED: [[SimpleTest]]: [MySQL] 57,735 pass(es), 8 fail(s), and 4 exception(s).
[ View ]

attached are the changes from #56. jibran also wanted some more details in the book_link @param, but I'm not familiar enough with what it does to add anything beneficial.

Status:Needs review» Needs work

The last submitted patch, drupal8.book-module.1938316-57.patch, failed testing.

1, 6: I purposefully changed addFormElements() to not be by reference, it seemed messy. It should be consistent though.

2: Sure

3: Unless we have a ManagerBase or require 5.4, that code is likely not moving. A @todo is fine.

4: Sure

5: The variable has to be $entity. The typehint in the parent is for EntityInterface, when we override this here it fixes the typehints for the file.

Status:Needs work» Needs review
StatusFileSize
new1.48 KB
new39.27 KB
FAILED: [[SimpleTest]]: [MySQL] 57,996 pass(es), 8 fail(s), and 4 exception(s).
[ View ]

reverting the parts #59 pointed out were bad.

Status:Needs review» Needs work

@disasm You missed #56.2. Rest is fine. Thanks for working on this like a reboot :).
@tim.plunkett Thank you for explaining everything. I didn't get #59.5 but if you think it is not important then it is fine by me.

StatusFileSize
new7.67 KB

Here's an interdiff to #49 (you'll see book_parent_select has been removed in the patch that was tested). Not sure exactly what that last diff was to, but it wasn't very helpful. It might help someone catch what I broke in 60.

  1. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
    @@ -50,10 +57,11 @@ class BookManager {
    +  public function __construct(Connection $connection, EntityManager $entityManager, TranslationInterface $translation, ConfigFactory $configFactory) {

    These should not be camel case.

  2. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
    @@ -50,10 +57,11 @@ class BookManager {
    +    $this->ConfigFactory = $configFactory;

    $this->configFactory = $config_factory;

  3. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
    @@ -387,4 +396,57 @@ function updateID($book_link) {
    +   * @return

    @return array

  4. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
    @@ -387,4 +396,57 @@ function updateID($book_link) {
    +        $form['#prefix'] .= '<em>' . t('This is the top-level page in this book.') . '</em>';
    ...
    +        $form['#prefix'] .= '<em>' . t('This will be the top-level page in this book.') . '</em>';
    ...
    +      $form['#prefix'] .= '<em>' . t('No book selected.') . '</em>';
    ...
    +        '#title' => t('Parent item'),
    ...
    +        '#description' => 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)),

    $this->t()

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

I think I found the problem. had a ConfigFactory where I wanted configFactory.

Status:Needs review» Needs work

The last submitted patch, drupal8.book-module.1938316-63.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.39 KB
new43.91 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Addressing #63. Also moving book_toc and book_toc_recurse to the service. That should be the end of the rabbit hole!

StatusFileSize
new784 bytes
new43.92 KB
FAILED: [[SimpleTest]]: [MySQL] 57,863 pass(es), 8 fail(s), and 4 exception(s).
[ View ]

Try again, had a missing $this->.

Status:Needs review» Needs work

The last submitted patch, drupal8.book-module.1938316-67.patch, failed testing.

Status:Needs work» Needs review

Just trying to verify if this patch really breaks an install related test.

Status:Needs review» Needs work
Issue tags:+#SprintWeekend, +WSCCI-conversion

The last submitted patch, drupal8.book-module.1938316-67.patch, failed testing.

A quick search for this exception, and it looks like it's related to PHP/APC version on the testbot. I'm still unclear why the issue showed up since #49. I moved a good bit of code in book.module to the book service, but none of the functionality was changed. Here's a link to the PHP bug I found:

https://bugs.php.net/bug.php?id=61956

Status:Needs work» Needs review
StatusFileSize
new43.91 KB
FAILED: [[SimpleTest]]: [MySQL] 57,429 pass(es), 8 fail(s), and 4 exception(s).
[ View ]

reroll!

Status:Needs review» Needs work

The last submitted patch, drupal8.book-module.1938316-73.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.25 KB
new43.9 KB
PASSED: [[SimpleTest]]: [MySQL] 58,329 pass(es).
[ View ]

For future reference, That xml error tends to be thrown in tests when a class can't be found. Would be nice if it gave a better error message than that. The issue was with the configFactory stuff added earlier. Changed it to be more like other patches I've seen where the service stores just the config it needs, in this case 'menu.settings'. I called the variable $menuConfig.

This interdiff is a new pattern that I've personally pushed back against. Most of core until this point has kept configFactory around...

StatusFileSize
new1.16 KB
new43.91 KB
PASSED: [[SimpleTest]]: [MySQL] 58,429 pass(es).
[ View ]

switching back to configFactory.

+++ b/core/modules/book/lib/Drupal/book/BookManager.php
@@ -30,6 +34,20 @@ class BookManager {
+  protected $ConfigFactory;

it should be $configFactory.

StatusFileSize
new539 bytes
new43.92 KB
PASSED: [[SimpleTest]]: [MySQL] 58,415 pass(es).
[ View ]

attached patch addresses #78.

StatusFileSize
new44.18 KB
PASSED: [[SimpleTest]]: [MySQL] 58,479 pass(es).
[ View ]

reroll!

Status:Needs review» Needs work

Great work @disasm here is some minor issues.

  1. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
    @@ -94,4 +114,401 @@ protected function loadBooks() {
    +  public function parentDepthLimit($book_link) {
    ...
    +  protected function recurseTableOfContents($tree, $indent, &$toc, $exclude, $depth_limit) {
    ...
    +  public function getTableOfContents($bid, $depth_limit, $exclude = array()) {

    typehint missing.

  2. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
    @@ -94,4 +114,401 @@ protected function loadBooks() {
    +        drupal_static_reset('book_get_books');
    ...
    +          drupal_static_reset('book_get_books');

    This is a function is already converted to BookManger. I don't know we need this or not.

  3. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
    @@ -94,4 +114,401 @@ protected function loadBooks() {
    +  function updateID($book_link) {
    ...
    +  function addParentSelectFormElements($book_link) {

    scope and typehint.

I think we should add all the api changes to issue summary so that core committer can approve api changes. And please add api changes tag after updating the summary. see #1951268: Convert /forum and /forum/% to new router, remove forum_forum_load(), forum_get_topics(), create Forum service for reference.

Status:Needs work» Needs review
StatusFileSize
new7.58 KB
new44.09 KB
FAILED: [[SimpleTest]]: [MySQL] 58,137 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Addressing #1, #2 and #3 in #82. Also, renamed methods to to be more descriptive.

Great work @disasm. I have no more feedback. Let's wait @tim.plunkett for RTBC.

Status:Needs review» Needs work

The last submitted patch, drupal8.book-module.1938316-83.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new680 bytes
new44.09 KB
PASSED: [[SimpleTest]]: [MySQL] 58,505 pass(es).
[ View ]

doh! I swear that looked like an array! Removing type hint on that specific function for array.

doxygen for protected function t() on the BookManager should be the shorter form like shown at: #2079095: Shorten the doxygen for method t() in ControllerBase to the standard version

StatusFileSize
new1.31 KB
new43.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,857 pass(es).
[ View ]

attached fixes doxygen for t(). I left the todo to remove in though.

+++ b/core/modules/book/lib/Drupal/book/BookManager.php
@@ -94,4 +114,383 @@ protected function loadBooks() {
+   *   @todo Remove when https://drupal.org/node/2018411 is in.

This issue is resolved. We try to put this t() method on as many base classes as possible, but for top level classes, this is what we're going to have to do until we require PHP 5.4

So let's just remove the @todo.

StatusFileSize
new680 bytes
new43.39 KB
PASSED: [[SimpleTest]]: [MySQL] 58,878 pass(es).
[ View ]

removing @todo

Status:Needs review» Needs work
Issue tags:-API change, -#SprintWeekend, -WSCCI-conversion

The last submitted patch, drupal8.book-module.1938316-90.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+API change, +#SprintWeekend, +WSCCI-conversion

#90: drupal8.book-module.1938316-90.patch queued for re-testing.

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

Status:Needs review» Reviewed & tested by the community

It would be nice to be able to convert 'node/%node/outline' to a local task plugin, but that's block by node/%node not yet being converted to a route. Same for the patch used for the form redirect - it would be nice to use the UrlGenerator, but that needs to be converted to a route.

I find the concept of a book manager a little funny, but that's already in core, so not an issue for this conversion.

I don't see any other problems except things blocked on other route conversions, so I think this is ready.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Needs review
StatusFileSize
new43.44 KB
FAILED: [[SimpleTest]]: [MySQL] 58,126 pass(es), 8 fail(s), and 4 exception(s).
[ View ]

straight reroll. if it's green back to rtbc.

Status:Needs review» Needs work

The last submitted patch, drupal8.book-module.1938316-96.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new945 bytes
new42.78 KB
PASSED: [[SimpleTest]]: [MySQL] 59,176 pass(es).
[ View ]

lost a - in merge.

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs reroll

Back to RTBC.

Status:Reviewed & tested by the community» Needs work

+++ w/core/modules/book/lib/Drupal/book/Form/BookOutlineForm.php
@@ -0,0 +1,128 @@
+    $form = $this->bookManager->addFormElements($form, $form_state, $this->entity, $this->getCurrentUser());

#2077513: Refactor ControllerBase to be more consistent with FormBase has changed getCurrentUser() to currentUser()

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new946 bytes
new42.78 KB
PASSED: [[SimpleTest]]: [MySQL] 59,177 pass(es).
[ View ]

Back to RTBC as it is minor fix.

Status:Reviewed & tested by the community» Needs work
Issue tags:-API change, -#SprintWeekend, -WSCCI-conversion

The last submitted patch, drupal8.book-module.1938316-101.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal8.book-module.1938316-101.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+API change, +#SprintWeekend, +WSCCI-conversion

Status:Needs review» Reviewed & tested by the community

minor fix, and the failure was RandomTest, unrelated testbot quirk.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/book/book.module
@@ -190,13 +197,9 @@ function book_menu() {
     'type' => MENU_LOCAL_TASK,

Sorry to point this out, but since this issue was originally started, we now have a means of migrating MENU_LOCAL_TASKs as well, so we should take care of that here. https://drupal.org/node/2044515 has the details.

+++ b/core/modules/book/book.module
@@ -343,145 +334,7 @@ function book_pick_book_nojs_submit($form, &$form_state) {
+=======

That looks wrong. :)

I didn't get through the rest of the patch yet, but those are enough to mark it needs work for now. Keep up the great work!

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new42.77 KB
PASSED: [[SimpleTest]]: [MySQL] 58,905 pass(es).
[ View ]
new438 bytes

@webchick, thanks for the comments.

I removed the ========. As for the conversion, this is impossible. According to the change notice:
In order for a local task to be converted, both it and the path it appears on must first be converted to routes.

So pretty much every dependent task needs to be converted before any local tasks can be converted. In this case, it's node_page_view that is preventing this from using local_tasks.yml.

As such, I would highly suggest waiting until the old menu router is gone before requiring conversions to local_tasks.yml.

As such, moving back to RTBC for webchick's approval.

Status:Reviewed & tested by the community» Fixed
Issue tags:+Needs tests

Ok, at long last!

Committed and pushed to 8.x. Thanks so much!! :D

Can we get a (major) follow-up to figure out why that ==== didn't fail tests?

It was inside a comment.

/**
* This is valid.
============= This is also valid
*/

Issue tags:-Needs tests

D'oh. Right. :)

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

Issue summary:View changes

Adding issue summary.