custom_block.module - Convert theme_ functions to Twig

Attached patch converts theme_custom_block_block to twig. Doing so introduced an error in BlockTest::testCustomBlock:

    $this->assertEqual(check_markup($custom_block['body[value]'], $config['format']), render($data), 'BlockInterface::build() provides correct block content.');

Since the theme call was moved to a twig template, white space is added around the body value, causing this assertion to fail. I got around this by adding a trim around the render call:

    $this->assertEqual(check_markup($custom_block['body[value]'], $config['format']), trim(render($data)), 'BlockInterface::build() provides correct block content.');

But maybe there's a better way?

custom-block-block.html.twig

There is no reason to have a twig template that just prints {{ body }}.

In fact, I can't confirm that this template even does anything at all. Modifying the new template had no affect on the output for me.

We should get rid of this file and make sure that these blocks are rendered via block.html.twig instead. Let's move the preprocessing (which is really just a call to check_markup($variables['body'], $variables['format']); - not really a pre-processor at all - someplace else, and get rid of the whole hook_theme definition.

custom_block_add_list.html.twig - custom block module
It looks like this patch didn't get as far as this template.

custom_block_add_list.html.twig - seven theme
The seven theme also overrides theme_custom_block_add_list so we'll need to provide a template override instead. I think it needs to be in this patch or tests will break. :/

Here's a re-roll of the existing patch, along with the addition of the two new custom_block_add_list.html.twig files and preprocess functions. (one in custom_block module and one in the seven theme)

I'm still not sure what to do about removing theme_custom_block_block, could use some feedback on where/how to kill that one :)

Added To Test section

@jenlampton

It's not clear to me that theme_custom_block_block() is even used. Neither the preprocess function nor the twig file are called when theming a custom block, and I removed the theme function and tests pass. It looks like a custom block is using 'custom_block' as it's theme, not 'custom_block_block'. Can we just scrap theme_custom_block_block, or should we change it to theme_custom_block?

If we don't scrap it, the doc headers need to be updated to match the new styles.

typo

added blocked by

Nice! I love killing theme functions :)
This version removes theme_custom_block_block conversion, and existence!

#6: core-convert_custom_block_to_twig-1898038-6.patch queued for re-testing.

+++ b/core/modules/block/custom_block/templates/custom-block-add-list.html.twigundefined
@@ -0,0 +1,22 @@
\ No newline at end of file

Newline needed

+++ b/core/themes/seven/templates/custom-block-add-list.html.twigundefined
@@ -0,0 +1,23 @@
\ No newline at end of file

Same

newlines removed.

this looks quite good, but the seven-specific template file never gets used: core/themes/seven/templates/custom-block-add-list.html.twig
instead the module's twig file was selected, even if the theme was seven: core/modules/block/custom_block/templates/custom-block-add-list.html.twig

maybe that behavior should get filed as a separate bug?

@dasjo - I think that is #939462: Specific preprocess functions for theme hook suggestions are not invoked.

Seven's override isn't working because its still using PHPTemplate. This issue will need to be postponed until we can get Seven changed over to use Twig.

Discussion in IRC led to #1938948: Temporarily allow PHPTemplate themes to use .html.twig templates during Twig conversion. So if we can get that in, then we can un-postpone this.

added note about non-blocking, but delaying, issue

#9: core-convert_custom_block_to_twig-1898038-9.patch queued for re-testing.

Rerolled so patch applies. Looks like tests were removed from BlockTest, so no longer need the custom handling for the block body.

Thanks for keeping this going @duellj! Some nitpicks ahead:

+++ b/core/modules/block/custom_block/custom_block.pages.incundefined
@@ -50,22 +50,15 @@ function custom_block_add_page() {
  *
  * @ingroup themeable
  */
-function theme_custom_block_add_list($variables) {
-  $content = $variables['content'];
-  $output = '';
-
-  if ($content) {
-    $output = '<dl class="node-type-list">';
-    foreach ($content as $type) {
-      $output .= '<dt>' . l($type->label(), 'block/add/' . $type->id()) . '</dt>';
-      $output .= '<dd>' . filter_xss_admin($type->description) . '</dd>';
-    }
-    $output .= '</dl>';
+function template_preprocess_custom_block_add_list(&$variables) {

Looks like the preprocess docblock will need to be updated because of the theme -> preprocess conversion here, see #1913208: [policy] Standardize template preprocess function documentation.

+++ b/core/modules/block/custom_block/templates/custom-block-add-list.html.twigundefined
@@ -0,0 +1,22 @@
+ * Default theme implementation to present the content of a custom block.

This needs a @file:

@file
Default theme implementation…

+++ b/core/themes/seven/templates/custom-block-add-list.html.twigundefined
@@ -0,0 +1,23 @@
+ * Overrides custom-block-add-list.html.twig

This needs a @file also, and the comment should end in a period.

+++ b/core/themes/seven/templates/custom-block-add-list.html.twigundefined
@@ -0,0 +1,23 @@
+ * Displays the list of available custom block types for creation.
+ *
+ */

Extra line here can be removed.

cleanup from #17

Thanks @joelpittet, the changes look great. Just one tiny fix:

+++ b/core/modules/block/custom_block/templates/custom-block-add-list.html.twigundefined
@@ -1,6 +1,7 @@
+ * Default theme implementation to present a list of a custom block types.

"list of a custom block types" should be "list of custom block types".

@Cottser, thanks added a couple more.

Thanks @joelpittet! And here's a couple more documentation tweaks :)

depends

More docs tweaks, tagging for manual testing (steps for testing are up in the summary).

seven_custom_block_add_list() is converted both here and in #1938848: seven.theme - Convert PHPTemplate templates to Twig. Can we move it so that it's only converted in the seven issue, reconcile any differences, and have manual testers use Stark to test this issue? At a glance it looks like the conversion here is closer to what we need, except for this possible bug:

+++ b/core/themes/seven/templates/custom-block-add-list.html.twigundefined
@@ -0,0 +1,28 @@
+  {% for type in content %}

{% for type in types %}? The docblock doesn't indicate that content is available to this template.

@Cottser, yes I think {% for type in types %} is what it should be.

Mine. All mine...

Manual testing steps taken:

1. Set Stark to both default and admin theme.
2. Applied patch #22
3. Applied http://drupal.org/node/1898034#comment-7304972 . It applies cleanly. Needed to clear cache, though.
4. Created new custom block and compared html to fresh D8 install. It matches.
5. Added random text to block.html.twig. Verified that the custom block is run through block.html.twig. (NOTE: in OP, please change manual-testing direction #1 to "block.html.twig")
6. Created new block type and compared the html of the list at block/add. It matches.
7. Added random text to custom-block-add-list.html.twig. Verified that the list is run through that twig file.

Also: posted a comment in #1938848: seven.theme - Convert PHPTemplate templates to Twig regarding #23:
http://drupal.org/node/1938848#comment-7305438

Marking as needs work because of #23.

Updated the patch to remove:

diff --git a/core/themes/seven/seven.theme b/core/themes/seven/seven.theme
index fa7124c..a53cfd5 100644
--- a/core/themes/seven/seven.theme
+++ b/core/themes/seven/seven.theme

...

diff --git a/core/themes/seven/templates/custom-block-add-list.html.twig b/core/themes/seven/templates/custom-block-add-list.html.twig
new file mode 100644
index 0000000..fe1d3c3
--- /dev/null
+++ b/core/themes/seven/templates/custom-block-add-list.html.twig

...

And re-rolled a patch for #1938848: seven.theme - Convert PHPTemplate templates to Twig to include those changes.
http://drupal.org/node/1938848#comment-7305534

For the record, the following three patches can be applied cleanly on top of each other:
block: http://drupal.org/node/1898034#comment-7304972
custom_block: http://drupal.org/node/1898038#comment-7305560 (yes, this one)
and
seven: http://drupal.org/node/1938848#comment-7305534

@Shawn DeArmond - thank you! Happy to see this got split and code is looking good :) Minor documentation nitpick ahead:

+++ b/core/modules/block/custom_block/templates/custom-block-add-list.html.twigundefined
@@ -0,0 +1,23 @@
+ * - types: A collection of all the available custom block types.
+ *   - type: The custom block type, containing all the items below.
+ *   - type.link: A link to add a block of this type.
+ *   - type.description: A description of this custom block type.

We are looping through the 'types' variable, but the variable inside the loop could be called anything (not necessarily 'type') so in the documentation we shouldn't use 'type.link', etc. I would get rid of the first sub-bullet point "type" and roll that into the first point, something like "A collection of all the available custom block types. Each block type contains the follow variables:"

…and then have 'link' and 'description' as the two sub-list items. Basically we want an indent in a list to mean 'add a dot' as much as we can :)

See #1898418-14: forum.module - Convert PHPTemplate templates to Twig for an example.

Add conversion summary table

dibbs.

The only difference in manual testing was the line breaks and spaces. Adding the spaceless tag should solve this.

Manual testing:
before:

<dl class="node-type-list"><dt><a href="/block/add/ad_block">Ad block</a></dt><dd>Ad</dd><dt><a href="/block/add/basic">Basic block</a></dt><dd>A basic block contains a title and a body.</dd></dl>

after:

<dl class="node-type-list">
  <dt><a href="/block/add/ad_block">Ad block</a></dt>
  <dd>Ad</dd>
  <dt><a href="/block/add/basic">Basic block</a></dt>
  <dd>A basic block contains a title and a body.</dd>
</dl>

after addition of spaceless tag:

<dl class="node-type-list"><dt><a href="/block/add/ad_block">Ad block</a></dt><dd>Ad</dd><dt><a href="/block/add/basic">Basic block</a></dt><dd>A basic block contains a title and a body.</dd></dl>

Also made minor docs updates as Cottser suggested. New patch needs quick review.

Added the \n missing from #31

This is moving along nicely! Found some docs and possible coding standard tweaks:

1. +++ b/core/modules/block/custom_block/custom_block.pages.incundefined
   @@ -40,32 +40,25 @@ function custom_block_add_page() {
   - * Returns HTML for a list of available custom block types for block creation.
   + * Prepares variables for a list of custom block types for block creation.
   

   This should end in 'templates' per #1913208: [policy] Standardize template preprocess function documentation but still needs to be under 80 characters. Consider adding a separate 'description' line between the summary and 'Default template:' to expand on the summary if necessary.

2. +++ b/core/modules/block/custom_block/templates/custom-block-add-list.html.twigundefined
   @@ -0,0 +1,25 @@
   + * - types: A collection of all the available custom block types.
   + *   - type: The custom block type. Each block type contains the following:
   + *     - link: A link to add a block of this type.
   + *     - description: A description of this custom block type.
   

   Per http://drupal.org/node/1354#lists the top level list item needs to end in a colon before starting the sub-list. I don't think this needs three levels of indentation, especially because 'type' is not a variable, it's just what we're using to loop through and could be anything.

   I recommend something like this (needs wrapping):

   + * - types: A collection of all the available custom block types. Each custom block type in types contains the following:
   + *   - link: A link to add a block of this type.
   + *   - description: A description of this custom block type.
   

3. +++ b/core/modules/block/custom_block/templates/custom-block-add-list.html.twigundefined
   @@ -0,0 +1,25 @@
   +{% spaceless %}
   +<dl class="node-type-list">
   +  {% for type in types %}
   +    <dt>{{ type.link }}</dt>
   +    <dd>{{ type.description }}</dd>
   +  {% endfor %}
   +</dl>
   +{% endspaceless %}
   

   I think we're indenting the contents of the spaceless tag per http://drupal.org/node/1823416#whitespace.

Cleanup from #33

Awesome review, great cleanup, this looks awesome.
Good job team!

#34: 1898038-34-twig-custom-block.patch queued for re-testing.

Tagging for reroll.

rerolled.

#41: 1898038-41.patch queued for re-testing.

Rerolled version is still applying, so for me this seems to be RTBC!

Still needs profiling

rm sand

Create a few custom block types at admin/structure/custom-blocks
Set home page to block/add

=== 8.x..8.x compared (51a40ba394e16..51a40d0547739):

ct  : 38,886|38,886|0|0.0%
wt  : 331,898|332,642|744|0.2%
cpu : 328,021|328,020|-1|-0.0%
mu  : 6,719,984|6,719,984|0|0.0%
pmu : 6,828,296|6,828,296|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a40ba394e16&...

=== 8.x..custom_block-1898038-41 compared (51a40ba394e16..51a40d7881868):

ct  : 38,886|39,076|190|0.5%
wt  : 331,898|333,058|1,160|0.3%
cpu : 328,021|328,020|-1|-0.0%
mu  : 6,719,984|6,745,748|25,764|0.4%
pmu : 6,828,296|6,854,636|26,340|0.4%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a40ba394e16&...

same as #45 with twig_debug turned off this time... :/

=== 8.x..8.x compared (51a43d65a397f..51a43de4b57ff):

ct  : 38,634|38,634|0|0.0%
wt  : 328,457|328,948|491|0.1%
cpu : 324,020|324,021|1|0.0%
mu  : 6,710,540|6,710,540|0|0.0%
pmu : 6,815,672|6,815,672|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a43d65a397f&...

=== 8.x..custom_block-1898038-41 compared (51a43d65a397f..51a43e329a163):

ct  : 38,634|38,770|136|0.4%
wt  : 328,457|330,256|1,799|0.5%
cpu : 324,020|324,020|0|0.0%
mu  : 6,710,540|6,736,036|25,496|0.4%
pmu : 6,815,672|6,841,172|25,500|0.4%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a43d65a397f&...

LOL, Ed how did it get slower with Twig debugging turned off and less function calls? I'm going to chalk that into margin of error land, maybe Fabianx can explain?

Nevertheless, thanks for profiling this one:)

Yes, that is within error margin. If we compare the pure function calls:

theme_custom_block_add_list	-2,594
template_preprocess_custom_block_add_list	2,490
twig_render_template	773

=> Around 0.6 - 0.8 ms slower should be reachable from just the changed function calls, which would be an overhead of around 0.3%.

So with error margin, this would probably be like 0.3 - 0.5% slower to call the template than to call the theme_ function.

Ok great. Thanks for your expertise @Fabianx

Committed cd4c61e and pushed to 8.x. Thanks!