book.module - Convert PHPTemplate templates to Twig

i'll give this a go!

alrighty!! i've added the twig template files and tested it and the markup and functionality both look pretty much 100%. have checked through the templates and A/B compared them to the tpl.phps and it's looking pretty much smooth.

the last thing that needs to be done here is converting theme_book_admin_table in book.admin.inc to be split into a preprocess function and a separate twig template file.

see corresponding issue in twig for the theme function conversion: http://drupal.org/node/1912660

This looks like a great start, thanks @meeli. Sending to testbot.

Just some notes after reviewing the patch a bit closer, thanks again @meeli. The test failures just seem to be about missing previous/next links. I haven't had a chance to run the tests locally yet.

+++ b/core/modules/book/templates/book-all-books-block.html.twigundefined
@@ -0,0 +1,24 @@
+ *   - menu: @TODO

+++ b/core/modules/book/templates/book-node-export-html.html.twigundefined
@@ -0,0 +1,26 @@
+ * - node: @TODO

These will need to be filled out.

+++ b/core/modules/book/templates/book-all-books-block.html.twigundefined
@@ -0,0 +1,24 @@
+ * @see template_preprocess
+ * @see template_preprocess_book_all_books_block

+++ b/core/modules/book/templates/book-export-html.html.twigundefined
@@ -0,0 +1,51 @@
+ * @see template_preprocess
+ * @see template_preprocess_book_export_html

+++ b/core/modules/book/templates/book-navigation.html.twigundefined
@@ -0,0 +1,59 @@
+ * @see template_preprocess
+ * @see template_preprocess_book_navigation

+++ b/core/modules/book/templates/book-node-export-html.html.twigundefined
@@ -0,0 +1,26 @@
+ * @see template_preprocess
+ * @see template_preprocess_book_node_export_html

These @see references should end in parens (i.e. @see template_preprocess() per http://drupal.org/node/1354#see.

Unassigning since it's been a while. @meeli - If you'd like to take this up again, feel free to re-assign to yourself.

I'm going to give this one a go. It looks like book_admin_table can be replaced with a render array with a #table element, I might attempt to remove that theme function too.

Here's the first pass at reworking this patch. Fixes failing tests (mostly whitespace issues) and updates doc blocks for preprocess functions to new standards.

I'm still working on converting book_admin_table to a #theme render array.

Converted theme_book_admin_table to a #theme render array. Another theme function bites the dust :).

Note that there's an outstanding issue when reordering book pages: #1933190: Fatal error when reordering book pages in book admin . There's no test coverage for the error, so everything should pass.

Thanks @duellj!
I'm working on re-rolling this now with a few changes.
- twig filters should not have a space before or after the pipe
- there was an extra greater-than symbol in one of the templates
- one of the preprocess functions needs a little tweaking
- docs need to be updated for all templates and preprocess functions (variable definitions added)
And would be nice:
- change the template names not to include "html" or "block" since those are redundant and less words = better :)

meant to change status too

I've updated so patch applies cleanly now, and applied all the changes from #13 (except for the template renames).

how to test, and API changes

Add conversion summary table

Tagging.

Add pointer to test steps

On the advice of Cottser, moving conversion of theme_book_admin_table() to #1968982: Convert book theme tables to table #type

Thanks @duellj, that's much easier to review :)

I haven't done manual testing yet but I found some documentation tweaks, I think as long as manual testing checks out we'll be good to go after the below are updated:

1. +++ b/core/modules/book/book.moduleundefined
   @@ -971,7 +971,9 @@ function book_preprocess_block(&$variables) {
   + * Default template: book-all-books-block.twig.html.
   
   @@ -993,13 +994,14 @@ function template_preprocess_book_all_books_block(&$variables) {
   + * Default template: book-navigation.twig.html.
   
   @@ -1115,15 +1117,15 @@ function book_toc($bid, $depth_limit, $exclude = array()) {
   + * Default template: book-export-html.twig.html
   

   These should be .html.twig, not .twig.html.

2. +++ b/core/modules/book/templates/book-all-books-block.html.twigundefined
   @@ -0,0 +1,24 @@
   + *   - book_id: the parent book ID.
   + *   - menu: an array of top-level book links.
   

   The descriptions of the variables need to be a complete sentence (in this case, start with a capital letter) per http://drupal.org/node/1354#drupal. We're also trying to avoid using 'array', 'string', 'object' in Twig docblocks per Twig coding standards. I suggest "The top-level book links".

3. +++ b/core/modules/book/templates/book-export-html.html.twigundefined
   @@ -0,0 +1,51 @@
   + * - language_rtl: TRUE or FALSE depending on right to left language scripts.
   

   Similar to the above, here's an alternate description (needs to be wrapped at 80 characters per http://drupal.org/node/1354#drupal): "A flag indicating whether the current display language is a right to left language."

4. +++ b/core/modules/book/templates/book-export-html.html.twigundefined
   @@ -0,0 +1,51 @@
   + * - base_url: URL to home page.
   

   "URL to the home page." would be a bit friendlier here.

5. +++ b/core/modules/book/templates/book-export-html.html.twigundefined
   @@ -0,0 +1,51 @@
   +      The given node is /embedded to its absolute depth in a top level
   +      section/. For example, a child node with depth 2 in the hierarchy is
   

   I'm not sure why these slashes are here, I think they can be removed.

6. +++ b/core/modules/book/templates/book-navigation.html.twigundefined
   @@ -0,0 +1,59 @@
   + * - has_links: Flags TRUE whenever the previous, parent or next data has a
   + *   value.
   

   (needs to be wrapped at 80 characters)

   A flag indicating that the previous, parent or next data has a value.

7. +++ b/core/modules/book/templates/book-node-export-html.html.twigundefined
   @@ -0,0 +1,23 @@
   + * - node: Fully loaded node entity.
   

   Let's remove the word entity here per Twig coding standards.

Forgot to move tests over to #1968982: Convert book theme tables to table #type. Tests should pass now. Also implemented documentation tweaks from #19. Thanks for the excellent review Cottser!

A patch would help :)

For some reason the table #type conversion snuck back in, which caused tests to fail. Also fixed comment wrapping from #19.5

Code and documentation wise this looks ready to go, just needs manual testing and markup comparison - testing steps are in the summary thanks to @duellj!

4. Click the 'Printer-friendly version' link at the right of node links to confirm that
-- has 2 lines with class starting with "section-"; the second one is missing an id - see section-error.png

5. Place the 'book navigation' block into a region to confirm that book-all-books-block.html.twig is being used
-- nav id is incorrect - see book-block-error.png

Thanks for the great review dsdardt! This is why we do manual testing.

Patch contains fixes for both errors. For the second fix, had to cast the variable to a string to print the 0 (see #1970960: Twig implementation does not print int(0)). The ids are actually wrong here, but I opened a new issue for that: #1970968: Book navigation block ids don't match book ids.

Thanks @duellj! We just need someone to manually test this one again.

I'm going to test and review this tonight.

Update conversion summary table

Removed API changes and moved (now empty) API changes section down, latest patch doesn't make any API changes.

I added two books with content of varying levels of depth, tested all the templates and compared the markup thoroughly. I only found two *tiny* tweaks so I'm rolling a new patch and marking RTBC.

The tweaks two:

1. Use consistent syntax for range: http://twig.sensiolabs.org/doc/functions/range.html
2. Added missing period at the end of a comment.

Ship it! Fantastic work @duellj :)

#30: 1898050-30.patch queued for re-testing.

Edit: c'mon test bot, not a good time.

That's better.

Update template names in testing steps

The .tpl.php conversions here look good... and the patch still applies to head.

+1

However in line with everything that's being said... we need to profile the patch.

This is next on my list for profiling.

Updated issue summary.

First of all, rolling in two small changes here:

book-export-html.html.twig:

-<!DOCTYPE html >
+<!DOCTYPE html>

The extra space was in the .tpl.php version but I see no reason to keep it.

book-all-books-block.html.twig:

-  {# @todo remove type casting once http://drupal.org/node/1970960 is fixed #}
-  <nav id="book-block-menu-{{ "" ~ book_id }}" class="book-block-menu">
+  <nav id="book-block-menu-{{ book_id }}" class="book-block-menu">

The printing 0 issue has been fixed so the concatenation with an empty string is no longer needed.

Now, on to the profiling results.

Profiling done with Stark theme and APC class loader enabled.

Created 10 book nodes and placed them in a book with multiple levels of nesting.

book-all-books-block and book-navigation conversions

Tested on a node page with both templates in use - book navigation block placed in sidebar and set to always display.

=== 8.x..8.x compared (518eee9367b5d..518eefcaea15f):

ct  : 30,651|30,651|0|0.0%
wt  : 172,916|172,850|-66|-0.0%
cpu : 149,942|150,820|878|0.6%
mu  : 11,349,664|11,349,664|0|0.0%
pmu : 11,466,288|11,466,288|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=518eee9367b5d&...

=== 8.x..book-1898050-38 compared (518eee9367b5d..518ef09a47b3b):

ct  : 30,651|30,728|77|0.3%
wt  : 172,916|172,730|-186|-0.1%
cpu : 149,942|150,453|511|0.3%
mu  : 11,349,664|11,384,360|34,696|0.3%
pmu : 11,466,288|11,501,552|35,264|0.3%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=518eee9367b5d&...

The numbers for this were coming back slightly faster on a consistent basis. Nice!

book-export-html and book-node-export-html (printer friendly view)

Displaying the printer friendly view for 10 book nodes.

=== 8.x..8.x compared (518fb7370421e..518fbcc2155d3):

ct  : 40,348|40,348|0|0.0%
wt  : 221,767|221,620|-147|-0.1%
cpu : 190,300|189,605|-695|-0.4%
mu  : 10,810,784|10,810,784|0|0.0%
pmu : 10,889,360|10,889,360|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=518fb7370421e&...

=== 8.x..book-1898050-38 compared (518fb7370421e..518fbd1297c93):

ct  : 40,348|41,125|777|1.9%
wt  : 221,767|224,920|3,153|1.4%
cpu : 190,300|194,620|4,320|2.3%
mu  : 10,810,784|11,240,096|429,312|4.0%
pmu : 10,889,360|11,315,600|426,240|3.9%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=518fb7370421e&...

Twig is slower in this case, the numbers fluctuated a bit in the 1.1% to 1.5% range but landed on 1.4% several times so I went with that.

I think we need to understand what's going on in book-export-html and book-node-export-html (printer friendly view) and why it is slower.

I can explain that in parts:

We call isset() on the EntityBC DEcorator twice, which seems to add around 0.9 ms already using 11 nodes.

This is perfectly logical (as twig needs to check access to properties) but a problem of EntityBC Decorator, which should einer not call __get on isset() or at least cache the result using pass-thru caching.

The rest is the TWIG subszstem being loaded for the first time (0.9ms) and class loading (0.9ms), which is one time only and legitimate (in other benchmarks this 2ms static overhead is already factored out.)

So I would say despite that EntityBCDecorator bites us, this is fine ...

To make things more even and reduce the static overhead, I propose to do the theme('node') within the template php or like old hook init or somewhere else this is always called even if book_export_html does not go through the normal layers.

This would make things comparable as the twig overhead is factored out. It can be easily seen in http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?symbol=twig_render_... that the overhead of the Twig Init is much more than normally.

Putting back to RTBC for alex to do a call on this.

Thanks,

Fabian

Discussed with alexpott and he is fine with that now.

I have opened: #1998684: EntityBCDecorator::__isset is very slow as follow-up to fix EntityBCDecorator.

The rest is just the one-time twig overhead, so we can go ahead here!

+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch

Update conversion table

Committed f2aa485 and pushed to 8.x. Thanks!