Task

Convert theme_ functions to Twig templates.

Remaining

  • theme_ function -> Twig conversions (and preprocess function additions or changes) moved over from the existing conversion issue
  • Reroll
  • Manual testing (markup comparison)
  • Patch review
  • Profiling
Theme function name Conversion status
theme_node_add_list converted
theme_node_admin_overview function deleted in #111715: Convert node/content types into configuration
theme_node_preview converted
theme_node_recent_block converted to a view in #2020393: Convert "Recent content" block to a View
theme_node_recent_content converted to a view in #2020393: Convert "Recent content" block to a View
theme_node_search_admin Will be converted to #type table in #1938920: Convert node_search_admin theme tables to table #type

To test this code

  1. Navigate to node/add to see if node-add-list.html.twig is working
  2. Preview the node to see if node-preview.html.twig is working

#1898432: node.module - Convert PHPTemplate templates to Twig
#1757550: [Meta] Convert core theme functions to Twig templates
#1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch

Files: 
CommentFileSizeAuthor
#68 1987406-68-twig-node-theme.patch6.34 KBidflood
PASSED: [[SimpleTest]]: [MySQL] 63,689 pass(es). View
#56 node-preview.php_.txt4.07 KBPetr Illek
#56 node-preview.twig_.txt4.1 KBPetr Illek
#56 recentcontent.php_.txt1.11 KBPetr Illek
#56 recentcontent.twig_.txt1.12 KBPetr Illek
#53 1987406-53-twig-node-theme.patch10.57 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 57,917 pass(es). View
#53 interdiff.txt730 bytesjoelpittet
#50 1987406-50-twig-node-theme.patch10.63 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 58,520 pass(es). View
#50 interdiff.txt1.6 KBjoelpittet
#46 twig-convert_node-1987406-46.patch11.2 KBherom
PASSED: [[SimpleTest]]: [MySQL] 58,366 pass(es). View
#42 twig-convert_node-1987406-42.patch11.22 KBjenlampton
PASSED: [[SimpleTest]]: [MySQL] 57,555 pass(es). View
#39 1987406-39.patch11.44 KBjerdavis
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987406-39.patch. Unable to apply patch. See the log in the details link for more information. View
#38 preview.tar_.gz490 bytesjerdavis
#32 1987406-32-twig-node-theme.patch11.57 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 57,007 pass(es). View
#32 interdiff.txt8.56 KBjoelpittet
#30 twig-node_convert-1987406-30.patch16.78 KBsanguis
FAILED: [[SimpleTest]]: [MySQL] 55,987 pass(es), 98 fail(s), and 2,109 exception(s). View
#26 twig-node_convert-1987406-26.patch14.44 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch twig-node_convert-1987406-26.patch. Unable to apply patch. See the log in the details link for more information. View
#24 twig-node_convert-1987406-24.patch14.49 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] 57,962 pass(es), 0 fail(s), and 7 exception(s). View
#22 twig-node-theme-only-1987406-10.patch15.93 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch twig-node-theme-only-1987406-10_0.patch. Unable to apply patch. See the log in the details link for more information. View
#19 node-1987406-19.patch15.93 KBezeedub
PASSED: [[SimpleTest]]: [MySQL] 57,249 pass(es). View
#12 twig-node-theme-only-1987406-10.patch15.93 KBJon Pugh
None View
#8 1987406-preview - d8.localhost.png97.04 KBdogmatix
#3 twig-node-theme-only-1987406-3.patch13.39 KBshanethehat
PASSED: [[SimpleTest]]: [MySQL] 55,885 pass(es). View

Comments

Cottser’s picture

Issue summary: View changes

Updated issue summary.

c4rl’s picture

Title: node.module - Convert PHPTemplate templates to Twig » 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.

c4rl’s picture

Issue summary: View changes

Updated issue summary.

c4rl’s picture

Issue summary: View changes

Added list of functions

shanethehat’s picture

Assigned: Unassigned » shanethehat
shanethehat’s picture

Assigned: shanethehat » Unassigned
Status: Active » Needs review
FileSize
13.39 KB
PASSED: [[SimpleTest]]: [MySQL] 55,885 pass(es). View

Status: Needs review » Needs work
Issue tags: -Needs profiling, -Twig

The last submitted patch, twig-node-theme-only-1987406-3.patch, failed testing.

shanethehat’s picture

Status: Needs work » Needs review
jstoller’s picture

Issue tags: +Needs profiling, +Twig
ericaack’s picture

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

dogmatix’s picture

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.

ernie-g’s picture

profiling...

ernie-g’s picture

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]
rwohleb’s picture

Issue tags: -Needs profiling
Jon Pugh’s picture

Issue tags: +Needs profiling
FileSize
15.93 KB
None View

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
Jon Pugh’s picture

Issue tags: -Needs profiling

Oops cross posted. but on second thought...

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

Jon Pugh’s picture

Issue tags: +Needs profiling

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

Jon Pugh’s picture

Issue tags: -Needs profiling

Manually went through the test process...

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

ernie-g’s picture

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

Jon Pugh’s picture

Status: Needs review » Needs work

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

ernie-g’s picture

Status: Needs work » Needs review

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

ezeedub’s picture

FileSize
15.93 KB
PASSED: [[SimpleTest]]: [MySQL] 57,249 pass(es). View

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

Jon Pugh’s picture

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

joelpittet’s picture

Status: Needs review » Needs work

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

jenlampton’s picture

Status: Needs work » Needs review
FileSize
15.93 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch twig-node-theme-only-1987406-10_0.patch. Unable to apply patch. See the log in the details link for more information. View

@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

Status: Needs review » Needs work

The last submitted patch, twig-node-theme-only-1987406-10.patch, failed testing.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
14.49 KB
FAILED: [[SimpleTest]]: [MySQL] 57,962 pass(es), 0 fail(s), and 7 exception(s). View

whooboy did that need a reroll. Trying again.

Status: Needs review » Needs work

The last submitted patch, twig-node_convert-1987406-24.patch, failed testing.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
14.44 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch twig-node_convert-1987406-26.patch. Unable to apply patch. See the log in the details link for more information. View

duh

Cottser’s picture

@ezeedub and @jenlampton - I think you are talking about the patch in #12, which is yellow with no Re-test link. It was one of the unlucky patches that got caught in limbo around DrupalCon Portland.

Thanks for the fresh patch @jenlampton, and it's green!

sanguis’s picture

Issue tags: -Twig

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

Status: Needs review » Needs work
Issue tags: +Twig

The last submitted patch, twig-node_convert-1987406-26.patch, failed testing.

sanguis’s picture

Status: Needs work » Needs review
FileSize
16.78 KB
FAILED: [[SimpleTest]]: [MySQL] 55,987 pass(es), 98 fail(s), and 2,109 exception(s). View

rerolled patch from #26

Status: Needs review » Needs work

The last submitted patch, twig-node_convert-1987406-30.patch, failed testing.

joelpittet’s picture

FileSize
8.56 KB
11.57 KB
PASSED: [[SimpleTest]]: [MySQL] 57,007 pass(es). View

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

joelpittet’s picture

Status: Needs work » Needs review

and go

joelpittet’s picture

Issue tags: +Needs profiling

Adding profiling back.

jerdavis’s picture

Assigned: Unassigned » jerdavis
jerdavis’s picture

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

jerdavis’s picture

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

jerdavis’s picture

FileSize
490 bytes

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

jerdavis’s picture

Issue tags: -Needs profiling
FileSize
11.44 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987406-39.patch. Unable to apply patch. See the log in the details link for more information. View

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.

Status: Needs review » Needs work

The last submitted patch, 1987406-39.patch, failed testing.

Cottser’s picture

Thanks for all your work on this @jerdavis! I tried to apply the patch locally (git apply -v --check) and got this:

Checking patch core/modules/node/node.module...
Checking patch core/modules/node/node.pages.inc...
Checking patch core/modules/node/templates/node-add-list.html.twig...
Checking patch core/modules/node/templates/node-preview.html.twig...
Checking patch core/modules/node/templates/node-recent-block.html.twig...
Checking patch core/modules/node/templates/node-recent-content.html.twig...
Checking patch core/modules/system/templates/image.html.twig...
Checking patch core/modules/user/.user.install.swp...
error: cannot apply binary patch to 'core/modules/user/.user.install.swp' without full index line
error: core/modules/user/.user.install.swp: patch does not apply

So everything applies but there is a hidden .swp file that got included in the patch unintentionally that is throwing things off.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
11.22 KB
PASSED: [[SimpleTest]]: [MySQL] 57,555 pass(es). View

rerolled as per #41

Status: Needs review » Needs work
Issue tags: -Twig

The last submitted patch, twig-convert_node-1987406-42.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
Issue tags: +Twig

#42: twig-convert_node-1987406-42.patch queued for re-testing.

Cottser’s picture

Assigned: jerdavis » Unassigned
Status: Needs review » Needs work
Issue tags: +Needs reroll

Time for another reroll.

herom’s picture

Issue tags: -Needs reroll
FileSize
11.2 KB
PASSED: [[SimpleTest]]: [MySQL] 58,366 pass(es). View

Rerolled.

herom’s picture

Status: Needs work » Needs review

changing status.

Status: Needs review » Needs work
Issue tags: -Twig

The last submitted patch, twig-convert_node-1987406-46.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
Issue tags: +Twig

#46: twig-convert_node-1987406-46.patch queued for re-testing.

Cottser’s picture

Issue summary: View changes

Correct testing steps

joelpittet’s picture

FileSize
1.6 KB
10.63 KB
PASSED: [[SimpleTest]]: [MySQL] 58,520 pass(es). View

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.

joelpittet’s picture

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

Berdir’s picture

Status: Needs review » Needs work
+++ 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()

joelpittet’s picture

Status: Needs work » Needs review
FileSize
730 bytes
10.57 KB
PASSED: [[SimpleTest]]: [MySQL] 57,917 pass(es). View

Thanks for spotting that @Berdir!

Cottser’s picture

Issue tags: +Needs manual testing

I'm not really seeing any manual testing completed here, so tagging for the Twig sprint in Prague.

Petr Illek’s picture

Just working on it. @drupalcon

Petr Illek’s picture

Issue summary: View changes

Updated issue summary.

Petr Illek’s picture

Issue summary: View changes

The work is being done in another issue.

Petr Illek’s picture

Issue summary: View changes

The work is being done in other issue.

Petr Illek’s picture

Status: Needs review » Needs work
FileSize
1.12 KB
1.11 KB
4.1 KB
4.07 KB
499 bytes
486 bytes

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.

Theme function name Notes to do
theme_node_add_list twig working, same markup, one class more in twig (admin-list)
theme_node_admin_overview function removed from linked issue
theme_node_preview twig working, same markup, one class more in twig (visually-hidden)
theme_node_recent_block twig working, same markup
theme_node_recent_content twig working, same markup
theme_node_search_admin function exist, but we don't have the template, reopened the linked issue
Cottser’s picture

Thanks a lot @Petr Illek! So we do need some more work here to get the markup matching up.

Cottser’s picture

Issue summary: View changes

Updated the conversion status table

joelpittet’s picture

Status: Needs work » Needs review
longwave’s picture

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

Cottser’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

forbesgraham’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
10.59 KB
PASSED: [[SimpleTest]]: [MySQL] 59,342 pass(es). View

Rerolling patch from comment 53.

joelpittet’s picture

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

joelpittet’s picture

Status: Needs review » Needs work
xjm’s picture

Component: node.module » node system

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

Cottser’s picture

Thanks to @dawehner for the heads-up, #2020393: Convert "Recent content" block to a View will be removing theme_node_recent_block() and theme_node_recent_content() so let's back out those conversions please since they are going away :)

joelpittet’s picture

Status: Needs work » Needs review
FileSize
6.35 KB
PASSED: [[SimpleTest]]: [MySQL] 59,637 pass(es). View

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

Cottser’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +SprintWeekend2014, +Needs reroll, +LonDUG_Jan2014

Updating the issue summary, tagging for reroll, and calling dibs for the Drupal sprint happening as part of Drupal Global Sprint Weekend in London Ontario.

idflood’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.34 KB
PASSED: [[SimpleTest]]: [MySQL] 63,689 pass(es). View

Simple reroll of patch in #66

Cottser’s picture

Assigned: Unassigned » Cottser
Issue tags: +Needs profiling

Thanks @idflood. I started work on manual testing (markup comparison) and profiling so I'm assigning until I finish that up. We do have some profiling results starting in #36 but that was almost 6 months ago and the patch (and core) have changed enough that it's worth running through again. Not bad now since there are only two templates to profile!

Cottser’s picture

Cottser’s picture

Assigned: Cottser » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing, -Needs profiling

Markup matches up perfectly (other than whitespace of course), and performance looks quite comparable. I think this one's ready to go!

/node/add (2 content types)

=== 8.x..8.x compared (52f5c0b2e882a..52f5c0f4ebbb2):

ct  : 28,420|28,420|0|0.0%
wt  : 181,812|181,273|-539|-0.3%
cpu : 173,750|173,217|-533|-0.3%
mu  : 23,851,528|23,851,528|0|0.0%
pmu : 23,908,040|23,908,040|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52f5c0b2e882a&...

=== 8.x..node-twig-1987406-68 compared (52f5c0b2e882a..52f5c11235c73):

ct  : 28,420|28,523|103|0.4%
wt  : 181,812|181,396|-416|-0.2%
cpu : 173,750|173,987|237|0.1%
mu  : 23,851,528|23,895,792|44,264|0.2%
pmu : 23,908,040|23,952,336|44,296|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52f5c0b2e882a&...

/node/add (10 content types)

=== 8.x..8.x compared (52f5c520502f0..52f5c5484cf48):

ct  : 30,348|30,348|0|0.0%
wt  : 186,288|185,630|-658|-0.4%
cpu : 178,795|178,235|-560|-0.3%
mu  : 23,912,968|23,912,968|0|0.0%
pmu : 23,966,008|23,966,008|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52f5c520502f0&...

=== 8.x..node-twig-1987406-68 compared (52f5c520502f0..52f5c574bebaf):

ct  : 30,348|30,571|223|0.7%
wt  : 186,288|186,311|23|0.0%
cpu : 178,795|178,887|92|0.1%
mu  : 23,912,968|23,957,304|44,336|0.2%
pmu : 23,966,008|24,011,640|45,632|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52f5c520502f0&...

Node preview

(tested using /twig/node-preview from https://drupal.org/sandbox/cottser/2191591)

=== 8.x..8.x compared (52f5cb0964fa0..52f5cb9d4924f):

ct  : 72,199|72,199|0|0.0%
wt  : 318,907|318,376|-531|-0.2%
cpu : 307,551|307,124|-427|-0.1%
mu  : 30,383,440|30,383,440|0|0.0%
pmu : 30,464,968|30,464,968|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52f5cb0964fa0&...

=== 8.x..node-twig-1987406-68 compared (52f5cb0964fa0..52f5cbc19a0d8):

ct  : 72,199|72,270|71|0.1%
wt  : 318,907|319,000|93|0.0%
cpu : 307,551|307,746|195|0.1%
mu  : 30,383,440|30,413,152|29,712|0.1%
pmu : 30,464,968|30,494,712|29,744|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52f5cb0964fa0&...

Cottser’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 68: 1987406-68-twig-node-theme.patch, failed testing.

The last submitted patch, 68: 1987406-68-twig-node-theme.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
Cottser’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)

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