Task

Convert PHPTemplate templates to Twig templates.

Remaining

Template path Conversion status
core/themes/bartik/templates/comment-wrapper.tpl.php We're deleting this, only the @file differs
core/themes/bartik/templates/comment.tpl.php converted
core/themes/bartik/templates/maintenance-page.tpl.php converted
core/themes/bartik/templates/node.tpl.php converted
core/themes/bartik/templates/page.tpl.php Will be converted in #1961870: Convert page.tpl.php to Twig due to theme() calls in page.tpl.php

To test this code:

@todo

#1938864: [meta] Update all core themes to use Twig
#1757550: [META-63] Convert core theme functions to Twig templates
#1939286: The theme that Drupal core runs tests in on d.o. (Bartik) should not be allowed to override core templates
#1854344: [meta] Goals for core themes: Make Stark as semantic as possible; Make Bartik a better prospective base theme

Files: 
CommentFileSizeAuthor
#73 1938840-73.patch29.94 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 55,891 pass(es).
[ View ]
#73 interdiff.txt11.2 KBCottser
#71 1938840-bartik-71.patch28.58 KBtlattimore
PASSED: [[SimpleTest]]: [MySQL] 56,113 pass(es).
[ View ]
#71 interdiff.txt1.73 KBtlattimore
#55 interdiff.txt1.25 KBshanethehat
#55 twig-bartik-1938840-55.patch28.79 KBshanethehat
PASSED: [[SimpleTest]]: [MySQL] 55,593 pass(es).
[ View ]
#53 twig-bartik-1938840-53.patch28.74 KBshanethehat
PASSED: [[SimpleTest]]: [MySQL] 55,770 pass(es).
[ View ]
#53 interdiff.txt696 bytesshanethehat
#48 twig_bartik-1938840-48.patch28.78 KBidflood
PASSED: [[SimpleTest]]: [MySQL] 55,500 pass(es).
[ View ]
#48 interdiff.txt686 bytesidflood
#44 interdiff.txt540 byteschrisjlee
#44 1938840-bartik-convert-phptemplate-twig-44.patch28 KBchrisjlee
FAILED: [[SimpleTest]]: [MySQL] 55,602 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
#43 interdiff.txt982 byteschrisjlee
#43 1938840-bartik-convert-phptemplate-twig-43.patch28.01 KBchrisjlee
FAILED: [[SimpleTest]]: [MySQL] 55,829 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
#36 1938840-36-bartik-twig.patch28.79 KBduellj
PASSED: [[SimpleTest]]: [MySQL] 54,566 pass(es).
[ View ]
#31 1938840-31-bartik-twig.patch28.82 KBduellj
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1938840-31-bartik-twig.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#31 interdiff.txt16.97 KBduellj
#25 1938840-25-bartik-twig.patch11.85 KBduellj
PASSED: [[SimpleTest]]: [MySQL] 54,326 pass(es).
[ View ]
#25 interdiff.txt2.59 KBduellj
#13 1938840-13.patch11.24 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 54,147 pass(es).
[ View ]
#13 interdiff.txt2.05 KBCottser
#12 1938840-12.patch13.29 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 54,168 pass(es).
[ View ]
#12 interdiff.txt5.7 KBCottser
#7 1938840-convert-bartik-7.patch30.1 KBchrisjlee
FAILED: [[SimpleTest]]: [MySQL] 53,865 pass(es), 14 fail(s), and 0 exception(s).
[ View ]
#7 interdiff.txt395 byteschrisjlee
#2 interdiff-tpl.txt8.49 KBchrisjlee
#1 1938840-convert-bartik-to-twig-1.patch14.77 KBchrisjlee
FAILED: [[SimpleTest]]: [MySQL] 53,573 pass(es), 14 fail(s), and 0 exception(s).
[ View ]

Comments

Issue summary:View changes

todo the test section

Issue summary:View changes

update blocked issues

Issue summary:View changes

space

Status:Active» Needs work
StatusFileSize
new14.77 KB
FAILED: [[SimpleTest]]: [MySQL] 53,573 pass(es), 14 fail(s), and 0 exception(s).
[ View ]

A start

StatusFileSize
new8.49 KB

This interdiff between templates might help

Status:Needs work» Needs review

Thanks @chrisjlee! Let's run automated testing on this patch to see how that goes.

Status:Needs review» Needs work

The last submitted patch, 1938840-convert-bartik-to-twig-1.patch, failed testing.

Issue summary:View changes

info

@Cottser Probably i can't get the twig template to render.

Status:Needs work» Needs review
StatusFileSize
new395 bytes
new30.1 KB
FAILED: [[SimpleTest]]: [MySQL] 53,865 pass(es), 14 fail(s), and 0 exception(s).
[ View ]

added engine variable to yml file. maybe it'll make a difference?

Status:Needs review» Needs work

The last submitted patch, 1938840-convert-bartik-7.patch, failed testing.

@chrisjlee, thanks for working on this. Slight change of plan, please see #1938864-2: [meta] Update all core themes to use Twig. If you can test out the "override a theme function with a template" theory that would be a huge help :)

Issue summary:View changes

Add core themes conversion meta

Issue summary:View changes

Add conversion summary table, remove blocking issues

Issue summary:View changes

Remove boilerplate from remaining tasks

Assigned:Unassigned» Cottser

Well I've solved why the page template is not getting picked up, it's named page.tpl.twig.html, should be page.html.twig.

Going to test splitting up the templates between #1898054: comment.module - Convert PHPTemplate templates to Twig and this issue and see if that will work. Removing all Bartik templates from comment.module issue, and only adding Bartik's overrides here.

Status:Needs work» Needs review
StatusFileSize
new5.7 KB
new13.29 KB
PASSED: [[SimpleTest]]: [MySQL] 54,168 pass(es).
[ View ]

Changes:

  1. Removes the page.tpl.twig.html file.
  2. Temporarily reverts the .info.yml change because #1938948: Temporarily allow PHPTemplate themes to use .html.twig templates during Twig conversion is not a two-way street, Twig themes won't pick up PHPTemplate files. So let's wait until all of Bartik's .tpl.php files are converted here and tests are passing before adding that line.
  3. Slightly modifies comment-wrapper.html.twig because #1898054: comment.module - Convert PHPTemplate templates to Twig made preprocess changes that I'm thinking will wait until after conversion and be part of the cleanup/consolidation phase :)

Issue summary:View changes

added related issue

Issue summary:View changes

Update conversion table

StatusFileSize
new2.05 KB
new11.24 KB
PASSED: [[SimpleTest]]: [MySQL] 54,147 pass(es).
[ View ]

Actually, since I'm proposing we just remove comment-wrapper.tpl.php because it only changes the @file docblock, let's try this.

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

The last submitted patch, 1938840-13.patch, failed testing.

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

#13: 1938840-13.patch queued for re-testing.

Status:Needs review» Needs work

Looked like a random test failure. Back to CNW regardless, because conversion is not complete.

@Cottser, looks like progress is good but since you closed the issue I opened in #10 with a link to #1595028: Convert remaining tests using Standard profile to use Testing profile instead that means we possibly need tests here that explicitly enable Bartik and look for the Bartik-specific markup overrides.

What do you think?

@thedavidmeister - It's definitely an interesting thought, but I don't think we need tests added in this issue.

As far as I know, most tests in core (using the default testing profile) just use Stark and the default templates and theme output. Drupal\color\Tests\ColorTest has a bit of code that would allow for testing the same behaviour with multiple themes. I think if we wanted to do this on a larger scale, it would need a new issue and probably a DRY-er approach than Drupal\color\Tests\ColorTest uses. And it would probably make testbot runs take quite a bit longer :)

#18 - If we can't verify their markup isn't bung, can we really offer extra themes with templates that override core out-of-the-box in core?

@thedavidmeister - If history is any indication, yes. I'd ask that we please take this discussion to a new issue :) thanks!

#20 - sure thing.

Assigned:Cottser» Unassigned

Unassigning. A good starting point for moving this forward would be updating the comment.html.twig added here to match the markup in Bartik's comment.tpl.php. See the screenshots in #1898054-37: comment.module - Convert PHPTemplate templates to Twig.

Issue tags:+Needs manual testing

Tagging.

Assigned:Unassigned» duellj

Giving this a go

Status:Needs work» Needs review
StatusFileSize
new2.59 KB
new11.85 KB
PASSED: [[SimpleTest]]: [MySQL] 54,326 pass(es).
[ View ]

First pass, restores bartik comment styles. Still working on what can be done here and what has to be done in the base template conversions.

The patch looks good to me, but I would want a second set of eyes to look at it.

"Prepares variables for block admin display form templates."

Could probably be rephrased as "Prepares variables for display in block admin form templates."

But other than that I don't see anything.

@micheas - thanks for taking a look. The standards for template preprocess functions can currently be found here:

#1913208: [policy] Standardize template preprocess function documentation

This is only the start of this conversion, there is still much to do.

Status:Needs review» Needs work

Issue summary:View changes

Update page.tpl.php in table

We'll convert Bartik's page.tpl.php in #1961870: Convert page.tpl.php to Twig because of the theme() calls in the template. Replacing only the base page.tpl.php breaks things badly.

When we convert the node template here, we should take #1968322: Remove unused $id and $zebra variables from templates into account.

Status:Needs work» Needs review
StatusFileSize
new16.97 KB
new28.82 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1938840-31-bartik-twig.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Finished the conversions of node.tpl.php and maintenance-page.tpl.php. Note that I had to move some code out of node.tpl.php into bartik_preprocess_node() that did some links manipulation in order to properly show the links wrapper. If there's a better way to do this (i.e. directly in the twig template) let me know.

Also, I think bartik_menu_tree() and bartik_field__taxonomy_term_reference() need to be converted in their respective issues. All bartik_menu_tree() is doing is adding a class to the wrapper, so if that gets converted to a Attribute then we just need to do a preprocess. I'll update the corresponding tickets.

Issue summary:View changes

Update conversion summary table

Issue summary:View changes

Moved theme conversions to respective issues

Status:Needs review» Needs work

This patch won't apply anymore, needs a reroll.

Issue tags:+Needs reroll

Status:Needs work» Needs review
Issue tags:-Needs manual testing, -Twig, -Needs reroll

#31: 1938840-31-bartik-twig.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs manual testing, +Twig, +Needs reroll

The last submitted patch, 1938840-31-bartik-twig.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new28.79 KB
PASSED: [[SimpleTest]]: [MySQL] 54,566 pass(es).
[ View ]

Doesn't apply indeed :). Here's a new patch that should apply

Issue tags:-Needs reroll

Thanks @duellj!

Status:Needs review» Needs work
Issue tags:-Needs manual testing, -Twig

The last submitted patch, 1938840-36-bartik-twig.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Needs manual testing, +Twig

#36: 1938840-36-bartik-twig.patch queued for re-testing.

Issue summary:View changes

Added converted status to converted theme templates

Issue summary:View changes

Added related issue for Bartik

Title:Convert Bartik to Twigbartik.theme - Convert PHPTemplate templates to Twig
Status:Needs review» Needs work

Per #1757550-44: [META-63] Convert core theme functions to Twig templates, retitling to indicate this issue applies to templates rather than theme_ functions. See #1987422: bartik.theme - Convert theme_ functions to Twig for theme_ function conversion.

Issue summary:View changes

Update remaining

Issue summary:View changes

Revise summary

Status:Needs work» Needs review

Back to CNR since this only contains template conversions. Just closed #1987422: bartik.theme - Convert theme_ functions to Twig because the bartik theme_ function conversions are handled elsewhere.

StatusFileSize
new28.01 KB
FAILED: [[SimpleTest]]: [MySQL] 55,829 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
new982 bytes

Attempting changes for #1757550: [META-63] Convert core theme functions to Twig templates: Removing the preprocess hook in the theme; Preprocess function seperated and is in the interdiff.txt.

StatusFileSize
new28 KB
FAILED: [[SimpleTest]]: [MySQL] 55,602 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
new540 bytes

Whoops an extra empty newline somehow snuck in this patch from #36. Removed this extra line too.

Status:Needs review» Needs work

The last submitted patch, 1938840-bartik-convert-phptemplate-twig-44.patch, failed testing.

Assigned:duellj» steveoliver
Status:Needs work» Needs review

@chrisjlee - Preprocess functions are fine here and in the other .tpl.php conversion issues. We just don't want these issues removing theme_ functions.

Patch to review is #36, letting @steveoliver have a go at this one.

Assigned:steveoliver» Unassigned
Status:Needs review» Needs work
Issue tags:+Novice

+++ b/core/themes/bartik/templates/node.html.twigundefined
@@ -0,0 +1,112 @@
+ * language negotiation rule that was previously applied.
+ ¶
+ *
+ * @see template_preprocess()
+ * @see template_preprocess_node()
+ * @see template_process()

Tiny bit of trailing whitespace in node.html.twig here towards the end of the docblock can be removed per http://drupal.org/coding-standards#indenting.

Status:Needs work» Needs review
StatusFileSize
new686 bytes
new28.78 KB
PASSED: [[SimpleTest]]: [MySQL] 55,500 pass(es).
[ View ]

Here is a quick reroll based on patch in #36 with the empty line removed.

Issue tags:+Needs profiling

I'm planning on profiling these templates by copying them to the Stark theme.

Assigned:Unassigned» shanethehat

I'll do the manual testing

Status:Needs review» Needs work

Manual testing results:

The node/comment comparison seems to be missing some comments:

@@ -52,7 +52,6 @@
</p>
</div>
</div>
-<!-- /.attribution -->
<div class="comment-text">
<div class="comment-arrow">
</div>
@@ -60,10 +59,8 @@
<a rel="bookmark" class="permalink" href="/comment/1#comment-1">Comment1 Subject</a>
</h3>
</div>
-<!-- /.comment-text -->
</header>
-<!-- /.comment-header -->
-<div class="content">
+<div class="content ">
<span class="rdf-meta" resource="/node/1" rel="sioc:reply_of">
</span>
<div data-edit-id="comment/1/comment_body/und/full" class="field field-name-comment-body field-type-text-long field-label-hidden edit-processed">
@@ -74,7 +71,6 @@
</div>
</div>
</div>
-<!-- /.content -->
<footer class="comment-footer">
<nav>
<ul class="links inline">
@@ -90,7 +86,6 @@
</ul>
</nav>
</footer>
-<!-- /.comment-footer -->
</article>
<h2 class="title comment-form">Add new comment</h2>
<form accept-charset="UTF-8" id="comment-form" method="post" action="/comment/reply/1" class="comment-node-article-comment-form comment-form">

Maintenance page markup looks almost identical. The only difference is that in the new version the closing section tag is on a new line.

Before:

<!DOCTYPE html>
<html lang="en" dir="ltr">
<head>
<meta charset="utf-8" />
<link rel="shortcut icon" href="http://local.drupal8/core/misc/favicon.ico" type="image/vnd.microsoft.icon" />
<meta name="Generator" content="Drupal 8 (http://drupal.org)" />
<title>Site under maintenance | local.drupal8</title>
<style media="all">
@import url("http://local.drupal8/core/themes/bartik/css/maintenance-page.css?mmvqr2");
</style>
<style media="all">
@import url("http://local.drupal8/core/themes/bartik/css/layout.css?mmvqr2");
@import url("http://local.drupal8/core/themes/bartik/css/style.css?mmvqr2");
@import url("http://local.drupal8/core/themes/bartik/css/colors.css?mmvqr2");
</style>
<style media="print">
@import url("http://local.drupal8/core/themes/bartik/css/print.css?mmvqr2");
</style>
</head>
<body class="maintenance-page in-maintenance no-sidebars" >
<div id="skip-link">
<a href="#main-content" class="element-invisible element-focusable">Skip to main content</a>
</div>
<div id="page-wrapper">
<div id="page">
<header id="header" role="banner">
<div class="section clearfix">
<div id="name-and-slogan">
<div id="site-name">
<strong>
<a href="/" title="Home" rel="home">
<span>local.drupal8</span>
</a>
</strong>
</div>
</div>
<!-- /#name-and-slogan -->
</div>
</header>
<!-- /.section, /#header -->
<div id="main-wrapper">
<div id="main" class="clearfix">
<main id="content" class="column" role="main">
<section class="section">
<a id="main-content">
</a>
<h1 class="title" id="page-title">Site under maintenance</h1>        local.drupal8 is currently under maintenance. We should be back shortly. Thank you for your patience.              </section>
</main>
<!-- /.section, /#content -->
</div>
</div>
<!-- /#main, /#main-wrapper -->
</div>
</div>
<!-- /#page, /#page-wrapper -->
</body>
</html>

After:

<!DOCTYPE html>
<html lang="en" dir="ltr">
<head>
<meta charset="utf-8" />
<link rel="shortcut icon" href="http://local.drupal8/core/misc/favicon.ico" type="image/vnd.microsoft.icon" />
<meta name="Generator" content="Drupal 8 (http://drupal.org)" />
<title>Site under maintenance | local.drupal8</title>
<style media="all">
@import url("http://local.drupal8/core/themes/bartik/css/maintenance-page.css?mmvs7j");
</style>
<style media="all">
@import url("http://local.drupal8/core/themes/bartik/css/layout.css?mmvs7j");
@import url("http://local.drupal8/core/themes/bartik/css/style.css?mmvs7j");
@import url("http://local.drupal8/core/themes/bartik/css/colors.css?mmvs7j");
</style>
<style media="print">
@import url("http://local.drupal8/core/themes/bartik/css/print.css?mmvs7j");
</style>
</head>
<body class="maintenance-page in-maintenance no-sidebars">
<div id="skip-link">
<a href="#main-content" class="element-invisible element-focusable">Skip to main content</a>
</div>
<div id="page-wrapper">
<div id="page">
<header id="header" role="banner">
<div class="section clearfix">
<div id="name-and-slogan">
<div id="site-name">
<strong>
<a href="/" title="Home" rel="home">
<span>local.drupal8</span>
</a>
</strong>
</div>
</div>
<!-- /#name-and-slogan -->
</div>
</header>
<!-- /.section, /#header -->
<div id="main-wrapper">
<div id="main" class="clearfix">
<main id="content" class="column" role="main">
<section class="section">
<a id="main-content">
</a>
<h1 class="title" id="page-title">Site under maintenance</h1>        local.drupal8 is currently under maintenance. We should be back shortly. Thank you for your patience.
              </section>
</main>
<!-- /.section, /#content -->
</div>
</div>
<!-- /#main, /#main-wrapper -->
</div>
</div>
<!-- /#page, /#page-wrapper -->
</body>
</html>

Issue tags:-Twig

Ah, nevermind the comments issue. In the twig template they are twig comments, which seems fair enough. Just want to fix

-<div class="content">
+<div class="content ">

Status:Needs work» Needs review
Issue tags:+Twig
StatusFileSize
new696 bytes
new28.74 KB
PASSED: [[SimpleTest]]: [MySQL] 55,770 pass(es).
[ View ]

This removes the specific adding of a content class that was causing that extra space. The class is added to $content_attributes in #1898054: comment.module - Convert PHPTemplate templates to Twig.

Hmm, that's the first I've seen Twig comments being used like that. For now can we please make those HTML comments as before? I think changing them would be another issue. Thanks @shanethehat!

StatusFileSize
new28.79 KB
PASSED: [[SimpleTest]]: [MySQL] 55,593 pass(es).
[ View ]
new1.25 KB

Comments reverted

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

The last submitted patch, twig-bartik-1938840-55.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Novice, +Needs manual testing, +Needs profiling, +Twig

#55: twig-bartik-1938840-55.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

I have created a piece of content with comments on both my local sites (patch-site and clean site) and I have verified that the html-output is the same, except for the afforementioned "content"-class on the comments.

Also verified the maintenance-page output.

Status:Reviewed & tested by the community» Needs review
Issue tags:-Novice, -Needs manual testing

Has been manually tested. Still needs profiling I guess.

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs profiling, -Twig+undefined

Looks great to me, recommend RTBC

adding back the tags

Assigned:shanethehat» Unassigned

Unassigning so this can be profiled.

I mentioned in #49 that I was thinking of profiling these by copying the templates to Stark but that won't really work because then we lose the preprocess functions. So instead I recommend copying node.html.twig to core/themes/bartik/templates (on both branches) for profiling. And make sure you have a node displayed always.

See #1938848-57: seven.theme - Convert PHPTemplate templates to Twig for some extra steps needed for profiling the maintenance-page template.

Assigned:Unassigned» jerdavis

Profiling

Issue tags:-Needs profiling

Profiling this my results varied from wt: 1.7% to 2.5%. I selected the 2.3% run for upload.

This profiles a node page with 100 comments.

Scenario:
* Applied patch in this thread
* Applied patch for page.tpl.php from: #1961870
* Copied node.html.twig into Bartik theme for both 8.x and my twig branch
* Generate a node with 100 comments
* Point find-min-web.sh at a node display with comments shown
* Place node block in sidebar

=== 8.x..8.x compared (519d444ea4569..519d44a498311):
ct  : 151,990|151,990|0|0.0%
wt  : 553,943|552,960|-983|-0.2%
cpu : 540,159|538,470|-1,689|-0.3%
mu  : 16,940,032|16,940,032|0|0.0%
pmu : 17,407,640|17,407,640|0|0.0%
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d444ea4569&run2=519d44a498311&source=drupal-perf-drupalcon&extra=8.x..8.x
Run 519d444ea4569 uploaded successfully for drupal-perf-drupalcon.
Run 519d44e865217 uploaded successfully for drupal-perf-drupalcon.
=== 8.x..twig-bartik-1938840-55 compared (519d444ea4569..519d44e865217):
ct  : 151,990|154,650|2,660|1.8%
wt  : 553,943|566,941|12,998|2.3%
cpu : 540,159|549,382|9,223|1.7%
mu  : 16,940,032|17,006,080|66,048|0.4%
pmu : 17,407,640|17,470,472|62,832|0.4%
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d444ea4569&run2=519d44e865217&source=drupal-perf-drupalcon&extra=8.x..twig-bartik-1938840-55

Maintenance page Scenario
* Place site in maintenance mode

=== 8.x..8.x compared (519d48ac950bf..519d48ce066d7):
ct  : 6,346|6,346|0|0.0%
wt  : 39,013|39,039|26|0.1%
cpu : 36,787|36,819|32|0.1%
mu  : 5,105,328|5,105,328|0|0.0%
pmu : 5,160,952|5,160,952|0|0.0%
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d48ac950bf&run2=519d48ce066d7&source=drupal-perf-drupalcon&extra=8.x..8.x
Run 519d48ac950bf uploaded successfully for drupal-perf-drupalcon.
Run 519d48e206f9d uploaded successfully for drupal-perf-drupalcon.
=== 8.x..twig-bartik-1938840-55 compared (519d48ac950bf..519d48e206f9d):
ct  : 6,346|6,615|269|4.2%
wt  : 39,013|41,001|1,988|5.1%
cpu : 36,787|38,640|1,853|5.0%
mu  : 5,105,328|5,496,856|391,528|7.7%
pmu : 5,160,952|5,549,496|388,544|7.5%
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d48ac950bf&run2=519d48e206f9d&source=drupal-perf-drupalcon&extra=8.x..twig-bartik-1938840-55

Assigned:jerdavis» Unassigned

Status:Reviewed & tested by the community» Needs work

Status:Needs work» Reviewed & tested by the community

Performance results have been reviewed by @Fabianx, looks good.

The first one is the same as #1898054: comment.module - Convert PHPTemplate templates to Twig and that has been deemed acceptable for now.

The second scenario is showing loading all of Twig which is about 2ms and is fine, we already passed that profiling gate back at BADcamp time :)

+++ b/core/themes/bartik/bartik.themeundefined
@@ -111,6 +111,23 @@ function bartik_process_maintenance_page(&$variables) {
/**
+ * Implements hook_preprocess_HOOK() for node.html.twig.
+ */
+function bartik_preprocess_node(&$variables) {
+  // Operate on a copy of links in order to preserve the original.
+  $links = $variables['content']['links'];
+
+  // Remove the "Add new comment" link on the teaser page or if the comment
+  // form is being displayed on the same page.
+  if ($variables['teaser'] || !empty($variables['content']['comments']['comment_form'])) {
+    unset($links['comment']['#links']['comment-add']);
+  }
+
+  // Render links to test if there is content present to display.
+  $variables['links'] = drupal_render($links);
+}

Do we need this function and I can't understand the comment...

Maybe the text should be
Remove the "Add new comment" link on teasers or when the comment form is display on the page.

Status:Reviewed & tested by the community» Needs work

Assigned:Unassigned» tlattimore

I will work on this re-roll.

StatusFileSize
new1.73 KB
new28.58 KB
PASSED: [[SimpleTest]]: [MySQL] 56,113 pass(es).
[ View ]

The reason we have a added a preprocess to this patch is to cleanup the node.html.twig file and move some logic that was within node.tpl.php into preprocess, rather than the template. This is good because it makes our template much cleaner - but the patch from #55 is doing some unnecessary effort in bartik_preprocess_node(). The passing of $links to drupal_render() was not necessary and we did not need to create a new variable for this all. We can just reference content.links within our node template.

Here is a patch that removes the unnecessary work within bartik_preprocess_node() and adjusts the comment as requested in #68 by @alexpott.

Assigned:tlattimore» Unassigned
Status:Needs work» Needs review

Tagging for the bot.

StatusFileSize
new11.2 KB
new29.94 KB
PASSED: [[SimpleTest]]: [MySQL] 55,891 pass(es).
[ View ]

@tlattimore - thanks, that's much better and should be faster too!

After giving this a review I found that the docs are actually a bit off here - they have been much improved in other issues.

This should remedy the docs by just using the guts of the docblocks from #1898432: node.module - Convert PHPTemplate templates to Twig and #1898054: comment.module - Convert PHPTemplate templates to Twig, and changing one instance of page.tpl.php to page.html.twig.

Also tweaked spacing for the t filter according to http://drupal.org/node/1823416#filters.

Status:Needs review» Reviewed & tested by the community

Re-reviewed this patch:

Results look good!

Interdiffs are sane => Back to RTBC

Status:Reviewed & tested by the community» Fixed

Committed 8dc1fab and pushed to 8.x. Thanks!

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

Issue summary:View changes

Formatting