Issue #1898432 by WebDevDude, steveoliver, vlad.dancer, jenlampton, pixelmord, Fabianx, iztok, EVIIILJ, jwilson3, c4rl, Cottser: Convert node module to Twig.

Task

Convert PHPTemplate templates to Twig

Remaining

  • Patch needs review
  • Manual testing (see below).

Template path Conversion status
core/modules/node/templates/node-edit-form.tpl.php converted
core/modules/node/templates/node.tpl.php converted

To test this code

  1. Create a node to see if node-edit-form.html.twig is working
  2. Save your node to see if node.html.twig is working

Depends on

#1938948: Temporarily allow PHPTemplate themes to use .html.twig templates during Twig conversion

#1757550: [Meta] Convert core theme functions to Twig templates

Follow-ups

#54898: Add a description-list.html.twig template (ex. definition list)
#1939136: Combine node-recent-block and node-recent-content templates.
#1939238: Remove $submitted variable and print string directly in template instead
#1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead

CommentFileSizeAuthor
#122 1898432-122.patch21.45 KBstar-szr
#122 interdiff.txt1.86 KBstar-szr
#119 8.x-1898432-119.patch23.31 KBsteveoliver
#119 8.x-1898432-109-119--interdiff.txt1.3 KBsteveoliver
#109 interdiff.txt885 bytesHydra
#109 1898432-110.patch21.9 KBHydra
#102 1898432-102.patch22.77 KBHydra
#102 interdiff.txt1.05 KBHydra
#96 1898432-96.patch23.38 KBstar-szr
#96 interdiff.txt414 bytesstar-szr
#94 interdiff.txt3.16 KBshanethehat
#94 twig-node-template-only-1898432-94.patch23.38 KBshanethehat
#93 twig-node-template-only-1898432-93.patch20.98 KBshanethehat
#92 twig-node-template-only-1898432-92.patch20.98 KBshanethehat
#87 twig-node-1898432-87.patch35.18 KBshanethehat
#82 interdiff.txt3.75 KBshanethehat
#82 twig-node-1898432-82.patch35.18 KBshanethehat
#79 interdiff.txt4.07 KBshanethehat
#79 twig-node-1898432-79.patch35.67 KBshanethehat
#76 interdiff.txt3.29 KBshanethehat
#76 twig-node-1898432-76.patch36.04 KBshanethehat
#72 interdiff.txt5.65 KBshanethehat
#72 twig-node-1898432-72.patch36 KBshanethehat
#64 interdiff.txt4.02 KBchrisjlee
#64 1898432-convert-node-twig-64.patch35.04 KBchrisjlee
#63 1898432-convert-node-rerollof58-63.patch36.39 KBchrisjlee
#58 drupal-twig-node--1898432-58.patch35.53 KBsteveoliver
#58 drupal-twig-node--1898432-54-58-interdiff.txt4.87 KBsteveoliver
#54 1898432-54.patch35.53 KBstar-szr
#54 interdiff.txt2.28 KBstar-szr
#50 1898432-50.patch35.39 KBstar-szr
#50 interdiff.txt3.98 KBstar-szr
#49 1898432-49.patch31.41 KBstar-szr
#49 interdiff.txt3.74 KBstar-szr
#46 1898432-46.patch31.34 KBstar-szr
#46 interdiff.txt5.54 KBstar-szr
#40 drupal-twig-node--1898432-40.patch34.33 KBsteveoliver
#40 drupal-twig-node--1898432-37-40-interdiff.txt4.74 KBsteveoliver
#37 1898432-37.patch34.13 KBstar-szr
#36 1898432-36.patch34.13 KBstar-szr
#36 interdiff.txt2.08 KBstar-szr
#34 core-update_node_to_twig-1898432-34.patch34.72 KBjenlampton
#34 interdiff.txt5.27 KBjenlampton
#33 core-update_node_to_twig-1898432-33.patch34.02 KBjenlampton
#33 interdiff.txt4.57 KBjenlampton
#30 core-update_node_to_twig-1898432-30.patch157.48 KBjenlampton
#30 interdiff.txt11.41 KBjenlampton
#31 1898432-31.patch31.7 KBstar-szr
#31 interdiff.txt2.42 KBstar-szr
#29 1898432-29.patch26.11 KBstar-szr
#29 interdiff.txt5.88 KBstar-szr
#22 1898432-22.patch25.56 KBstar-szr
#22 interdiff.txt981 bytesstar-szr
#19 1898432-19.patch25.49 KBstar-szr
#19 interdiff.txt6.05 KBstar-szr
#9 1898432-9.patch25.63 KBstar-szr
#9 interdiff.txt1.68 KBstar-szr
#6 1898432-6.patch25.65 KBstar-szr
#6 interdiff.txt10.89 KBstar-szr
#5 drupal-1898432-5.patch27.72 KBc4rl
#5 drupal-1898432-5.patch-interdiff.txt585 bytesc4rl
#3 drupal-1898432-3.patch27.72 KBc4rl
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

c4rl’s picture

Issue tags: +Twig

Tagging

c4rl’s picture

Assigned: Unassigned » c4rl

Assigning

c4rl’s picture

Status: Active » Needs review
FileSize
27.72 KB

Status: Needs review » Needs work

The last submitted patch, drupal-1898432-3.patch, failed testing.

c4rl’s picture

Status: Needs work » Needs review
FileSize
585 bytes
27.72 KB

Fixed whitespace modifier

star-szr’s picture

FileSize
10.89 KB
25.65 KB

Thanks @c4rl!

Rerolled to address some minor coding style and documentation issues and revert some of the changes in the variables documentation for node.html.twig. In particular, the descriptions for "Node status variables" are IMO much improved in node.html.twig vs. node.tpl.php, and it looks like #5 would actually revert those improvements. Also removed an extra 'created' variable from "Other variables".

I'm curious about the use of "Setup template variables for…" vs. "Preprocess variables for…", is there a distinction? If not, I think we should stick with Preprocess.

Can we also get a list of contributors for the commit message, similar to #1898478: menu.inc - Convert theme_ functions to Twig?

c4rl’s picture

I'm curious about the use of "Setup template variables for…" vs. "Preprocess variables for…", is there a distinction? If not, I think we should stick with Preprocess.

Agreed.

Can we also get a list of contributors

Aside from myself and Cottser, from what git blame tells me, they would include: WebDevDude steveoliver vlad.dancer jenlampton pixelmord Fabianx iztok EVIIILJ jrwilson3

star-szr’s picture

Assigned: c4rl » star-szr
Status: Needs review » Needs work

Okay, CNW for #7, I'll roll a new patch tonight to update those.

star-szr’s picture

Assigned: star-szr » Unassigned
Status: Needs work » Needs review
FileSize
1.68 KB
25.63 KB

Updated the issue summary to include a commit message as well, thanks for that c4rl!

keyhitman’s picture

Assigned: Unassigned » keyhitman
c4rl’s picture

@keyhitman Tests have passed, can you provide some insight as to why you are assigning? Thanks

keyhitman’s picture

Status: Needs review » Needs work

Patch does not apply to latest head needs a re-roll. Do you mind if we have a go at the re-roll?

keyhitman’s picture

Assigned: keyhitman » Unassigned

This looks too complicated for a first-timer like me :)

star-szr’s picture

Status: Needs work » Needs review

#9: 1898432-9.patch queued for re-testing.

star-szr’s picture

Patch still applies.

Fabianx’s picture

+++ b/core/modules/node/content_types.incundefined
@@ -88,14 +89,10 @@ function node_overview_types() {
+  $variables['description'] = filter_xss_admin($variables['type']->description);
+  $variables['machine_name'] = t('(Machine name: @type)', array('@type' => $variables['type']->type));

I think we agreed that t('') should be done in templates as filter.

If that is no in our coding standards, lets add it.

+++ b/core/modules/node/node.moduleundefined
@@ -1533,10 +1541,9 @@ function theme_node_search_admin($variables) {
+  $variables['table'] = theme('table', array('header' => $header, 'rows' => $rows));
+  $variables['form'] = drupal_render_children($form);

According to newest developments, lets use a render array with

#theme => 'table' instead and not use theme() in preprocess().

Please lets add that to the docs. It is vital to render as late as possible.

The same is true for the form, though I do not know if render() will work the same as drupal_render_children or if we need a

#theme => children

Thanks!

+++ b/core/modules/node/node.moduleundefined
@@ -1973,18 +1980,18 @@ function theme_node_recent_block($variables) {
+  $variables['table'] = $rows ? theme('table', array('rows' => $rows)) : NULL;
+  if (user_access('access content overview') && $rows) {

Again use:

#theme => table

and maybe use array() instead of NULL.

+++ b/core/modules/node/node.moduleundefined
@@ -1973,18 +1980,18 @@ function theme_node_recent_block($variables) {
+    $variables['more_link'] = theme('more_link', array('url' => 'admin/content', 'title' => t('Show more content')));
+  }
+  else {

The same is true here for using a render_array.

And for all future uses of theme().

+++ b/core/modules/node/node.pages.incundefined
@@ -66,22 +67,17 @@ function node_add_page() {
+    $variables['no_content_text'] = t('You have not created any content types yet. Go to the <a href="@create-content">content type creation page</a> to add a new content type.', array('@create-content' => url('admin/structure/types/add')));

t() should be in templates.

+++ b/core/modules/node/node.pages.incundefined
@@ -168,30 +165,26 @@ function node_preview(Node $node) {
+  $variables['teaser']= drupal_render($node_teaser);
+  // Render full version of the post.
+  $node_full = node_view($node, 'full');
+  $variables['full'] = drupal_render($node_full);

No need to render() this here. This is just wasting CPU.

+++ b/core/modules/node/templates/node-preview.html.twigundefined
@@ -0,0 +1,25 @@
+{% if preview_teaser %}
+  <h3>{{ "Preview trimmed version" | t }}</h3>
+  {{ teaser }}
+  <h3>{{ "Preview full version" | t }}</h3>

Yu, here we would have rendered it needlessly.

Yay! render arrays FTW here.

+++ b/core/modules/node/templates/node.html.twigundefined
@@ -93,10 +93,10 @@
+    {{ hide(content.comments) }}
+    {{ hide(content.links) }}

Totally no:

{% is correct

BECAUSE

{{ == print and hide() does not print something.

steveoliver’s picture

Status: Needs review » Needs work

Excellent catches, Fabianx.

star-szr’s picture

Assigned: Unassigned » star-szr

I'll take a look at the changes in #16 and creating some documentation around those.

star-szr’s picture

Status: Needs work » Needs review
FileSize
6.05 KB
25.49 KB

Thanks for the notes @Fabianx, they were really helpful. This should address most of #16, the only part I didn't make changes for was this:

+++ b/core/modules/node/node.moduleundefined
@@ -1533,10 +1541,9 @@ function theme_node_search_admin($variables) {
+  $variables['form'] = drupal_render_children($form);

The same is true for the form, though I do not know if render() will work the same as drupal_render_children or if we need a

#theme => children

The form in question has an empty child array so I didn't feel confident changing that without being able to test it.

Documentation on "do's and don'ts" to come, but I wanted to get the revised patch up tonight.

I also removed the spaces before and after the t() filter, per the posted Twig coding standards at http://drupal.org/node/1823416.

star-szr’s picture

Issue summary: View changes

Adding commit message

Fabianx’s picture

Excellent work.

I found why drupal_render_children is used within theme() functions: http://api.drupal.org/comment/45288#comment-45288

Don't forget that in D7 modules should call drupal_render_children() and not drupal_render() in theme_() functions, otherwise they will end up in an infinite loop.

We should open up a follow-up as the drupal_render_children should be implicit as well.

Besides that: Very close to RTBC from my side.

star-szr’s picture

star-szr’s picture

FileSize
981 bytes
25.56 KB

This just fixes the alignment of the docblock in node-search-admin.html.twig.

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

The last submitted patch, 1898432-22.patch, failed testing.

star-szr’s picture

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

#22: 1898432-22.patch queued for re-testing.

Angry Dan’s picture

We've had a look and there are a couple of questions/comments about this patch:

  • Lot's of functions seem to have 2 lines for their initial comment, e.g. template_preprocess_node_admin_overview() and template_preprocess_node_recent_content(). I thought this was a violation of coding standards?
  • I can't work out why we've started cloning $node in template_preprocess_node_preview() for $node_teaser. Was this a conscious decision or a separate bug being fixed because it doesn't seem related to the move to twig?
  • node.html.twig no longer references {{ title_attributes }}, why was this removed?
star-szr’s picture

@Angry Dan - For now I can only comment on your first point, see #1913208: [policy] Standardize template preprocess function documentation which will shorten those significantly. Great catch!

Fabianx’s picture

#25 Great review catches!

I think both are problems that accidentally slipped into this patch. Probably coming from the sandbox.

star-szr’s picture

Status: Needs review » Needs work

CNW based on #25.

star-szr’s picture

Status: Needs work » Needs review
FileSize
5.88 KB
26.11 KB

This should address all points from #25.

Revised docblocks on preprocess functions per pending standards at #1913208: [policy] Standardize template preprocess function documentation. Feedback welcome, I know the wording on some of them can definitely be improved!

star-szr’s picture

Issue summary: View changes

Correcting one of the names in the commit message.

jenlampton’s picture

Issue summary: View changes

add link to dt issue

jenlampton’s picture

Issue summary: View changes

blockers

jenlampton’s picture

Issue summary: View changes

blocker

jenlampton’s picture

Status: Needs work » Needs review
FileSize
11.41 KB
157.48 KB

This patch is actually looking really good. If we can clean up the two blocking issues noted below (and in summary) we should be good to go.

1) Seven theme overrides node_add_list so this patch may not be able to be added until after we figure out how to allow both theme engines to run at the same time in themes. See #1938948: Temporarily allow PHPTemplate themes to use .html.twig templates during Twig conversion

2) the infinite loop problem, as indicated by @todos in the comments below. see #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead

That said, a few other small changes:

node-add-list.html.twig
- variables in docblock updated

node-edit-form.html.twig
- missing, converted from .tpl.php
- @todo in here

template_preprocess_node_recent_block
- contained a call to theme(), replaced with a render array.

node-search-admin.html.twig
- not printing out info as added into the form (bug in preprocess, var missing from template)
- @todo in here

node.module
- bug in node_search_admin(), render array key should be #markup not #value.

node.html.twig
- lots of docblock cleanup

Reroll includes changes noted above.

Also needed:
seven/templates/node-add-list.html.twig
- @todo

star-szr’s picture

FileSize
2.42 KB
31.7 KB

Thanks @jenlampton, great work!

Rerolled to tweak some docs and coding standards things and remove a stray debug() and a stray .orig file :)

Left the .orig out of the interdiff.

Status: Needs review » Needs work

The last submitted patch, 1898432-31.patch, failed testing.

jenlampton’s picture

Status: Needs review » Needs work
FileSize
4.57 KB
34.02 KB

This test is failing because there is a space in our class tag. I'm not sure our tests should be that sensitive! Shouldn't they look for a div with A class 'content' not THE div with this *exact* class string?

This new patch allows the test to pass, but I wonder if our testing framework shouldn't be adjusted?
Or, our twig process can be adjusted... see #1939214: Provide a Twig filter for cleaning up whitespace around classes.

While I was in here a few more changes:
- removed stuff that's in $node from preprocess, since we can get there via Twig now. Updated docs to reflect this too.

Also, created a follow-up: #1939238: Remove $submitted variable and print string directly in template instead

jenlampton’s picture

Issue summary: View changes

test

jenlampton’s picture

Status: Needs work » Needs review
FileSize
5.27 KB
34.72 KB

this one should pass tests. (interdiff replaces last one)

jenlampton’s picture

Issue summary: View changes

more follow up

Status: Needs review » Needs work

The last submitted patch, core-update_node_to_twig-1898432-34.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
FileSize
2.08 KB
34.13 KB

Looks like we won't be able to rename the "name" variable yet (or convert to a render array), at least until #1941286: Remove the process layer (rdf module) is figured out.

So reverting that change. Thanks for keeping this moving, @jenlampton!

star-szr’s picture

FileSize
34.13 KB

Rerolled so that this patch doesn't try to make the same change already committed in #1945066: node_theme() doesn't declare the file for theme_node_admin_overview() . No other changes.

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

The last submitted patch, 1898432-37.patch, failed testing.

star-szr’s picture

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

#37: 1898432-37.patch queued for re-testing.

steveoliver’s picture

+1 for RTBC if this passes, which it should (just code style and little nitpicks).

star-szr’s picture

Thanks @steveoliver! All these changes look good, except:

+++ b/core/modules/node/templates/node-search-admin.html.twigundefined
@@ -1,7 +1,8 @@
- * Returns HTML for the content ranking part of the search settings admin page.
+ * Default theme implementation for the content ranking part of the search
+ * settings admin page.

This summary line is now too long per http://drupal.org/node/1354#drupal.

star-szr’s picture

However I think that theme_node_search_admin() will get converted in #1938920: Convert node_search_admin theme tables to table #type anyway, so we can probably remove that and maybe node_recent_block from this conversion.

star-szr’s picture

Status: Needs review » Needs work

Needs work for #41 and #42. Will roll a new patch in the next few days unless someone else wants to :)

chrisjlee’s picture

Not sure if that sentence even makes sense:

+ * Default theme implementation for the content ranking part of the search
+ * settings admin page.

Wouldn't it be more clearer to understand if they were two phrases?

+ * Default theme implementation for the content ranking. Part of the search
+ * settings admin page.
star-szr’s picture

@chrisjlee - Yup, that's the direction I've been going. The second sentence would be on its own line with a blank line underneath the summary, something like this:

 * Default theme implementation for the content ranking form.
 *
 * Part of the search settings admin page.

But as mentioned in #42 (and swiftly forgotten when posting #43 :)), the theme_node_search_admin() conversion can be removed from this patch since it will be handled in #1938920: Convert node_search_admin theme tables to table #type.

So this is just CNW for removing that conversion from the patch.

star-szr’s picture

Status: Needs work » Needs review
FileSize
5.54 KB
31.34 KB

Removed the theme_node_search_admin() conversion which will be handled in #1938920: Convert node_search_admin theme tables to table #type.

And did another pass at the documentation, shortening summary lines to under 80 characters and improving wording.

joelpittet’s picture

Nice work @Cottser and @steveoliver, it ain't easy being green:)

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeFieldMultilingualTestCase.php
@@ -134,7 +134,7 @@ function testMultilingualDisplaySettings() {
       ':id' => 'node-' . $node->nid,
-      ':class' => 'content',
+      ':class' => 'content ',
     ));
     $this->assertEqual(current($body), $node->body['en'][0]['value'], 'Node body found.');

+++ b/core/modules/node/node.module
@@ -1113,7 +1118,7 @@ function node_is_page(EntityInterface $node) {
+ * Implements hook_preprocess_HOOK() for block.html.twig.

Is this needed?

star-szr’s picture

The test change? See #33. If we can rework the test to be less sensitive that'd be even better.

star-szr’s picture

Issue summary: View changes

depends

star-szr’s picture

FileSize
3.74 KB
31.41 KB

More documentation tweaks, and left one documentation change off for #1898034: block.module - Convert PHPTemplate templates to Twig to take care of.

star-szr’s picture

FileSize
3.98 KB
35.39 KB

Some node.tpl.php -> node.html.twig replacements. I left two alone:

The node.tpl.php in drupal_pre_render_links() because that has a PHPTemplate example and that will need to be part of a bigger effort to update PHPTemplate documentation in the code to Twig documentation.

The node.tpl.php string in theme_field() is already removed in #1898062: field.module - Convert PHPTemplate templates to Twig.

star-szr’s picture

Status: Needs review » Needs work

I talked to @Fabianx this morning, and he had a great idea.

I'm going to try one more thing for node-edit-form to see if we can get hide() working in the Twig file - basically a cleaned workaround for #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead, currently we are doing this:

+++ b/core/modules/node/node.moduleundefined
@@ -1129,19 +1134,39 @@ function node_preprocess_block(&$variables) {
+function template_preprocess_node_edit_form(&$variables) {
+  $form = $variables['form'];
+
+  // @todo Remove this preprocess function once http://drupal.org/node/1920886
+  //   is resolved.
+  $variables['advanced'] = $form['advanced'];
+  $variables['actions'] = $form['actions'];
+  unset($form['advanced'], $form['actions']);
+  $variables['form'] = drupal_render_children($form);
+}
star-szr’s picture

Waiting on results from #1920886-10: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead before posting another patch here. Things are looking good though :)

star-szr’s picture

Issue summary: View changes

Move drupal_render_children() issue to follow-ups. I don't think we have to solve it before committing this.

star-szr’s picture

Will post a patch tonight that puts the logic from #1920886-10: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead into template_preprocess_node_edit_form() only.

star-szr’s picture

Status: Needs work » Needs review
FileSize
2.28 KB
35.53 KB

And here it is.

Without removing the '#theme' key, WSOD when viewing node add/edit form. Without removing the '#theme_wrappers' key, two <form> tags.

I think this workaround will do for conversion purposes until we have a better solution.

star-szr’s picture

Issue summary: View changes

Add conversion summary table

star-szr’s picture

Issue summary: View changes

Update remaining

star-szr’s picture

Issue summary: View changes

Remove node-search-admin from testing steps

star-szr’s picture

Issue tags: +Needs manual testing

This could use manual testing, steps for testing are up in the summary.

star-szr’s picture

Assigned: star-szr » steveoliver

Assigning to @steveoliver for review.

carsonblack’s picture

Patch applies cleanly and I have confirmed that all HTML is output as expected.
In the case of node-add-list.html.twig it is necessary to switch admin theme to Stark inorder to see the output as Seven theme overrides this output in it's seven_node_add_list() function.

Other than that little gotcha in the testing process, this is is all good!

RTBC +1

steveoliver’s picture

Manually tested this, it works, but maybe not totally as expected (which has nothing to do with conversion, actually. It's just something I noticed in the process). Namely, one issue:

The node-edit-form template is not used by default (as you'll notice when testing node/1/edit in Stark, for example). Only forms that specify 'node_edit_form' as the #theme callback will use this theme callback. Otherwise theme_form() is used. (See node_page_edit and seven.theme).

At the moment, Seven is the only theme that implements the 'node_edit_form' theme callback. Realizing this, I wonder now if it shouldn't be renamed and/or moved to clarify what it is doing. Maybe 'node_edit_form_twocol' or something? Maybe not. I don't really care. In any case I clarified the docs in that template.

The other things I noticed were primarily docs related. Patch and diff show what I found.

Lastly, I changed up the #theme / #theme_wrappers 'hack' to prevent looping over every $form element, just targeting those two keys directly. (Related to #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead).

If there are no objections, +1 to RTBC. I'll RTBC it if nobody else wants to.

star-szr’s picture

Oh duh, your hack is much better, thanks @steveoliver :)

+++ b/core/modules/node/templates/node-edit-form.html.twigundefined
@@ -3,12 +3,15 @@
- * - form: Complete form. Use {{ form }} to print the entire form, or print a
- *   subset such as {{ form.actions }}. Use {% hide(form.actions) %} to
- *   temporarily suppress the printing of a given element.
- *   - form.advanced: The advanced options for the node form.
- *   - form.actions: The action buttons for save and delete.
+ * - form: The node add/edit form.

I think this should be reformatted so that we say 'advanced' instead of 'form.advanced', but I think we should keep the documentation about hide(). Not many templates use hide() and it probably deserves to be explained in the docs for each one.

thedavidmeister’s picture

+ <div class="content {{ content_attributes.class }}"{{ content_attributes }}>

I'm not a fan of the mixing hardcoded classes and class attributes that's going on in some of the Twig conversion patches. It splits class management across the preprocess and templates which is harder to work with than both approaches are individually and very often leads to trailing whitespace in class attributes.

I noticed you had to hack a test to get this to come back green...

I'm not saying don't RTBC, I'm just saying we should come up with a more elegant way to handle classes like this sometime down the track.

star-szr’s picture

Status: Needs review » Needs work

This will need a simple reroll to account for #1968322: Remove unused $id and $zebra variables from templates.

steveoliver’s picture

Update: will be reviewing this Thursday, incase anyone else wants to reroll/RTBC in the meantime.

chrisjlee’s picture

Status: Needs work » Needs review
FileSize
36.39 KB

Attempted to reroll.

There was two merge conflicts one was removing the node.tpl.php file and the node.html.twig documentation conflict:

node.html.twig

<<<<<<< HEAD
 * - node: Fully loaded node entity.
 * - type: Node type; for example, page, article, etc.
 * - comment_count: Number of comments attached to the node.
 * - uid: User ID of the node author.
 * - created: Time the node was published formatted as a Unix timestamp.
 *
 * Node status variables:
=======
 * - node: Full node entity.
 *   - type: Node type; for example, "page" or "article".
 *   - uid: User ID of the node author.
 *   - created: Formatted creation date.
 *   - promote: Flag for front page promotion state.
 *   - sticky: Flag for sticky post setting.
 *   - status: Flag for published status.
 *   - comment: State of comment settings for the node.
 *   - comment_count: Number of comments attached to the node.
 * - zebra: Outputs either "even" or "odd". Useful for zebra striping in
 *   teaser listings.
 * - id: Position of the node. Increments each time it's output.
>>>>>>> Patch number 58

I'm guessing that with recent commit of #1968322: Remove unused $id and $zebra variables from templates we'll probably want to go with the head. Anyways patch attached.

chrisjlee’s picture

After a discussion with @Cottser, looks like with moving to HEAD we nuked and lose some documentation that needs to be restored. I agree with that.

So the following patch contains some docs edits that restore the previous variables missing. Also, there are some small nitpick tweak to the docs.

cosmicdreams’s picture

I'll test this locally but when I test via Simplytest.me I get multiple ajax errors

An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: http://s0edce4a6fdcc2a8.s3.simplytest.me/core/install.php?langcode=en&profile=standard&id=1&op=do_nojs&op=do StatusText: OK ResponseText: Home | Drupal @import url("http://s0edce4a6fdcc2a8.s3.simplytest.me/core/misc/normalize/normalize.css?0"); @import url("http://s0edce4a6fdcc2a8.s3.simplytest.me/core/modules/system/system.base.css?0"); @import url("http://s0edce4a6fdcc2a8.s3.simplytest.me/core/modules/system/system.admin.css?0"); @import url("http://s0edce4a6fdcc2a8.s3.simplytest.me/core/modules/system/system.theme.css?0"); @import url("http://s0edce4a6fdcc2a8.s3.simplytest.me/core/modules/system/system.maintenance.css?0"); @import url("http://s0edce4a6fdcc2a8.s3.simplytest.me/core/themes/seven/style.css?0"); Home Installation tasksChoose language(done)Choose profile(done)Verify requirements(done)Set up database(done)Installation profile(active)Configure siteFinished SQLSTATE[42000]: Syntax error or access violation: 1071 Specified key was too long; max key length is 1000 bytes: CREATE TABLE {file_usage} ( `fid` INT unsigned NOT NULL COMMENT 'File ID.', `module` VARCHAR(255) NOT NULL DEFAULT '' COMMENT 'The name of the module that is using the file.', `type` VARCHAR(64) NOT NULL DEFAULT '' COMMENT 'The name of the object type in which the file is used.', `id` VARCHAR(64) NOT NULL DEFAULT 0 COMMENT 'The primary key of the object using the file.', `count` INT unsigned NOT NULL DEFAULT 0 COMMENT 'The number of times this file is used by this object.', PRIMARY KEY (`fid`, `type`, `id`, `module`), INDEX `type_id` (`type`, `id`), INDEX `fid_count` (`fid`, `count`), INDEX `fid_module` (`fid`, `module`) ) ENGINE = InnoDB DEFAULT CHARACTER SET utf8 COMMENT 'Track where a file is used.'; Array ( )
cosmicdreams’s picture

When manually testing it appears that the names of the content types are missing. Going to see if I can revert this patch and have that still work.

when creating nodes all side menu stuff is listed below the body field (instead of being listed on the side)

When previewing the node I get some errors saying:

arning: include(C:\Users\cweber\Sites\d8/core/modules/node/templates/node-edit-form.tpl.php) [function.include]: failed to open stream: No such file or directory in theme_render_template() (line 1500 of core\includes\theme.inc).
Warning: include() [function.include]: Failed opening 'C:\Users\cweber\Sites\d8/core/modules/node/templates/node-edit-form.tpl.php' for inclusion (include_path='.;C:\Program Files (x86)\acquia-drupal\common\pear') in theme_render_template() (line 1500 of core\includes\theme.inc).

saving works

I don't seem to have the option to add a recent content block.

steveoliver’s picture

Status: Needs review » Needs work

Thanks, guys.

re: #65: this is an unrelated bug; see #1969680: Installation fails with MyISAM - key too long.

re: #63 and #64:

+++ b/core/modules/node/templates/node.html.twig
@@ -21,31 +21,33 @@
  *
  * Other variables:
- * - node: Fully loaded node entity.
- * - type: Node type; for example, page, article, etc.
- * - comment_count: Number of comments attached to the node.
- * - uid: User ID of the node author.
- * - created: Time the node was published formatted as a Unix timestamp.
- *
- * Node status variables:
+ * - node: Full node entity.
+ *   - type: Node type; for example, "page" or "article".
+ *   - uid: User ID of the node author.
+ *   - created: Formatted creation date.
+ *   - promote: Flag for front page promotion state.
+ *   - sticky: Flag for sticky post setting.
+ *   - status: Flag for published status.
+ *   - comment: State of comment settings for the node.
+ *   - comment_count: Number of comments attached to the node.
  * - view_mode: View mode; for example, "teaser" or "full".
  * - teaser: Flag for the teaser state. Will be true if view_mode is 'teaser'.

1. I think the node variables should be ID'ed first. This should make the attributes+class and other variables' descriptions more concise and easier to understand.
2. Let's lose "Other variables:"
3. I'd like to see all these node+properties descriptions given a little attention while we're here. Maybe complete sentences? For example, I don't know what value I'll get from 'comment' with a description like "State of comment settings for the node." Just a little love on docs here will go a long way.

+++ b/core/modules/node/templates/node.html.twig
@@ -57,11 +59,11 @@
  * use these variables. Otherwise they will have to explicitly specify the
- * desired field language; for example, 'node.body.en', thus overriding any
+ * desired field language; for example, 'nod.txe.body.en', thus overriding any

typo, looks like a mistake.

Re: #66, I can't reproduce it locally on a clean install. Maybe a local cache issue? Can you reproduce?

steveoliver’s picture

Here's my complete pass on the whole patch from #64 (not just the interdiff, as was the last comment):

--- a/core/modules/node/node.module
+++ b/core/modules/node/node.module
@@ -157,20 +157,25 @@ function node_theme() {
     'node_add_list' => array(
       'variables' => array('content' => NULL),
       'file' => 'node.pages.inc',
+      'template' => 'node-add-list',
     ),
     'node_preview' => array(
       'variables' => array('node' => NULL),
       'file' => 'node.pages.inc',
+      'template' => 'node-preview',
     ),
     'node_admin_overview' => array(
       'variables' => array('name' => NULL, 'type' => NULL),
       'file' => 'content_types.inc',
+      'template' => 'node-admin-overview',
     ),
     'node_recent_block' => array(
       'variables' => array('nodes' => NULL),
+      'template' => 'node-recent-block',
     ),
     'node_recent_content' => array(
       'variables' => array('node' => NULL),
+      'template' => 'node-recent-content',
     ),
     'node_edit_form' => array(
       'render element' => 'form',
@@ -1129,19 +1134,38 @@ function node_preprocess_block(&$variables) {
 }
 
 /**
- * Processes variables for node.tpl.php.
+ * Prepares variables for node edit form templates.
  *
- * Most themes utilize their own copy of node.tpl.php. The default is located
- * inside "modules/node/node.tpl.php". Look in there for the full list of
- * variables.
+ * Default template: node-edit-form.html.twig.
  *
- * @param $variables
+ * @param array $variables
+ *   An associative array containing:
+ *   - form: The complete node form.
+ */
+function template_preprocess_node_edit_form(&$variables) {
+  // Prevent recursion loop by removing '#theme' and '#theme_wrappers' keys.
+  // See http://drupal.org/node/1920886.
+  foreach (array('#theme', '#theme_wrappers') as $theme_key) {
+    if (isset($variables['form'][$theme_key])) {
+      unset($variables['form'][$theme_key]);
+    }
+  }
+}
+
+/**
+ * Prepares variables for node templates.
+ *
+ * Default template: node.html.twig.
+ *
+ * Most themes utilize their own copy of node.html.twig. The default is located
+ * inside "core/modules/node/templates/node.html.twig". Look in there for the
+ * full list of variables.
+ *
+ * @param array $variables
  *   An associative array containing:
  *   - elements: An array of elements to display in view mode.
  *   - node: The node object.
  *   - view_mode: View mode; e.g., 'full', 'teaser'...
- *
- * @see node.tpl.php
  */
 function template_preprocess_node(&$variables) {
   $variables['view_mode'] = $variables['elements']['#view_mode'];
@@ -1150,25 +1174,18 @@ function template_preprocess_node(&$variables) {
   $variables['node'] = $variables['elements']['#node'];
   $node = $variables['node'];
 
-  $variables['date']      = format_date($node->created);
-  $variables['name']      = theme('username', array(
+  $variables['date'] = format_date($node->created);
+  // @todo Change 'name' to 'author' and also convert to a render array pending
+  //   http://drupal.org/node/1941286.
+  $variables['name'] = theme('username', array(
     'account' => $node,
     'link_attributes' => array('rel' => 'author'),
   ));
 
   $uri = $node->uri();
   $variables['node_url']  = url($uri['path'], $uri['options']);
-  $variables['label']     = check_plain($node->label());
-  $variables['page']      = $variables['view_mode'] == 'full' && node_is_page($node);
-
-  // Make useful flags and node data available.
-  // @todo: The comment properties only exist if comment.module is enabled, but
-  //   are documented in node.tpl.php, so we make sure that they are set.
-  //   Consider removing them.
-  $properties = array('type', 'comment_count', 'uid', 'created', 'promote', 'sticky', 'status', 'comment');
-  foreach ($properties as $property) {
-    $variables[$property] = isset($node->$property) ? $node->$property : NULL;
-  }
+  $variables['label'] = check_plain($node->label());
+  $variables['page'] = $variables['view_mode'] == 'full' && node_is_page($node);
 
   // Helpful $content variable for templates.

1. we're introducing template_preprocess_node() and we don't need to. node edit forms don't use any special form or theme callback by default. Seven is the only theme in core that implements this theme callback, and I strongly believe it should implement this theme callback, template and associated preprocess (which is a POS here anyways). Stark and other themes see straight entity forms ala #1499596: Introduce a basic entity form controller, I believe.

2. node.html.twig, however....

2.a

-  // Make useful flags and node data available.
-  // @todo: The comment properties only exist if comment.module is enabled, but
-  //   are documented in node.tpl.php, so we make sure that they are set.
-  //   Consider removing them.
-  $properties = array('type', 'comment_count', 'uid', 'created', 'promote', 'sticky', 'status', 'comment');
-  foreach ($properties as $property) {
-    $variables[$property] = isset($node->$property) ? $node->$property : NULL;
-  }

Why are we losing this?

2.b. To follow up on my last comment, let's give these docs some love. They are already really good in some spots, but lacking in organization and clarity. In addition to the ordering suggested above, I'd like to have a more intelligent distinction or consolidation of the content and node variables. I think that'd be a big ++ here.

--- a/core/modules/node/templates/node.html.twig
+++ b/core/modules/node/templates/node.html.twig
@@ -4,54 +4,53 @@
  * Default theme implementation to display a node.
  *
  * Available variables:
- * - label: the title of the node.
- * - content: node items. Use {{ content }} to print them all,
+ * - label: The node title.
+ * - content: All node items. Use {{ content }} to print them all,
  *   or print a subset such as {{ content.field_example }}. Use
  *   {% hide(content.field_example) %} to temporarily suppress the printing
  *   of a given element.
+ * - name: Themed username of node author output from username.html.twig.
  * - user_picture: The node author's picture from user-picture.html.twig.
  * - date: Formatted creation date. Preprocess functions can reformat it by
  *   calling format_date() with the desired parameters on
  *   $variables['created'].
- * - name: Themed username of node author output from theme_username().
  * - node_url: Direct URL of the current node.
  * - display_submitted: Whether submission information should be displayed.
  * - submitted: Submission information created from name and date during
  *   template_preprocess_node().
- * - attributes: HTML attributes for the surrounding element.
- *    Attributes include the 'class' information, which contains:
- *   - node: The current template type; for example, "theming hook".
+ * - attributes: HTML attributes for the containing element.
+ *   The attributes.class element may contain one or more of the following
+ *   classes:
+ *   - node: The current template type (also known as a "theming hook").
  *   - node-[type]: The current node type. For example, if the node is a
  *     "Article" it would result in "node-article". Note that the machine
  *     name will often be in a short form of the human readable label.
- *   - view-mode-[view_mode]: The View Mode of the node; for example, "teaser"
- *     or "full".
- *   - preview: Nodes in preview mode.
+ *   - view-mode-[view_mode]: The View Mode of the node; for example, a teaser
+ *     would result in: "view-mode-teaser", and full: "view-mode-full".
+ *   - preview: Whether a node is in preview mode.
  *   The following are controlled through the node publishing options.
- *   - promoted: Nodes promoted to the front page.
- *   - sticky: Nodes ordered above other non-sticky nodes in teaser
+ *   - promoted: Appears on nodes promoted to the front page.
+ *   - sticky: Appears on nodes ordered above other non-sticky nodes in teaser
  *     listings.
- *   - unpublished: Unpublished nodes visible only to administrators.
+ *   - unpublished: Appears on unpublished nodes visible only to site admins.
  * - title_prefix: Additional output populated by modules, intended to be
  *   displayed in front of the main title tag that appears in the template.
  * - title_suffix: Additional output populated by modules, intended to be
  *   displayed after the main title tag that appears in the template.
  *
  * Other variables:
- * - node: Fully loaded node entity.
- * - type: Node type; for example, page, article, etc.
- * - comment_count: Number of comments attached to the node.
- * - uid: User ID of the node author.
- * - created: Time the node was published formatted as a Unix timestamp.
- *
- * Node status variables:
+ * - node: Full node entity.
+ *   - type: Node type; for example, "page" or "article".
+ *   - uid: User ID of the node author.
+ *   - created: Formatted creation date.
+ *   - promote: Flag for front page promotion state.
+ *   - sticky: Flag for sticky post setting.
+ *   - status: Flag for published status.
+ *   - comment: State of comment settings for the node.
+ *   - comment_count: Number of comments attached to the node.
  * - view_mode: View mode; for example, "teaser" or "full".
  * - teaser: Flag for the teaser state. Will be true if view_mode is 'teaser'.
  * - page: Flag for the full page state. Will be true if view_mode is 'full'.
- * - promote: Flag for front page promotion state.
- * - sticky: Flag for sticky post setting.
- * - status: Flag for published status.
- * - comment: State of comment settings for the node.
  * - readmore: Flag for more state. Will be true if the teaser content of the
  *   node cannot hold the main body content.
  * - is_front: Flag for front. Will be true when presented on the front page.
@@ -60,26 +59,28 @@
  * - is_admin: Flag for admin user status. Will be true when the current user
  *   is an administrator.
  *
- * Field variables: for each field instance attached to the node a corresponding
- * variable is defined; for example, $node->body becomes body. When needing to
+ * In field variables, each field instance attached to the node a corresponding
+ * variable is defined; for example, 'node.body' becomes 'body'. When needing to
  * access a field's raw values, developers/themers are strongly encouraged to
  * use these variables. Otherwise they will have to explicitly specify the
- * desired field language; for example, $node->body['en'], thus overriding any
- * language negotiation rule that was previously applied.
+ * desired field language; for example, 'nod.txe.body.en', thus overriding any
+ * language negotiation rule that may have been applied previously.
  *
  * @see template_preprocess()
  * @see template_preprocess_node()
  *
+ * @todo Remove the id attribute (or make it a class), because if that gets
+ *   rendered twice on a page this is invalid CSS for example: two lists
+ *   in different view modes.
+ *
  * @ingroup themeable
  */
 #}
star-szr’s picture

I agree about template_preprocess_node_edit_form(), that's my fault for not realizing only Seven uses that template.

Regarding 2.a from above - "Why are we losing this?"

I did not make this change to the preprocess function but I can give you my understanding of it and why I think it's a good change. Now that accessing those properties directly is {{ node.type }} rather than <?php print $node->type; ?> it seems like unnecessary preprocessing to me to create a duplicate 'type' variable available to the Twig template when it can be accessed at 'node.type'.

(and after writing that, scrolled up and found #33 which backs this up - "- removed stuff that's in $node from preprocess, since we can get there via Twig now. Updated docs to reflect this too.")

star-szr’s picture

Assigned: steveoliver » Unassigned

Unassigning so someone can work on a revised patch, I probably won't get to this over the next few days myself.

shanethehat’s picture

Assigned: Unassigned » shanethehat

I'll have a go

shanethehat’s picture

Assigned: shanethehat » Unassigned
Status: Needs work » Needs review
FileSize
36 KB
5.65 KB

I think I covered all the points in #67 and #68, although I wasn't sure about the order of the documentation in node.html.twig. Moved the preprocess for the edit form into seven.theme until it can be removed completely by #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead.

cosmicdreams’s picture

simplytest.me seems to be having some problems with Drupal 8 so I'll test this locally.

cosmicdreams’s picture

Testing this now and here's the results

To test this code

  • Navigate to node/add to see if node-add-list.html.twig is working: SUCCESS
  • Navigate to admin/structure/types to see if node-admin-overview.html.twig is working: SUCCESS
  • Create a node to see if node-edit-form.html.twig is working: SUCCESS
  • Preview the node to see if node-preview.html.twig is working: : Fail
  • -- The preview is showing the trimmed and the full version of the node twice. I can't seem to be able to revert the code back to the state before the patch to compare but I'll reinstall Drupal to get a second look. It also didn't play well with Overlay. But that might be overlay's problem
  • Save your node to see if node.html.twig is working: SUCCESS
  • Add the Recent Content block to a region to see if node-recent-block.html.twig and node-recent-content.block.html.twig are working: SUCCESS

I'm not sure what the recent content block is supposed to look at but it renders so it's not a show stopper. It just looks bare bones right now.

thedavidmeister’s picture

#74 - when you say "working", did you A/B compare all the markup with the patch applied to a Drupal install without the patch?

shanethehat’s picture

FileSize
36.04 KB
3.29 KB

This patch addresses a couple of issues steveoliver raised in IRC yesterday. It does not fix #74.

star-szr’s picture

Issue tags: +Needs manual testing

Thanks @shanethehat and @cosmicdreams! It sounds like this needs more manual testing, re-tagging.

If it's feasible to move the seven code (node-edit-form.html.twig and the preprocess in seven.theme) to #1938848: seven.theme - Convert PHPTemplate templates to Twig can we please do so? If that works out we can have testers use Stark for testing functionality and markup.

shanethehat’s picture

#77 my last patch moves the edit form template, and the previous one moved the preprocess.

I misread. The relocation of the template and preprocess should be really easy to revert. I would recommend doing that rather than picking up an old patch because of the extra changes that have occurred.

shanethehat’s picture

FileSize
35.67 KB
4.07 KB

node-edit-form back in node module for later removal

shanethehat’s picture

I've followed the steps in the issue description and grabbed markup from a clean install and with #79 applied. I then stripped all newlines and compared the output with Kdiff. Other than indentation related whitespace differences, I found the following:

node/add
Identical

admin/structure/types
Identical

node/add/article
Identical

node/add/article - preview
Teaser and Full views have both have the same differences as node, below. Otherwise identical.

node/x
<article id="node-" class="node node-article promoted view-mode-teaser clearfix"
becomes
<article id="node-" class="clearfix node node-article promoted view-mode-teaser"

<div class="content" class="">
becomes
<div class="content">

Recent content block
Identical

I think that the differences in node are either inconsequential (order of classes) or beneficial (no more duplicate class attribute), so I think this must be very close to RTBC.

steveoliver’s picture

Status: Needs review » Needs work

Very nice, Shane.

A few little nitpicks here, a few that I think will address the 'inconsequential' concerns in #80.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeFieldMultilingualTestCase.php
@@ -134,7 +134,7 @@ function testMultilingualDisplaySettings() {
     $this->drupalGet("node/$node->nid");
     $body = $this->xpath('//article[@id=:id]//div[@class=:class]/descendant::p', array(
       ':id' => 'node-' . $node->nid,
-      ':class' => 'content',
+      ':class' => 'content ',

Is this whitespace addition maybe a typo?

+++ b/core/modules/node/node.module
@@ -157,24 +157,28 @@ function node_theme() {
     'node_edit_form' => array(
       'render element' => 'form',
-      'path' => drupal_get_path('module', 'node') . '/templates',
       'template' => 'node-edit-form',

Yeah, this 'path' declaration was unnecessary.

+++ b/core/modules/node/node.module
@@ -1119,19 +1123,38 @@ function node_preprocess_block(&$variables) {
+function template_preprocess_node_edit_form(&$variables) {
+  // Prevent recursion loop by removing '#theme' and '#theme_wrappers' keys.
+  // Remove this function when http://drupal.org/node/1920886 is resolved.
+  foreach (array('#theme', '#theme_wrappers') as $theme_key) {
+    if (isset($variables['form'][$theme_key])) {
+      unset($variables['form'][$theme_key]);
+    }
+  }

Looking forward to this being moved to Seven.

+++ b/core/modules/node/node.module
@@ -1140,25 +1163,18 @@ function template_preprocess_node(&$variables) {
-  $variables['date']      = format_date($node->created);
-  $variables['name']      = theme('username', array(
+  $variables['date'] = format_date($node->created);
+  // @todo Change 'name' to 'author' and also convert to a render array pending
+  //   http://drupal.org/node/1941286.
+  $variables['name'] = theme('username', array(
     'account' => $node,
     'link_attributes' => array('rel' => 'author'),
   ));
 
   $uri = $node->uri();

Why can't this theme call be turned into a render array now?

+++ b/core/modules/node/node.module
@@ -1948,36 +1968,44 @@ function theme_node_recent_block($variables) {
+  $variables['table'] = array(
+    '#theme' => 'table',
+  );

This is an interesting partial render array. Any way we can make more sense of this?

+++ b/core/modules/node/node.module
@@ -1948,36 +1968,44 @@ function theme_node_recent_block($variables) {
+  $variables['more_link'] = array();
   if ($rows) {
-    $output = theme('table', array('rows' => $rows));
+    $variables['table']['#rows'] = $rows;
     if (user_access('access content overview')) {
-      $output .= theme('more_link', array('url' => 'admin/content', 'title' => t('Show more content')));
+      $variables['more_link'] = array(
+        '#theme' => 'more_link',
+        '#url' => 'admin/content',
+        '#title' => t('Show more content'),
+      );
     }

Is an empty array a sane default value for this? Maybe NULL instead?

+++ b/core/modules/node/node.pages.inc
@@ -56,32 +56,27 @@ function node_add_page() {
-function theme_node_add_list($variables) {
-  $content = $variables['content'];
-  $output = '';
-
-  if ($content) {
-    $output = '<dl class="node-type-list">';
-    foreach ($content as $type) {
-      $output .= '<dt>' . l($type->name, 'node/add/' . $type->type) . '</dt>';
-      $output .= '<dd>' . filter_xss_admin($type->description) . '</dd>';
+function template_preprocess_node_add_list(&$variables) {
+  $variables['types'] = array();
+  if (!empty($variables['content'])) {
+    foreach ($variables['content'] as $type) {
+      $variables['types'][$type->type] = array(
+        'type' => $type->type,
+        'add_link' => l($type->name, 'node/add/' . $type->type),
+        'description' => filter_xss_admin($type->description),
+      );
     }
-    $output .= '</dl>';
   }
-  else {
-    $output = '<p>' . t('You have not created any content types yet. Go to the <a href="@create-content">content type creation page</a> to add a new content type.', array('@create-content' => url('admin/structure/types/add'))) . '</p>';
-  }

It'd be nice to replace this with #theme 'definition_list' ala #54898: Add a description-list.html.twig template (ex. definition list)... ;) But not necessary. This is fine for now.

+++ b/core/modules/node/node.pages.inc
@@ -157,41 +152,37 @@ function node_preview(EntityInterface $node) {
+  // Do we need to preview a trimmed teaser version of a post as well as a full
+  // version?
+  if ($variables['teaser'] != $variables['full']) {
     drupal_set_message(t('The trimmed version of your post shows what your post looks like when promoted to the main page or when exported for syndication.<span class="no-js"> You can insert the delimiter "&lt;!--break--&gt;" (without the quotes) to fine-tune where your post gets split.</span>'));

"Do we need to preview a trimmed teaser version of a post..." Yeah, I think we do. Let's lose this comment?

+++ b/core/modules/node/templates/node-recent-block.html.twig
@@ -0,0 +1,21 @@
+{#
+/**
+ * @file
+ * Default theme implementation for a list of recent content.
+ *
+ * Available variables:
+ * - table: A rendered HTML table of recent content.
+ * - more_link: A rendered link to show more content.
+ *
+ * @see template_preprocess()
+ * @see template_preprocess_node_recent_block()
+ *
+ * @ingroup themeable
+ */
+#}
+{% if table %}
+  {{ table }}
+{% endif %}
+{% if more_link %}
+  {{ more_link }}

{{ table }} could become {{- table -}} and {{ more_link }} could become {{- more_link -}} to remove the indentation differences in this patch here.

+++ b/core/modules/node/templates/node.html.twig
@@ -4,54 +4,59 @@
  * Available variables:
- * - label: the title of the node.
- * - content: node items. Use {{ content }} to print them all,
+ * - node: Full node entity.
+ *   - type: The type of the node, for example, "page" or "article".
+ *   - uid: The user ID of the node author.
+ *   - created: Formatted creation date. Preprocess functions can reformat it by
+ *     calling format_date() with the desired parameters on
+ *     $variables['node']->created.
+ *   - promote: Whether the node is promoted to the front page.
+ *   - sticky: Whether the node is 'sticky'. Sticky nodes are ordered above
+ *     other non-sticky nodes in teaser listings
+ *   - status: Whether the node is published.
+ *   - comment: A value representing the comment status of the current node. May
+ *     be one of the following:
+ *     - 0: The comment form and any existing comments are hidden.
+ *     - 1: Comments are closed. No new comments may be posted, but existing
+ *       comments are displayed.
+ *     - 2: Comments are open on this node.
+ *   - comment_count: Number of comments attached to the node.
+ * - label: The title of the node.
+ * - content: All node items. Use {{ content }} to print them all,
  *   or print a subset such as {{ content.field_example }}. Use
  *   {% hide(content.field_example) %} to temporarily suppress the printing
  *   of a given element.
+ * - name: Themed username of node author output from username.html.twig.
  * - user_picture: The node author's picture from user-picture.html.twig.
  * - date: Formatted creation date. Preprocess functions can reformat it by
  *   calling format_date() with the desired parameters on
  *   $variables['created'].
- * - name: Themed username of node author output from theme_username().
  * - node_url: Direct URL of the current node.
  * - display_submitted: Whether submission information should be displayed.
  * - submitted: Submission information created from name and date during
  *   template_preprocess_node().
- * - attributes: HTML attributes for the surrounding element.
- *    Attributes include the 'class' information, which contains:
- *   - node: The current template type; for example, "theming hook".
+ * - attributes: HTML attributes for the containing element.
+ *   The attributes.class element may contain one or more of the following
+ *   classes:
+ *   - node: The current template type (also known as a "theming hook").
  *   - node-[type]: The current node type. For example, if the node is a
  *     "Article" it would result in "node-article". Note that the machine
  *     name will often be in a short form of the human readable label.
- *   - view-mode-[view_mode]: The View Mode of the node; for example, "teaser"
- *     or "full".
- *   - preview: Nodes in preview mode.
+ *   - view-mode-[view_mode]: The View Mode of the node; for example, a teaser
+ *     would result in: "view-mode-teaser", and full: "view-mode-full".
+ *   - preview: Whether a node is in preview mode.
  *   The following are controlled through the node publishing options.
- *   - promoted: Nodes promoted to the front page.
- *   - sticky: Nodes ordered above other non-sticky nodes in teaser
+ *   - promoted: Appears on nodes promoted to the front page.
+ *   - sticky: Appears on nodes ordered above other non-sticky nodes in teaser
  *     listings.
- *   - unpublished: Unpublished nodes visible only to administrators.
+ *   - unpublished: Appears on unpublished nodes visible only to site admins.
  * - title_prefix: Additional output populated by modules, intended to be
  *   displayed in front of the main title tag that appears in the template.
  * - title_suffix: Additional output populated by modules, intended to be
  *   displayed after the main title tag that appears in the template.
- *
- * Other variables:
- * - node: Fully loaded node entity.
- * - type: Node type; for example, page, article, etc.
- * - comment_count: Number of comments attached to the node.
- * - uid: User ID of the node author.
- * - created: Time the node was published formatted as a Unix timestamp.
- *
- * Node status variables:
  * - view_mode: View mode; for example, "teaser" or "full".
  * - teaser: Flag for the teaser state. Will be true if view_mode is 'teaser'.
  * - page: Flag for the full page state. Will be true if view_mode is 'full'.
- * - promote: Flag for front page promotion state.
- * - sticky: Flag for sticky post setting.
- * - status: Flag for published status.
- * - comment: State of comment settings for the node.
  * - readmore: Flag for more state. Will be true if the teaser content of the
  *   node cannot hold the main body content.
  * - is_front: Flag for front. Will be true when presented on the front page.
@@ -60,26 +65,28 @@

@@ -60,26 +65,28 @@
  * - is_admin: Flag for admin user status. Will be true when the current user
  *   is an administrator.
  *
- * Field variables: for each field instance attached to the node a corresponding
- * variable is defined; for example, $node->body becomes body. When needing to
+ * In field variables, each field instance attached to the node a corresponding
+ * variable is defined; for example, 'node.body' becomes 'body'. When needing to
  * access a field's raw values, developers/themers are strongly encouraged to
  * use these variables. Otherwise they will have to explicitly specify the
- * desired field language; for example, $node->body['en'], thus overriding any
- * language negotiation rule that was previously applied.
+ * desired field language; for example, 'node.body.en', thus overriding any
+ * language negotiation rule that may have been applied previously.
  *
  * @see template_preprocess()
  * @see template_preprocess_node()
  *
+ * @todo Remove the id attribute (or make it a class), because if that gets
+ *   rendered twice on a page this is invalid CSS for example: two lists
+ *   in different view modes.
+ *
  * @ingroup themeable
  */

Excellent cleanup on this docblock here. Very nice.

+++ b/core/modules/node/templates/node.html.twig
@@ -60,26 +65,28 @@
@@ -90,7 +97,7 @@

@@ -90,7 +97,7 @@
     </footer>
   {% endif %}
 
-  <div class="content"{{ content_attributes }}>
+  <div class="content {{ content_attributes.class }}"{{ content_attributes }}>

in the node.html.twig file, let's move this content class to preprocess and just print [edit: code wrapped properly:] <div{{ content_attributes }}> [/edit]. I think this would resolve the need for that 'typo' looking whitespace addition above.

shanethehat’s picture

Status: Needs work » Needs review
FileSize
35.18 KB
3.75 KB

Thanks for the review @steveoliver. I've implemented most of this, and have a couple of responses:

Why can't this theme call be turned into a render array now?

Turning this into a render array has a knock-on effect with the creation of $variables['submitted']. Personally I think that $variables['submitted'] shouldn't exist and this string should be constructed in the template, but I feel that would be too severe a change for a conversion task and should probably have a followup issue. Additionally, until #1941286: Remove the process layer (rdf module) lands, this causes an RDF error.

"Do we need to preview a trimmed teaser version of a post..." Yeah, I think we do. Let's lose this comment?

I think that a comment here is valuable to express the intent of the conditional, but it was not well written. I've rephrased it to be a statement of intent, not a question.

This is an interesting partial render array. Any way we can make more sense of this?

Is an empty array a sane default value for this? Maybe NULL instead?

I've just move these inside the row condition. If there are no rows there's no point defining the table or the link, and Twig will handle the missing properties.

star-szr’s picture

Excellent work @shanethehat and @steveoliver!

Fabianx’s picture

Issue tags: -Needs manual testing, -Twig

#82: twig-node-1898432-82.patch queued for re-testing.

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

The last submitted patch, twig-node-1898432-82.patch, failed testing.

shanethehat’s picture

Status: Needs work » Needs review

reroll

shanethehat’s picture

FileSize
35.18 KB

with a patch attached

c4rl’s picture

Title: Convert node module to Twig » node.module - Convert PHPTemplate templates to Twig
Status: Needs review » Needs work

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

c4rl’s picture

Issue summary: View changes

Update remaining

c4rl’s picture

Issue summary: View changes

Revised list of conversions

c4rl’s picture

Issue summary: View changes

Testing steps

c4rl’s picture

Issue summary: View changes

Correct testing steps

cosmicdreams’s picture

Is that reason enough to demote to needs work?

I've manually tested the patch in 87 and everything works as required in the OP's test case.

thedavidmeister’s picture

#89 - yes, there are theme functions in that patch but we're splitting template and theme conversions into separate tasks. The patch just needs to be rerolled to have the theme->template conversions in one patch and the template->template conversions in another.

"needs work" is just for organisational reasons, not because the code is bad.

shanethehat’s picture

Assigned: Unassigned » shanethehat
shanethehat’s picture

Assigned: shanethehat » Unassigned
Status: Needs work » Needs review
FileSize
20.98 KB

Just the existing templates converted. No theme_* changes.

shanethehat’s picture

This time without whitespace errors

shanethehat’s picture

Missed a few mentions to node.tpl.php

star-szr’s picture

Issue tags: +needs profiling

Tagging for profiling.

star-szr’s picture

FileSize
414 bytes
23.38 KB

This is just s/themable/themeable/ in node-edit-form.html.twig.

intergalactic overlords’s picture

node-page, node-teaser and node-edit-page output the same html with twig and tpl.php.

intergalactic overlords’s picture

Issue tags: -Needs manual testing

Has been tested manually

Hydra’s picture

Did a manual test, and everything is displayed properly.

star-szr’s picture

Status: Needs review » Needs work
+++ b/core/modules/node/node.moduleundefined
@@ -1111,20 +1110,39 @@ function node_preprocess_block(&$variables) {
 + /**
+  * Prepares variables for node edit form templates.
+  *
+  * Default template: node-edit-form.html.twig.
+  *
+  * @param array $variables
+  *   An associative array containing:
+  *   - form: The complete node form.
+  */
+function template_preprocess_node_edit_form(&$variables) {
+  // Prevent recursion loop by removing '#theme' and '#theme_wrappers' keys.
+  // Remove this function when http://drupal.org/node/1920886 is resolved.
+  foreach (array('#theme', '#theme_wrappers') as $theme_key) {
+    if (isset($variables['form'][$theme_key])) {
+      unset($variables['form'][$theme_key]);
+    }
+  }
+}
+

This entire function can be removed now that #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead is in :)

Hydra’s picture

Assigned: Unassigned » Hydra

Let's have a look at this

Hydra’s picture

Assigned: Hydra » Unassigned
FileSize
1.05 KB
22.77 KB

Okay here is an updated patch. Manual testing worked, don't forget clearing cache :)

Hydra’s picture

Status: Needs work » Needs review

Update status to get the bot running :)

tlattimore’s picture

Assigned: Unassigned » tlattimore

I'll take a stab at reviewing this.

tlattimore’s picture

The patch from #102 looks good to me and addressing cotter's request in #100. After desirable results are confirmed with profiling I think this is ready for RTBC.

tlattimore’s picture

Assigned: tlattimore » Unassigned

I can't do profiling so I am going go un-assign.

steveoliver’s picture

Assigned: Unassigned » steveoliver

profiling...

star-szr’s picture

Assigned: steveoliver » Unassigned
Status: Needs review » Needs work
+++ b/core/modules/field/field.moduleundefined
@@ -1087,7 +1087,7 @@ function template_process_field(&$variables, $hook) {
- * page.tpl.php or node.tpl.php. However, it takes longer for the server to
+ * page.tpl.php or node.html.twig. However, it takes longer for the server to

We should not change this hunk in this patch, it is causing an unnecessary conflict, #1898062: field.module - Convert PHPTemplate templates to Twig already updates this and updates both instances of .tpl.php to .html.twig.

Please remove this hunk from the patch :)

Hydra’s picture

Status: Needs work » Needs review
FileSize
21.9 KB
885 bytes

Awesome hint!

geoffreyr’s picture

Assigned: Unassigned » geoffreyr

Will have a stab at profiling Hydra's patch from #110.

geoffreyr’s picture

Assigned: geoffreyr » Unassigned
Issue tags: -needs profiling

Scenario: Displaying a stack of node teasers in a view, in both the main content area and right sidebar.
Looks like there's a significant speed saving here. Would probably want someone to check and corroborate this, but so far looks good.

=== 8.x..8.x compared (519d4cd4e396d..519d4d6ecb28c):

ct  : 145,877|145,877|0|0.0%
wt  : 989,142|989,898|756|0.1%
cpu : 916,136|915,819|-317|-0.0%
mu  : 9,857,484|9,857,484|0|0.0%
pmu : 10,150,176|10,150,176|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d4cd4e396d&...

=== 8.x..1898432 compared (519d4cd4e396d..519d4dc9980f7):

ct  : 145,877|143,637|-2,240|-1.5%
wt  : 989,142|973,737|-15,405|-1.6%
cpu : 916,136|901,543|-14,593|-1.6%
mu  : 9,857,484|9,856,816|-668|-0.0%
pmu : 10,150,176|10,149,532|-644|-0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d4cd4e396d&...

thedavidmeister’s picture

Status: Needs review » Reviewed & tested by the community
mr.baileys’s picture

Status: Reviewed & tested by the community » Needs review

The above set of profiling data seems to be for the node.twig.html only, not the node-edit-form.twig.html template?

I ran the numbers for node-edit-form (using Seven as theme, since Stark does not use the node-edit-form template):

=== 8.x..8.x compared (519d5c4cd1983..519d5c92d66a0):

ct  : 28,213|28,213|0|0.0%
wt  : 175,166|174,921|-245|-0.1%
cpu : 168,011|172,011|4,000|2.4%
mu  : 10,923,296|10,923,296|0|0.0%
pmu : 11,111,848|11,111,848|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d5c4cd1983&...

=== 8.x..1898432-node-templates compared (519d5c4cd1983..519d5cada282f):

ct  : 28,213|28,487|274|1.0%
wt  : 175,166|177,362|2,196|1.3%
cpu : 168,011|172,011|4,000|2.4%
mu  : 10,923,296|11,341,968|418,672|3.8%
pmu : 11,111,848|11,535,400|423,552|3.8%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d5c4cd1983&...

alexpott’s picture

What's causing the node edit slow down?

+++ b/core/modules/node/templates/node.html.twigundefined
@@ -60,26 +65,28 @@
-<article id="node-{{ node.nid }}" class="{{ attributes.class }} clearfix"{{ attributes }}>
+<article id="node-{{ node.nid }}" class="clearfix {{ attributes.class }}"{{ attributes }}>

Not sure that swapping the clearfix around makes sense. What's in attributes.class is likely to be more significant.

mr.baileys’s picture

What's causing the node edit slow down?

This could be caused by the fact that I was forced to benchmark using Seven instead of Stark, since the latter does not make use of the node edit template (rather renders the node edit page as a regular form). This means that our trick to load the Twig engine in both the 8.x and twig branch is not working here, and the comparison is "Twig Engine Loading" & "Conversion", instead of just profiling the conversion.

Not sure how to get around that issue and profile just the template conversion itself...

star-szr’s picture

This is just the one-time Twig overhead so it's fine, if we want to measure just the node-edit-form template we can copy node.html.twig to seven/templates and re-profile.

cweagans’s picture

Status: Needs review » Needs work

CNW for second point in #114

steveoliver’s picture

Issue tags: +needs profiling

Since the profile results aren't clear, someone re-profile this, please.

Two scenarios:

1. Node edit form
2. Node view, with template in stark theme.

Then, a few nitpicks. Easy stuff:

3. I agree with Alex's clearfix comment in #114. Let's leave clearfix at the end.

Then...

+++ b/core/modules/node/node.module
@@ -1091,19 +1090,19 @@ function node_preprocess_block(&$variables) {
- * Processes variables for node.tpl.php.
+ * Prepares variables for node templates.
  *
- * Most themes utilize their own copy of node.tpl.php. The default is located
- * inside "modules/node/node.tpl.php". Look in there for the full list of
- * variables.
+ * Default template: node.html.twig.
  *
- * @param $variables
+ * Most themes utilize their own copy of node.html.twig. The default is located
+ * inside "modules/node/templates/node.html.twig". Look in there for the full
+ * list of variables.
+ *
+ * @param array $variables
  *   An associative array containing:

4. May as well update the path here to include the new /core/ before /modules/...

+++ b/core/modules/node/templates/node.html.twig
@@ -60,26 +65,28 @@
-    <h2{{ title_attributes }}>
-      <a href="{{ node_url }}" rel="bookmark">{{ label }}</a>
-    </h2>
+    <h2{{ title_attributes }}><a href="{{ node_url }}" rel="bookmark">{{ label }}</a></h2>
   {% endif %}
   {{ title_suffix }}

5. Let's not lose the indentation here. Please re-indent.

steveoliver’s picture

Status: Needs work » Needs review
Issue tags: -needs profiling
FileSize
1.3 KB
23.31 KB

3., 4., 5.

Fabianx’s picture

Thanks @steveoliver.

To explain profiling results from #111 - http://drupal.org/node/1898432#comment-7441576:

* We are faster, because we do a _lot_ less preprocessing now.

And from #113 - http://drupal.org/node/1898432#comment-7441780:

* We are at the usual Twig overhead, besides for the fact that Twig is loaded here completely as can be seen by checking the run_1 output and searching for twig_render_template. So just substract the twig loading overhead of 1.3-1.5ms here and the edit form is fine.

Besides that 1.3 ms on an edit form ... is not a big deal. @alexpott just wanted to understand why it is slower than other templates.

The answer is: It isn't - there is just the Twig loading overhead.

I don't think it is worth re-profiling that as results are consistent with other templates, where default loading overhead was substracted.

I will re-review the patch now and provided it passes testbot this is probably RTBC.

Fabianx’s picture

Issue tags: +Novice

There is a stray field.html.twig in the last patch. Please remove.

Afterwards this is RTBC. I re-reviewed it all.

star-szr’s picture

Issue tags: -Novice
FileSize
1.86 KB
21.45 KB

Stray field.html.twig gone.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC per #121, provided this passes testbot.

alexpott’s picture

Title: node.module - Convert PHPTemplate templates to Twig » [READY] node.module - Convert PHPTemplate templates to Twig
Status: Reviewed & tested by the community » Closed (duplicate)
alexpott’s picture

Title: [READY] node.module - Convert PHPTemplate templates to Twig » node.module - Convert PHPTemplate templates to Twig
Status: Closed (duplicate) » Fixed

Committed 6b4b127 and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Clarify summary