Remove some pointless preprocessing from template_preprocess_node()

Why are we removing documentation for variables that still exists?

Made a few improvements, fixed bartik_preprocess_node(), and made sure that uid/created are always hidden. And updated node.html.twig in bartik.

@laurii: What are we removing that still exists? teaser no longer exists, the others are just moved.

Fixed that test. Switched to test both page and article to test different settings, only those that show the author now add the relevant user tags.

I think I misread the patch because there's too many spaces before date and authorname on the documentation

Rerolled the patch from #5. There were some conflicts.

Needs another reroll I suppose :)

Reroll

That time again apparently. Needs reroll.

Here's a re-roll it fell back to 3-way auto merge so nothing to resolve.

1. +++ b/core/modules/node/node.module
   @@ -591,18 +586,14 @@ function template_preprocess_node(&$variables) {
   -  // Helpful $content variable for templates.
   -  $variables += array('content' => array());
   
   @@ -610,6 +601,14 @@ function template_preprocess_node(&$variables) {
   +  // Helpful $content variable for templates.
   +  $variables += array('content' => array());
   

   Is moving this hunk needed?

2. +++ b/core/modules/node/node.module
   @@ -591,18 +586,14 @@ function template_preprocess_node(&$variables) {
   +    $variables['date'] = drupal_render($variables['elements']['created']);
   +    $variables['author_name'] = drupal_render($variables['elements']['uid']);
   

   We don't need to drupal_render these variables, let the template do that.

3. +++ b/core/modules/node/templates/node.html.twig
   @@ -15,13 +15,16 @@
   - * - author_picture: The node author user entity, rendered using the "compact"
   - *   view mode.
   ...
   - * - date: Themed creation date field.
   - * - author_name: Themed author name field.
   ...
   + *   If display_submitted is set, additional variables are prepared:
   
   @@ -42,17 +45,15 @@
   - * - author_attributes: Same as attributes, except applied to the author of
   - *   the node tag that appears in the template.
   ...
   - * - teaser: Flag for the teaser state. Will be true if view_mode is 'teaser'.
   

   Because of the duplication we have for templates in core from classy this also needs to happen in classy's node template.

4. +++ b/core/modules/node/templates/node.html.twig
   @@ -15,13 +15,16 @@
   + *    - author_picture: The node author user entity, rendered using the "compact"
   ...
   + *    -  date: Themed creation date field.
   + *    -  author_name: Themed author name field.
   

   There is some coding standards for the indent and 80 chars from what dreditor is saying.

5. +++ b/core/modules/node/templates/node.html.twig
   @@ -42,17 +45,15 @@
   + * - is_front: Flag for front. Will be true when presented on the front page.
   
   +++ b/core/themes/bartik/templates/node.html.twig
   @@ -42,17 +45,15 @@
   + * - is_front: Flag for front. Will be true when presented on the front page.
   

   This looks like it snuck in and may not exist? Or is this a clean-up of something that is missing?

6. +++ b/core/modules/page_cache/src/Tests/PageCacheTagsIntegrationTest.php
   @@ -97,7 +98,6 @@ function testPageCacheTags() {
   -      'user:' . $author_1->id(),
   

   Does the user: need to be removed?

7. +++ b/core/themes/bartik/templates/node.html.twig
   @@ -1,7 +1,7 @@
   - * Bartik's theme implementation to display a node.
   + * Default theme implementation to display a node.
   

   That change is not needed as it is Bartik's.

Ok I think the drupal_render() is needed... but it really shouldn't be, RDF is concatinating it and using SafeMarkup... would be nicer if they just did prefix/suffix on the render element or something. That should be a follow-up.

I've put the other bits in place and fixed the failing test but turning on display_submitted (NodeTestBase has it off be default).

Let's see if testbot agrees.

Hmm 2 different errors, text showing that shouldn't be...

Looking into fails and working on it.

+++ b/core/modules/node/node.module
@@ -610,6 +601,14 @@ function template_preprocess_node(&$variables) {
+  // Helpful $content variable for templates.
+  $variables += array('content' => array());
+  foreach (Element::children($variables['elements']) as $key) {
+    $variables['content'][$key] = $variables['elements'][$key];
+  }

Moved this hunk down as the test was failing because author_name and date variable should not be included in $variables['content'] . we render them before building the 'content' array so they don't show up there.
In #18 patch $variable['date'] and $variable['author_name'] was set after building the content variable.

+++ b/core/modules/node/node.module
@@ -591,18 +586,14 @@ function template_preprocess_node(&$variables) {
+    $variables['date'] = \Drupal::service('renderer')->render($variables['elements']['created']);

Replaced deprecated drupal_render with \Drupal::service('renderer')->render.

I am going to try do some profiling tomorrow.

Here is profiling result -

=== 8.0.x..8.0.x compared (556d0efcc18be..556d13edd0b22):

ct : 610,204|610,204|0|0.0%
wt : 2,045,741|2,013,482|-32,259|-1.6%
mu : 58,569,312|58,569,344|32|0.0%
pmu : 59,801,216|59,801,624|408|0.0%

=== 8.0.x..2370145-a compared (556d0efcc18be..556d153c4efc5):

ct : 610,204|586,877|-23,327|-3.8%
wt : 2,045,741|1,773,106|-272,635|-13.3%
mu : 58,569,312|58,496,288|-73,024|-0.1%
pmu : 59,801,216|59,727,592|-73,624|-0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=556d0efcc18be&...

@kgoel you should include the lionsad.de links those are very important for being able to verify your results :)

Nice you fixed my other bug.

+++ b/core/modules/node/node.module
@@ -572,14 +572,9 @@ function node_theme_suggestions_node(array $variables) {
-  $variables['date'] = drupal_render($variables['elements']['created']);
-  unset($variables['elements']['created']);
-  $variables['author_name'] = drupal_render($variables['elements']['uid']);
-  unset($variables['elements']['uid']);

@@ -591,18 +586,14 @@ function template_preprocess_node(&$variables) {
   if ($variables['display_submitted']) {
+    $variables['author_attributes'] = new Attribute();
+    $variables['date'] = \Drupal::service('renderer')->render($variables['elements']['created']);
+    $variables['author_name'] = \Drupal::service('renderer')->render($variables['elements']['uid']);

The cache tags are no longer needed for user because by default the tests have display_submitted false and the now won't get rendered and there for don't get tags for user context bubbling up.

Just need to remove the unneeded user cache tags from the test now.

Looks you are on the right track @kgoel. The next one looks like it needs timezone cache key removed.

Maybe this will finish them off? Didn't work locally (10 fails) running

php ./core/scripts/run-tests.sh  --url http://d8.dev --color --verbose --class "Drupal\page_cache\Tests\PageCacheTagsIntegrationTest"

engage!

Well those 10 fails are there! Well I'll be. That's an unexpected but nice to see that local is showing the same.

Assuming it needs a reroll at this point, but lets check.

All cache failures it looks like.

Needed rerolling. Lets see if that worked.

Is this still relevant now that #2923701: Mechanism to disable preprocessing of node base fields so they can be configured via the field UI is in an we don't always process those fields if they're configured via manage-display?

There are still some micro-optimizations like removing the teaser boolean flag. Which is a BC break and I'm not sure that's worth removing, would need a BC layer and stuff. I'd say we can close it..

Thanks, closing as duplicate of #2923701: Mechanism to disable preprocessing of node base fields so they can be configured via the field UI