node.module - Convert theme_ functions to Twig

Per #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions rather than templates. See #1898432: node.module - Convert PHPTemplate templates to Twig for template conversion.

Updated issue summary.

Added list of functions

#3: twig-node-theme-only-1987406-3.patch queued for re-testing.

#3: twig-node-theme-only-1987406-3.patch queued for re-testing.

I'll do the manual testing here at the Drupalcon code sprint.

Testing patch in #3:

I added some text in the .twig files to confirm that these files are being used.

These are the files I received:
node.html.twig - can't confirm this is working and I'm not sure where to check. My test text isn't visible on node view
node-add-list.html.twig - can't confirm this is working, my test text isn't visible when adding a list field to a content type or when creating a new page and editing a list field or on the node/add page for any content type
node-admin-overview.html.twig is working. (Shows once per content type)
node-edit-form.html.twig is working.
node-preview.html.twig seems to add in the previews twice (screenshot)
node-recent-block.html.twig is working.
node-recent-content.html.twig is working (once per node)

I took a quick look at node-preview.html.twig and node-edit-form.html.twig and I don't see why the Preview would be appearing twice.

My first patch test and at the DrupalCon sprint :)
Please let me know if I can provide more information.

profiling...

Profile bbranches output:

=== 8.x..8.x compared (519fe99884ff9..519ff9386b5e5):

ct  : 71,226|71,226|0|0.0%
wt  : 466,475|482,413|15,938|3.4%
cpu : 419,918|419,137|-781|-0.2%
mu  : 16,002,240|16,002,240|0|0.0%
pmu : 16,130,312|16,130,312|0|0.0%

<a href="http://drupal8.local/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=%2F&run1=519fe99884ff9&run2=519ff9386b5e5&extra=8.x..8.x">Profiler output</a>

=== 8.x..profiling-1987406 compared (519fe99884ff9..519ff9764f055):

ct  : 71,226|71,226|0|0.0%
wt  : 466,475|496,949|30,474|6.5%
cpu : 419,918|421,834|1,916|0.5%
mu  : 16,002,240|16,002,176|-64|-0.0%
pmu : 16,130,312|16,129,984|-328|-0.0%

<a href="http://drupal8.local/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=%2F&run1=519fe99884ff9&run2=519ff9764f055&extra=8.x..profiling-1987406">Profiler output</a>

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage
The drush command 'rr' could not be found.  Run `drush cache-clear drush` to clear the commandfile cache if you have installed new extensions.    [error]
'all' cache was cleared in self                                                                                                                   [success]

Got node-add-list.html.twig working...

• Removed seven_node_add_list()
• Added "admin-list" to node-add-list.html.twig (Should this be in core or in seven_preprocess_node_add_list()? If so we need a classes variable.
• Added css for .admin-list dt to match the .admin-list li

Oops cross posted. but on second thought...

Can we even profile this if the templates aren't being used yet?

node.html.twig seems to work too...

Manually went through the test process...

The last task here looks like node-search-admin.html.twig

MANY APOLOGIES
It turns out, the profiling instructions I received at the DrupalCon Code Sprint was incomplete.
I am going to be re-running all my profiling :-/

So the last theme_function to kill here is theme_node_search_admin()

This one is tricky because it has a table in it.... @bradwade and I think maybe a table isn't the best way to do this, but either way... I do not know enough about twig to figure out how to render a form in a table!

So... code sprints over... needs work :)

P.S. @ernie-g No worries! Gotta start somewhere, right? :D

MANY APOLOGIES
It turns out, the profiling instructions I received at the DrupalCon Code Sprint was incomplete.
I am going to be re-running all my profiling :-/

Resubmitting the patch in #10 to kick the test bot into gear (pls tell me if there's a better way to do that).

If we can de-scope this issue to remove theme_node_search_admin (which has it's own issue anyway #1938920: Convert node_search_admin theme tables to table #type) than this is ready to be RTBC'd by someone.

#1938920: Convert node_search_admin theme tables to table #type is close to done, but its also not a twig issue at all, so there's another argument to call this one (ready to be called) done...

@Jon Pugh re: #20 that type table conversion is not in the patch and is called out in the summary to not convert it, am I missing something?

Also, what's up with the CSS change of adding the dl selector, is that needed? That looks like it's part of the seven theme conversion and should probably make it's way over to that or separate issue.

@ezeedub there is a link on the very right of each patch that reads "Re-test". Click that link if you want to retest a patch, no need to upload something that's not-quite the same :)

reuploading #10

whooboy did that need a reroll. Trying again.

duh

#26: twig-node_convert-1987406-26.patch queued for re-testing.

rerolled patch from #26

@Jon Pugh
seven_node_add_list should be taken care of here:
#1987424: seven.theme - Convert theme_ functions to Twig
Along with the CSS change if needed, which I also reverted (needs a new issue or or if needed for conversion then move it there as well.)

Removed the @see template_preprocess() from the twig files.

There seemed to be a language module change from $language_content->langcode to $language_content->id so I reverted those changes.

Also reverted a change from ->nid to ->id() because there is another issue open for that and there a few more in node.module that need conversion.

and go

Adding profiling back.

First profiling run for this patch. This is of the node/add page

Scenario:
- Have 10 content types with descriptions
- Give Anonymous role all permissions
- Set home page to node/add

=== 8.x-node..8.x-node compared (51ec3b81ad265..51ec3c1813b11):
ct  : 52,129|52,129|0|0.0%
wt  : 243,653|241,193|-2,460|-1.0%
cpu : 230,023|230,778|755|0.3%
mu  : 16,295,256|16,295,256|0|0.0%
pmu : 16,433,224|16,433,224|0|0.0%

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

=== 8.x-node..1987406-twig-node-theme compared (51ec3b81ad265..51ec3c5ab9ae4):
ct  : 52,129|52,343|214|0.4%
wt  : 243,653|244,027|374|0.2%
cpu : 230,023|231,082|1,059|0.5%
mu  : 16,295,256|16,332,304|37,048|0.2%
pmu : 16,433,224|16,470,680|37,456|0.2%

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

Profiling of Recent Content block

Scenario:
- Recent content block in content region
- 50 nodes generated
- Front page display showing only 1 node

=== 8.x-node..8.x-node compared (51ec485525502..51ec489b18752):

ct  : 158,792|158,792|0|0.0%
wt  : 628,399|627,377|-1,022|-0.2%
cpu : 609,055|608,662|-393|-0.1%
mu  : 19,350,392|19,350,392|0|0.0%
pmu : 19,467,584|19,467,584|0|0.0%

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

=== 8.x-node..1987406-twig-node-theme compared (51ec485525502..51ec491641923):

ct  : 158,792|159,164|372|0.2%
wt  : 628,399|629,948|1,549|0.2%
cpu : 609,055|610,806|1,751|0.3%
mu  : 19,350,392|19,484,376|133,984|0.7%
pmu : 19,467,584|19,601,400|133,816|0.7%

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

Profiling of Preview

Scenario -
- Custom module adds a callback to preview node 1 at /preview
- Make sure there's a node 1
- See files for the module

=== 8.x-node..8.x-node compared (51ec517fc2424..51ec51f5849d9):

ct  : 223,036|223,036|0|0.0%
wt  : 854,166|857,092|2,926|0.3%
cpu : 834,322|837,469|3,147|0.4%
mu  : 18,717,216|18,717,216|0|0.0%
pmu : 18,871,808|18,871,808|0|0.0%

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

=== 8.x-node..1987406-twig-node-theme compared (51ec517fc2424..51ec52a09990a):

ct  : 223,036|223,101|65|0.0%
wt  : 854,166|858,252|4,086|0.5%
cpu : 834,322|838,585|4,263|0.5%
mu  : 18,717,216|18,739,224|22,008|0.1%
pmu : 18,871,808|18,894,800|22,992|0.1%

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

Above I've profiled the 3 functions included in this patch.

The template for theme_node_admin_overview is no longer invoked, as the Admin Overview of content types has been switched to separate table columns for type and description.

theme_node_search_admin is not included in the patch.

Given the change to admin overview, here's a re-roll of #32 removing that extra twig template.

rerolled as per #41

Rerolled.

changing status.

Did my first git bisect to find out who got rid of theme_node_admin_overview

Updated issue summary: Removed from #111715: Convert node/content types into configuration: Convert node/content types into configuration

Removed the extra image.html.twig template that snuck in there and cleaned up the preprocess comments to remove the @see template_preprocess_* because that is not done anywhere else in core and is a bit unclear of what it does.

+++ b/core/modules/node/templates/node-recent-block.html.twig
@@ -0,0 +1,20 @@
+{% if table %}
+  {{- table -}}
+{% endif %}
+{% if more_link %}
+  {{- more_link -}}
+{% endif %}

We could really simplify this template one of 3 ways.

1. Remove the whitespace modifiers
2. Add blank variables in the preprocess and remove both the conditions and whitespace modifiers.
3. Leave it alone, it's fine.

Thoughts?

Other than that, I think this is RTBC.

+++ b/core/modules/node/node.module
@@ -1410,52 +1414,41 @@ function theme_node_recent_block($variables) {
-    '#status' => node_mark($node->id(), $node->getChangedTime()),
+    '#type' => node_mark($node->id(), $node->changed),

Accidental revert of using getChangedTime()

Thanks for spotting that @Berdir!

Just working on it. @drupalcon

Updated issue summary.

The work is being done in another issue.

The work is being done in other issue.

I've check if the twig templates are used - see the updated conversion table on top.
There are still some minor differences in the markup – see attached txt files with samples.

53: 1987406-53-twig-node-theme.patch queued for re-testing.

+  // Render trimmed teaser version of the post.
+  $node_teaser = node_view($node, 'teaser');
+  $node_teaser['#attached']['library'][] = array('node', 'drupal.node.preview');
+  $variables['teaser'] = $node_teaser;
+  // Render full version of the post.
+  $node_full = node_view($node, 'full');
+  $variables['full'] = $node_full;
+
+  // Display a preview of the teaser only if the content of the teaser is
+  // different to the full post.
+  if ($variables['teaser'] != $variables['full']) {

Not sure this will work as expected - the non-Twig version compared the drupal_render() output instead of the original render array. Additionally, $variables['teaser']['#attached'] will be set by the above block, and $variables['full']['#attached'] is unlikely to include the drupal.node.preview library.

Having said that, I wonder if this has ever worked as expected, because it's likely that the rendered output will at least contain different HTML classes for the full and teaser versions, isn't it?

Rerolling patch from comment 53.

@forbesgraham thanks for the re-roll.

+++ b/core/modules/node/templates/node-add-list.html.twig
@@ -0,0 +1,27 @@
+  <dl class="node-type-list admin-list">

'admin-list' class shouldn't be here as per #56

Not sure about that extra visually-hidden class on preview... may need a re-test.

node_search_admin is to be possibly taken care of through #type=>table or possibly something else...

@longwave I think you're right, it may need to be still passed through drupal_render and we should confirm the output comparison is doing what is meant to. (crosses fingers for #1510544: Allow to preview content in an actual live environment to pull through)

(Merging "node system" and "node.module" components for 8.x; disregard.)

Re-rolled, removed those recent block/content conversions and added a trans block for the longer text.

Simple reroll of patch in #66

Committed 5e112d8 and pushed to 8.x. Thanks!