Convert book theme tables to table #type

#3: 1968982-3-book-table.patch queued for re-testing.

Will need a re-roll after #1938296: Convert book_admin_overview and book_render to a new-style Controller.

#3: 1968982-3-book-table.patch queued for re-testing.

#3: 1968982-3-book-table.patch queued for re-testing.

Patch rerolled.

#11: 1968982-11-book-table.patch queued for re-testing.

Rerolled the patch and fixed some bugs.

Looks pretty good. Maybe we should set the table ID? Previously, it was 'book-outline'.

+++ /dev/null
@@ -1,101 +0,0 @@
-    '#attributes' => array('id' => 'book-outline'),

Rerolled.

Thanks for the re-roll @longwave and it's green nice! There a couple of things and after those I'll give a manual test:

1. +++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
   @@ -117,22 +117,23 @@ public function submitForm(array &$form, array &$form_state) {
   +          $node->log = $this->t('Title changed from %original to %current.', array('%original' => $node->label, '%current' => $values['title']));
   

   I think that is supposed to still be $node->label()

2. +++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
   @@ -117,22 +117,23 @@ public function submitForm(array &$form, array &$form_state) {
   +          $updated = TRUE;
   

   Not sure why this is needed?

3. +++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
   @@ -159,8 +160,25 @@ public function submitForm(array &$form, array &$form_state) {
   +      '#header' => array(t('Title'), t('Weight'), t('Parent'), t('Operations')),
   

   All the t() can be $this->t to use the injected translations.

Fixed all points raised in #18.

Any thoughts on whether we still want the table ID?

If the table ID is not referenced anywhere in CSS or JavaScript, I don't see why we need it.

I think the ID was previously needed only for tabledrag, but not sure we need that any more, since #732022: drupal_add_tabledrag() still using drupal_add_(js|library)(), should return array for #attached was committed.

Conflicting with #2084421: Phase 2 - Decouple book module schema from menu links
I'll attempt a reroll.

+++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
@@ -197,33 +214,76 @@ protected function bookAdminTableTree(array $tree, array &$form) {
+      $form[$id]['operations'] = array(
+        '#type' => 'operations',
+      );
+      $form[$id]['operations']['#links']['view'] = array(

['operations'] conflicted, because the other patch had removed the ['href']. I am not sure if adding this back in breaks what they were trying to do in the other patch.

1. +++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
   @@ -159,8 +159,25 @@ public function submitForm(array &$form, array &$form_state) {
   +          'relationship' => 'parent',
   
   @@ -197,33 +214,76 @@ protected function bookAdminTableTree(array $tree, array &$form) {
   +        '#prefix' => theme('indentation', array('size' => $data['link']['depth'] - 2)),
   

   Change to a render array.

2. +++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
   @@ -197,33 +214,76 @@ protected function bookAdminTableTree(array $tree, array &$form) {
   +        'href' => $data['link']['href'],
   

   Change to route_name?

rerolled ..

On it.

Patch rerolled.

Updated tests, corrected errors from previous patch.

No longer applies, sorry I didn't get to review earlier, will after re-roll.

Rerolled, some route names changed in the theme function in #2278567: Standardize node route names by relationship. Somewhat of an interdiff attached.

I've got a feeling the book module is broken... create one book, no sub pages and save this edit form page:
http://d8.dev/admin/structure/book/1

And it borks up good before and after the patch (slightly different error though, regarding array flipping or missing table index... recoverable fatal). Will need to write a webtest or something, though I think that may need to go into a different issue if one doesn't exist.

Yeah, tried to test this patch but the book module is broken so the book page cannot be viewed.

I was working on this reroll also so we did a bit double work.. But here's the differences from my patch

During manual testing, the operations links were missing, though the text was there. I'm not sure if the other difference is relevant, but I've included a screenshot. @joelpittet, do you happen to know if the [parent] key matters? ie: name="table[book-admin-2][parent][pid]", vs. name="table[book-admin-2][pid]"

Attaching the before and after txt files from patch 46.

After looking at the code, I had these questions:

1. +++ b/core/modules/book/src/Form/BookAdminEditForm.php
   @@ -181,32 +199,91 @@ protected function bookAdminTableTree(array $tree, array &$form) {
   +        'href' => $href = \Drupal::url('entity.node.canonical', array('node' => $nid)),
   

   The $href here is funky.

2. +++ b/core/modules/book/src/Form/BookAdminEditForm.php
   @@ -181,32 +199,91 @@ protected function bookAdminTableTree(array $tree, array &$form) {
   +        $form[$id]['operations']['#links']['edit'] = array(
   +          'title' => $this->t('Edit'),
   +          'route_name' => 'entity.node.edit_form',
   +          'route_parameters' => array('node' => $nid),
   +          'query' => $destination,
   +        );
   +        $form[$id]['operations']['#links']['delete'] = array(
   +          'title' => $this->t('Delete'),
   +          'route_name' => 'entity.node.delete_form',
   +          'route_parameters' => array('node' => $nid),
   +          'query' => $destination,
   +        );
   

   These links no longer work. They are missing the a tags and hrefs when rendered to the page.

Thanks for your review @a-fro! Fixed the links :)

Manual test results below. Looks good to me! RTBC, @joelpittet?

Did a manual test as well, looks clean.

Screenshot after:

#51 shows that we are missing test coverage here.

Added test coverage for the View link. It is currently not possible very easily to test Edit link because it requires 'administer nodes' permission which breaks some other stuff because of changed Save button on nodes. I think this should be fixed on a follow-up.

Interdiff for my patch :)

Little clean up for docs & stuff

+++ /dev/null
@@ -1,100 +0,0 @@
-    '#tabledrag' => array(
-      array(
-        'action' => 'match',
-        'relationship' => 'parent',
-        'group' => 'book-pid',
-        'subgroup' => 'book-pid',
-        'source' => 'book-nid',
-        'hidden' => TRUE,
-        'limit' => BookManager::BOOK_MAX_DEPTH - 2,
-      ),

+++ b/core/modules/book/src/Form/BookAdminEditForm.php
@@ -143,8 +145,25 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+      '#tabledrag' => array(
+        array(
+          'action' => 'match',
+          'relationship' => 'parent',
+          'group' => 'book-pid',
+          'subgroup' => 'book-pid',
+          'source' => 'book-nid',
+          'hidden' => TRUE,
+          'limit' => 7,
+        ),

The 'limited' key is now hardcoded to 7. This should still use the BookManager::BOOK_MAX_DEPTH constant.

fixed #62

The feedback by alexpott in #56 was addressed, setting this back to RTBC and added a beta evalutation.

lauriii asked me about the requirement for 'administer nodes' to use the operations. I suspect that was just done for speed and simplicity rather than checking each nodes. I don't think it should be changed in this patch, but could be examined in a follow-up.

I didn't see this discussed earlier but wanted to point it out as it looks like it's not needed:

+++ b/core/modules/book/src/Form/BookAdminEditForm.php
@@ -107,16 +109,16 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
-            $node = $this->nodeStorage->load($values['nid']);
+            $node = $this->nodeStorage->load($row['#nid']);

Is this change needed, it looks suspicious because we aren't using the row's values anywhere else.

Other than that, I'd leave it as RTBC, though if you want to throw in some short array syntax for good measure that'd be cool too;)

@robmc: did you find find any other solution to fix that? I checked but the 'nid' item doesn't exist in $values array anymore.

@lauriii robmc was working it, he may have a patch to get most of the way there. Essentially the 'nid' was moved onto the 'parent' in the values, and it shouldn't get moved there, which may constitute an API change. So we just need to move that back.

@lauriii here's an attempt, I left the ['parent'] key in the form because those form elements need to be in a column and that one makes sense, just removed the parent key from the #parents within FAPI which preserves the $form_state values and leaves the tests and submit handler mostly alone.

Also did all the array short syntax changes that didn't affect the patch size.

Local tests aren't working well for me on the command line or the UI, but manual testing was fine and the values were the way they should be on the submit handler, so here's hoping testbot agrees;)

While manual testing, noticed that the weights for new pages are negative numbers, whereas in Drupal 7 they are 0. Noticed that when "show weights" is selected, we show the parent column (with nothing in it) but that's the same as Drupal 7. Both are unrelated to this issue, I think.
RTBC+1.

After manually testing this I don't see any regressions. Also code looks good. I also like the fancy array syntax used in the patch ;)

array() is sooooo php5.4.

5.3 but yeah... agreed:P

Committed 0f2348c and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.