Remove theme_book_link, make book tree align with MenuLinkTree build

@ashwinikumar, are you still working on this?

Trying to get this closer to the MenuLinkTree.php (#pairprogramming with @sqndr)

Nice pair programming with joelpittet!

This looks really good, nice work you two!

+++ b/core/modules/update/src/Tests/UpdateUploadTest.php
@@ -7,6 +7,7 @@
+use Drupal\Core\Updater\Updater;

@@ -111,4 +112,18 @@ function testUpdateManagerCoreSecurityUpdateMessages() {
+
+  /**
+   * Tests that modules with only module info.yml files are detected without
+   * a module file.
+   * @covers Module::canUpdateDirectory().
+   */
+  function testUpdateDirectory() {
+    $type = Updater::getUpdaterFromDirectory(\Drupal::root() . '/core/modules/update/tests/modules/aaa_update_test');
+    $this->assertEqual($type, 'Drupal\\Core\\Updater\\Module', 'Detected a Module');
+
+    $type = Updater::getUpdaterFromDirectory(\Drupal::root() . '/core/modules/update/tests/themes/update_test_basetheme');
+    $this->assertEqual($type, 'Drupal\\Core\\Updater\\Theme', 'Detected a Theme.');
+  }
+

Oops :)

@Cottser thanks for spotting that so quickly!

@Manuel Garcia Because it's in context and easy to change (mostly because of the class in classy) We'd like to abstract it out a bit but I think that's game for a follow-up for sure! @sqndr was thinking that yesterday too.

Would you mind opening a follow up for that @Manuel Garcia?

Thanks for creating the follow up @Manuel! Better to keep issue small in my opinion.

@joelpittet: High five for the pair programming once more! ;)

Added an API change. We still need to
- add a beta phase evaluation

+++ b/core/modules/book/book.module
@@ -62,13 +62,9 @@ function book_theme() {
     'book_navigation' => array(
       'variables' => array('book_link' => NULL),
     ),
...
+    'book_tree' => [
+      'variables' => ['items' => [], 'attributes' => []],
+    ],

Hm, didn't tim.plunkett had a comment about this at the sprints? Shouldn't we refactor the rest of the code in that function to the short syntax [] (instead of the long array()) as well?

Changed to the short syntax to keep the code consistent.

Trying to update the beta phase evaluation.

#2483181: Make book navigation block cacheable is blocked on this, tagging as blocker, and bumping to major because of that.

1. +++ b/core/modules/book/src/BookManagerInterface.php
   @@ -254,11 +254,8 @@ public function deleteFromBook($nid);
   +  public function build(array $tree, $level = 0);
   

   I don't think the $level = 0 should be part of the interface?

   Look at how public MenuLinkTree::build() and protected MenuLinkTree::buildItems() work together to have cleaner code, and avoid polluting the public API with internal details.

2. +++ b/core/modules/book/src/Plugin/Block/BookNavigationBlock.php
   --- a/core/modules/book/templates/book-tree.html.twig
   +++ b/core/modules/book/templates/book-tree.html.twig
   

   Interestingly, this template is now functionally identical to menu.html.twig…

   Couldn't we just choose to import that template and call it a day?

   That still allows themes to have completely different markup for books versus menus, but prevents the need for duplication.

3. +++ b/core/themes/classy/templates/navigation/book-tree.html.twig
   @@ -1,14 +1,44 @@
   +      <ul{{ attributes.addClass('menu') }}>
   

   AFAICT this is the only difference. Can't we reorganize things to avoid duplicating everything?

For 2 and 3: #2490308: Refactor the macro on menu.html.twig for reusability

You keep linking to awesome issues, @Cottser :D Thanks!

@Wim Leers we copied MenuLinkTree::build() almost verbatim when we did this... it had $level in there. That was likely there because before that data was used to build up first and last classes I believe.

We can take that out though now as it looks like that method has changed since a couple weeks ago and we don't need the $level because it's in the macro.

#27: no, $level is definitely being added in this patch.

@sqndr Yes Tim was saying he wanted to keep it consistent but we avoid making changes outside of the diffs that we are already making so that it's easy to review the changes that are relevant to the patch and avoid conflicts with other patches that may be touching that part of the code (less unnessasary re-rolls).

I'm trying to convert the syntax bit by bit, the inconsistency is not a big concern. More discussion here #2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8.

@Wim Leers I'm telling the truth. Hash 8e866ef

That change was introduce by one of the issues you worked on: #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees

I think I got the gist of the changes minus the cachable meta data stuff as that is out of scope on this issue.

Whoops left a level in there.

A mix of short array vs old syntax should't be done.

@hass it is done... everywhere in core. Read and contribute to the policy #2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8

But you're right I shouldn't mix in the same patch (within the hunks i'm changing). So thanks @hass (though not sure you meant that?) that triggered me to find it.

Also had a double period typo.

#29: oh, yes, sure, it used to be in MenuLinkTree. But this patch doesn't touch that. This patch touches the equivalent Book module code. And it indeed got removed in that patch that I worked on, precisely for the reasons I mentioned: it wasn't on the interface, so it never should've been there. (And I was also the one to introduce it IIRC, and that was wrong.) I'm only asking to 1) not make the same mistake here, 2) adopt the same pattern that HEAD's MenuLinkTree has :)

#31: splendid!

+++ b/core/modules/book/src/BookManager.php
@@ -500,21 +501,49 @@ public function getActiveTrailIds($bid, $link) {
+   * @throws \DomainException

This is c/p'ed from MenuLinkTree, but this code doesn't actually throw this exception. Can be deleted :)

IMO once this patch is green, it's ready to go. Great clean-up.

+++ b/core/modules/book/src/BookManagerInterface.php
@@ -254,11 +254,8 @@ public function deleteFromBook($nid);
+  public function build(array $tree);

Perhaps an @see \Drupal\Core\Menu\MenuLinkTree::build() is useful, to make the reader aware of the similarity?

@Wim Leers yeah I was just mentioning we tried copying, it wasn't introducing anything new explicitly. The "end goal" would be to maybe extend from MenuLinkTree and avoid the duplication all together... but baby steps, we'll get there.

Local tests weren't happening this morning... so I just guess and cleaned up the items from #39

Looks great!

Just found a couple docs fixes.

1. +++ b/core/themes/classy/templates/navigation/book-tree.html.twig
   @@ -1,14 +1,44 @@
   - * Theme override for a book tree.
   + * Default theme implementation to display a book tree.
   ...
   - * @see template_preprocess_book_tree()
   + * @ingroup themeable
   

   Minor: Classy templates shouldn't say "Default theme implementation" or say "@ingroup themeable" because they are overrides.

2. +++ b/core/modules/book/templates/book-tree.html.twig
   @@ -1,16 +1,44 @@
   + *   - url: The book link url, instance of \Drupal\Core\Url
   
   +++ b/core/themes/classy/templates/navigation/book-tree.html.twig
   @@ -1,14 +1,44 @@
   + *   - url: The book link url, instance of \Drupal\Core\Url
   

   Missing period.

That's all I could catch, so fixing and leaving at RTBC.

Sorry, can't resist this fix (url → URL). Manually edited interdiff is from #40.

Yes, i mean inconsistency in one function. I know core is mixed, but it is consistent per file or function.

@hass thank you for clarifying, it's hard to know sometimes without a reference context lines.

Thanks for the doc cleanup @Cottser!

Nice work! :)

+++ b/core/modules/book/book.module
@@ -62,13 +62,9 @@ function book_theme() {
     'book_navigation' => array(
       'variables' => array('book_link' => NULL),
     ),
-    'book_tree' => array(
-      'render element' => 'tree',
-    ),
-    'book_link' => array(
-      'render element' => 'element',
-      'function' => 'theme_book_link',
-    ),
+    'book_tree' => [
+      'variables' => ['items' => [], 'attributes' => []],
+    ],
     'book_export_html' => array(
       'variables' => array('title' => NULL, 'contents' => NULL, 'depth' => NULL),
     ),
@@ -500,37 +496,6 @@ function template_preprocess_book_node_export_html(&$variables) {

This is still a mix of old and new array style and this in just one array. We should not do this I think.

I asked the same question in #21. See the answer in #29. And again in #35.

Feel free to change it if you feel strongly about it. I'll not block this from being committed on that.

Though I'd rather it change.

1. +++ b/core/modules/book/book.module
   @@ -62,13 +62,9 @@ function book_theme() {
        'book_navigation' => array(
          'variables' => array('book_link' => NULL),
        ),
   -    'book_tree' => array(
   -      'render element' => 'tree',
   -    ),
   -    'book_link' => array(
   -      'render element' => 'element',
   -      'function' => 'theme_book_link',
   -    ),
   +    'book_tree' => [
   +      'variables' => ['items' => [], 'attributes' => []],
   +    ],
        'book_export_html' => array(
          'variables' => array('title' => NULL, 'contents' => NULL, 'depth' => NULL),
        ),
   

   Yep mixing array styles within an array is not nice - this should only be using array()

2. +++ b/core/modules/book/src/BookManagerInterface.php
   @@ -255,10 +255,9 @@ public function deleteFromBook($nid);
   -  public function bookTreeOutput(array $tree);
   +  public function build(array $tree);
   

   I'm not sure about the new method name of BookManager::build() - how about buildTree since it is generating something using a book tree template. In fact why rename it? It does not look necessary. And not renaming it would make this patch really only change the theme layer.

@lauriii and @alexpott the name was chosen so that we can potentially merge the classes between MenuLinkTree class which has nearly identical logic. Set BookManager to extend MenuLinkTree or something later.

@joelpittet but if we do that at some point in the future, we can do it using a bc layer.

Sure, why not. I just wanted to keep them close inline with each other as possible. BC layers just extra crud laying about to me.

Just want to unblock and move forward.

Nice work! Hopefully we can get this in now!

Committed 24bd910 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

Thanks, unlocked and unblocked caching level!

Followup.