Problem/Motivation

The book navigation markup looks like the menu link tree. Get the two inline with each other.

Proposed resolution

Remaining tasks

Create follow up issue to reuse the macro. (done: #2490308: Refactor the macro on menu.html.twig for reusability)

User interface changes

None

API changes

- theme_book_link() will be gone (part of theme() to Twig)

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because all theme() functions should be replaced with Twig template files.
Issue priority Not critical because without this core will still work, but it's an improvement to the TX.
Files: 
CommentFileSizeAuthor
#51 interdiff.txt3.41 KBlauriii
#51 remove_theme_book_link-2443361-51.patch9.89 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,083 pass(es). View
#43 interdiff-40-43.txt1.36 KBCottser
#43 remove_theme_book_link-2443361-43.patch12.25 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,533 pass(es). View
#42 interdiff.txt1.36 KBCottser
#42 remove_theme_book_link-2443361-42.patch12.25 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,273 pass(es). View
#40 remove_theme_book_link-2443361-40.patch12.28 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,265 pass(es). View
#40 interdiff.txt1.66 KBjoelpittet
#37 interdiff.txt1.46 KBjoelpittet
#37 remove_theme_book_link-2443361-37.patch12.26 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#32 remove_theme_book_link-2443361-32.patch12.28 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#32 interdiff.txt968 bytesjoelpittet
#31 interdiff.txt2.32 KBjoelpittet
#31 remove_theme_book_link-2443361-31.patch12.28 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#29 MenuLinkTree_php_—_core.png75.79 KBjoelpittet
#22 remove_theme_book_link-2443361-15.patch11.71 KBsqndr
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,569 pass(es). View
#22 remove_theme_book_link-2443361-22.patch12.34 KBsqndr
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,589 pass(es). View
#15 remove_theme_book_link-2443361-15.patch11.71 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,358 pass(es). View
#10 remove_theme_book_link-2443361-10.patch11.71 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,367 pass(es), 1 fail(s), and 33 exception(s). View
#1 remove-theme_book_link-2443361-1.patch1.44 KBManuel Garcia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,388 pass(es), 3 fail(s), and 0 exception(s). View

Comments

Manuel Garcia’s picture

Status: Active » Needs review
FileSize
1.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,388 pass(es), 3 fail(s), and 0 exception(s). View

Here is a straight removal patch. Let's see what the bot says...

Manuel Garcia’s picture

Issue summary: View changes
Manuel Garcia’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 1: remove-theme_book_link-2443361-1.patch, failed testing.

ashwinikumar’s picture

Assigned: Unassigned » ashwinikumar
JeroenT’s picture

Assigned: ashwinikumar » JeroenT
Issue tags: +drupaldevdays

@ashwinikumar, are you still working on this?

JeroenT’s picture

Assigned: JeroenT » Unassigned
Issue tags: -drupaldevdays
sqndr’s picture

Assigned: Unassigned » sqndr
sqndr’s picture

Assigned: sqndr » Unassigned
joelpittet’s picture

Status: Needs work » Needs review
FileSize
11.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,367 pass(es), 1 fail(s), and 33 exception(s). View

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

joelpittet’s picture

Title: Remove theme_book_link » Remove theme_book_link, make book tree align with MenuLinkTree build.
Issue summary: View changes
sqndr’s picture

Nice pair programming with joelpittet!

Status: Needs review » Needs work

The last submitted patch, 10: remove_theme_book_link-2443361-10.patch, failed testing.

Cottser’s picture

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
11.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,358 pass(es). View

@Cottser thanks for spotting that so quickly!

Manuel Garcia’s picture

Loving this guys! I really like where this is going.

+++ b/core/modules/book/templates/book-tree.html.twig
@@ -1,16 +1,44 @@
+{% import _self as book_tree %}
+
+{#
+  We call a macro which calls itself to render the full tree.
+  @see http://twig.sensiolabs.org/doc/tags/macro.html
+#}
+{{ book_tree.book_links(items, attributes, 0) }}
+
+{% macro book_links(items, attributes, menu_level) %}
+  {% import _self as book_tree %}
+  {% if items %}
+    {% if menu_level == 0 %}
+      <ul{{ attributes }}>
+    {% else %}
+      <ul>
+    {% endif %}
+      {% for item in items %}
+        <li{{ item.attributes }}>
+          {{ link(item.title, item.url) }}
+          {% if item.below %}
+            {{ book_tree.book_links(item.below, attributes, menu_level + 1) }}
+          {% endif %}
+        </li>
+      {% endfor %}
+    </ul>
+  {% endif %}
+{% endmacro %}

Why are we adding a twig macro here? Do you see this being used elsewhere besides book itself? Perhaps the idea to later on move this to a general macro? We could have this render all trees of links on book, menu etc.

joelpittet’s picture

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

Manuel Garcia’s picture

Thanks @joelpittet for explaining the situation on IRC
Here is the followup (although I don't think its really a follow up, we should do this no matter what imho): #2490308: Refactor the macro on menu.html.twig for reusability

sqndr’s picture

Issue summary: View changes

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

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

sqndr’s picture

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

sqndr’s picture

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

sqndr’s picture

FileSize
12.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,589 pass(es). View
11.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,569 pass(es). View

Changed to the short syntax to keep the code consistent.

sqndr’s picture

Issue summary: View changes

Trying to update the beta phase evaluation.

Wim Leers’s picture

Priority: Normal » Major
Status: Needs review » Needs work
Issue tags: -Needs beta evaluation +blocker

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

Cottser’s picture

Wim Leers’s picture

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

joelpittet’s picture

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

Wim Leers’s picture

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

joelpittet’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
75.79 KB

@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

joelpittet’s picture

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

joelpittet’s picture

FileSize
12.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
2.32 KB

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

joelpittet’s picture

FileSize
968 bytes
12.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Whoops left a level in there.

The last submitted patch, 31: remove_theme_book_link-2443361-31.patch, failed testing.

hass’s picture

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

joelpittet’s picture

@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

Status: Needs review » Needs work

The last submitted patch, 32: remove_theme_book_link-2443361-32.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
12.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
1.46 KB

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.

Status: Needs review » Needs work

The last submitted patch, 37: remove_theme_book_link-2443361-37.patch, failed testing.

Wim Leers’s picture

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.66 KB
12.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,265 pass(es). View

@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

Wim Leers’s picture

Title: Remove theme_book_link, make book tree align with MenuLinkTree build. » Remove theme_book_link, make book tree align with MenuLinkTree build
Status: Needs review » Reviewed & tested by the community

Looks great!

Cottser’s picture

FileSize
12.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,273 pass(es). View
1.36 KB

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.

Cottser’s picture

Issue summary: View changes
FileSize
12.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,533 pass(es). View
1.36 KB

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

hass’s picture

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

joelpittet’s picture

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

Thanks for the doc cleanup @Cottser!

sqndr’s picture

Nice work! :)

hass’s picture

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

sqndr’s picture

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

joelpittet’s picture

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.

alexpott’s picture

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

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
9.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,083 pass(es). View
3.41 KB
joelpittet’s picture

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

alexpott’s picture

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

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

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.

Manuel Garcia’s picture

+1

sqndr’s picture

Nice work! Hopefully we can get this in now!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 24bd910 on 8.0.x
    Issue #2443361 by joelpittet, Cottser, sqndr, lauriii, Manuel Garcia,...
joelpittet’s picture

Thanks, unlocked and unblocked caching level!

hass’s picture

Status: Fixed » Closed (fixed)

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