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:

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

CommentFileSizeAuthor
#108 interdiff.txt438 bytesdisasm
#108 drupal8.book-module.1938316-108.patch42.77 KBdisasm
#101 drupal8.book-module.1938316-101.patch42.78 KBjibran
#101 interdiff.txt946 bytesjibran
#98 drupal8.book-module.1938316-98.patch42.78 KBdisasm
#98 interdiff.txt945 bytesdisasm
#96 drupal8.book-module.1938316-96.patch43.44 KBdisasm
#90 drupal8.book-module.1938316-90.patch43.39 KBdisasm
#90 interdiff.txt680 bytesdisasm
#88 drupal8.book-module.1938316-88.patch43.45 KBdisasm
#88 interdiff.txt1.31 KBdisasm
#86 drupal8.book-module.1938316-86.patch44.09 KBdisasm
#86 interdiff.txt680 bytesdisasm
#83 drupal8.book-module.1938316-83.patch44.09 KBdisasm
#83 interdiff.txt7.58 KBdisasm
#80 drupal8.book-module.1938316-80.patch44.18 KBdisasm
#79 drupal8.book-module.1938316-79.patch43.92 KBdisasm
#79 interdiff.txt539 bytesdisasm
#77 drupal8.book-module.1938316-77.patch43.91 KBdisasm
#77 interdiff.txt1.16 KBdisasm
#75 drupal8.book-module.1938316-75.patch43.9 KBdisasm
#75 interdiff.txt1.25 KBdisasm
#73 drupal8.book-module.1938316-73.patch43.91 KBdisasm
#67 drupal8.book-module.1938316-67.patch43.92 KBdisasm
#67 interdiff.txt784 bytesdisasm
#66 drupal8.book-module.1938316-66.patch43.91 KBdisasm
#66 interdiff.txt8.39 KBdisasm
#64 drupal8.book-module.1938316-63.patch39.27 KBdisasm
#64 interdiff.txt581 bytesdisasm
#62 interdiff.txt7.67 KBdisasm
#60 drupal8.book-module.1938316-60.patch39.27 KBdisasm
#60 interdiff.txt1.48 KBdisasm
#57 drupal8.book-module.1938316-57.patch39.23 KBdisasm
#57 interdiff.txt6.28 KBdisasm
#49 drupal8.book-module.1938316-49.patch34.88 KBdisasm
#49 interdiff.txt3.94 KBdisasm
#46 drupal8.book-module.1938316-46.patch34 KBdisasm
#46 interdiff.txt472 bytesdisasm
#45 drupal8.book-module.1938316-45.patch34 KBdisasm
#45 interdiff.txt10.4 KBdisasm
#42 book-1938316-42.patch25.5 KBtim.plunkett
#42 interdiff.txt18.46 KBtim.plunkett
#39 1938316-book_outline-39.patch21.78 KBscor
#37 1938316-book_outline-37.patch21.67 KBstella
#37 1938316-29-37-interdiff.txt8.49 KBstella
#29 book_outline1938316-29.patch21.72 KBjaskho
#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
#17 interdiff.txt1.51 KBjaskho
#14 drupal-book_outline-1938316-14.patch8.91 KBjaskho
#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
#6 interdiff.txt945 bytesdisasm
#5 drupal-book_outline-1938316-5.patch8.68 KBjaskho
#3 book-1938316.patch0 bytesjaskho
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DamienMcKenna’s picture

Issue tags: +SprintWeekend2013
jaskho’s picture

Assigned: Unassigned » jaskho
jaskho’s picture

Status: Active » Needs work
FileSize
0 bytes

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

Crell’s picture

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

jaskho’s picture

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.)?

disasm’s picture

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'

jaskho’s picture

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

jibran’s picture

Status: Needs work » Needs review
Crell’s picture

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.

jaskho’s picture

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.

tim.plunkett’s picture

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

jaskho’s picture

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.

jaskho’s picture

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.

jaskho’s picture

Status: Needs work » Needs review
FileSize
8.91 KB

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

ParisLiakos’s picture

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

ParisLiakos’s picture

Status: Needs review » Needs work
jaskho’s picture

Status: Needs work » Needs review
FileSize
1.51 KB
8.81 KB

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

ParisLiakos’s picture

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

jaskho’s picture

With null comment removed per #18

ParisLiakos’s picture

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

jaskho’s picture

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.

ParisLiakos’s picture

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

disasm’s picture

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.

jaskho’s picture

Will do

kim.pepper’s picture

jasko, are you still working on this?

jaskho’s picture

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

jaskho’s picture

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.

jaskho’s picture

working... getting close...

jaskho’s picture

Status: Needs work » Needs review
FileSize
16.46 KB
21.72 KB

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.

jaskho’s picture

Status: Needs review » Needs work

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

jaskho’s picture

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

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

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

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

stella’s picture

Status: Needs work » Needs review

Passes that test for me locally, resubmitting

stella’s picture

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

jibran’s picture

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

stella’s picture

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

disasm’s picture

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?

scor’s picture

#37 no longer applied, rerolled.

ygerasimov’s picture

Status: Needs work » Needs review

Changing status to run tests.

Crell’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
18.46 KB
25.5 KB

Here is a major overhaul.

disasm’s picture

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

tim.plunkett’s picture

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().

disasm’s picture

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.

disasm’s picture

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.

tim.plunkett’s picture

  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)

disasm’s picture

Status: Needs work » Needs review
FileSize
3.94 KB
34.88 KB

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

jibran’s picture

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.

disasm’s picture

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.

tim.plunkett’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review

Status crosspost.

disasm’s picture

Assigned: jaskho » disasm
Status: Needs review » Needs work
disasm’s picture

Status: Needs work » Needs review
jibran’s picture

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
6.28 KB
39.23 KB

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.

tim.plunkett’s picture

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
1.48 KB
39.27 KB

reverting the parts #59 pointed out were bad.

jibran’s picture

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.

disasm’s picture

FileSize
7.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.

tim.plunkett’s picture

  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()

disasm’s picture

Status: Needs work » Needs review
FileSize
581 bytes
39.27 KB

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
8.39 KB
43.91 KB

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

disasm’s picture

Try again, had a missing $this->.

Status: Needs review » Needs work

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

Letharion’s picture

Status: Needs work » Needs review

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

Letharion’s picture

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

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

disasm’s picture

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

disasm’s picture

Status: Needs work » Needs review
FileSize
43.91 KB

reroll!

Status: Needs review » Needs work

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

disasm’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
43.9 KB

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.

tim.plunkett’s picture

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

disasm’s picture

switching back to configFactory.

jibran’s picture

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

it should be $configFactory.

disasm’s picture

attached patch addresses #78.

disasm’s picture

reroll!

jibran’s picture

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.

jibran’s picture

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
7.58 KB
44.09 KB

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

jibran’s picture

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
680 bytes
44.09 KB

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

pwolanin’s picture

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

disasm’s picture

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

tim.plunkett’s picture

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

disasm’s picture

removing @todo

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

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

tim.plunkett’s picture

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

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.

pwolanin’s picture

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.

alexpott’s picture

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

Patch no longer applies.

disasm’s picture

Status: Needs work » Needs review
FileSize
43.44 KB

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
945 bytes
42.78 KB

lost a - in merge.

jibran’s picture

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

Back to RTBC.

alexpott’s picture

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()

jibran’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
946 bytes
42.78 KB

Back to RTBC as it is minor fix.

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

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

jibran’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

disasm’s picture

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

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

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!

disasm’s picture

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

webchick’s picture

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?

tim.plunkett’s picture

It was inside a comment.

/**
 * This is valid.
 ============= This is also valid
 */
webchick’s picture

Issue tags: -Needs tests

D'oh. Right. :)

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

Anonymous’s picture

Issue summary: View changes

Adding issue summary.