Comment parent template variables are built twice

We are not only duplicating the parent template variables, we are loading the parents twice also. If the parent is rendered on the same page, it is still loaded again when rendering its children as comments have static entity load caches turned off.

This can be fixed after feature freeze I assume.

The loading problem is being fixed in #1844146: comment_load() call in template_preprocess_comment() re-loads comment parent entities because of disabled static cache.

This is different. What @catch means is that for example if you have a parent comment with 100 sub-comments, then we are calling "format_date($comment_parent->created)" over and over again for the parent comment for every comment below it.

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

How is this a major performance problem? (vs. a normal one).

Updated issue summary, needs profiling.

Updated issue summary.

Updated issue summary.

I was taking a look at this and was thinking we could solve it with a static variable containing all the parent entities loaded in the page request, and checking for it in the static before loading it - does that sound a sensible way forward or do we need to be looking at something higher up the call stack?

Updated issue summary.

1) Comment::hasParentComment() should not load parent entity.
2) Suppose removing "parent" is not possible without breaking following "accessibility":

    {#
      Indicate the semantic relationship between parent and child comments
      for accessibility. The list is difficult to navigate in a screen
      reader without this information.
    #}
    {% if parent %}
      <p class="parent visually-hidden">{{ parent }}</p>
    {% endif %}

If we get rid of this line then parent entity could not be loaded,
but the same time when thread of comments is rendering parent comment already loaded so fine.

At least better to minimize a defaults for "parent"

Is there another way to do this? What if the link is just to the parent, without actually defining the parent title?

It would be important to have some semantic relationship, but maybe we could do a simpler one so that comment template variables aren't built twice...

doesn't apply.

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -316,7 +316,8 @@ public static function bundleFieldDefinitions(EntityTypeInterface $entity_type,
   public function hasParentComment() {
-    $parent = $this->get('pid')->entity;
+    // Do not load parent comment entity.
+    $parent = $this->get('pid')->target_id;
     return !empty($parent);

At least this part should be commited

hasParentComment() looks fairly straight forward, but then so do the changes to comment.html.twig.

Would it help if I rolled a patch just for everything but the changes to comment.module?

Sure!
I just think that better to limit variables related to parent comment.
If contrib will need more vars it's easy to add them and anyway will need to change template

That means:
1) there's comment_parent: Full parent comment entity (if any). variable already
2) a12y needs "parent" (last patch forgets to translate this
3) templates can access to parent comment properties via comment_parent.subject so no reason to prepare so much useless variables.

I don't think it needs more variables, but I'm pretty certain that the inline docs in comment.html.twig were wrong.

No new variables have been added in this patch. Thanks for pointing out the missing internationalization.

I didn't touch on anything by comment_parent.subject, but right now it seems to be working.

Mike, great! now we should remove the same string from preprocess.
Also s/entity/commented_entity - fix wrong name in template

I don't know why this seems to be happening, but I'm not seeing <p class="comment-parent visually-hidden">...</p> in #19 but do in #18.

I'm not sure why this would be but I spun up to instances to compare and they both had the same issue. Why would parent_comment be true in one and not the other?

EDIT: Example (for the next 3 hrs) - http://sa5dd4e9d2ca61c1.s2.simplytest.me/node/1

@mgifford You was right! I missed to change bartik's template

Great fixes here!

+++ b/core/modules/comment/templates/comment.html.twig
@@ -90,9 +85,12 @@
+      <p class="parent visually-hidden">{{ "In reply to !parent_title by !parent_author"|t({ '!parent_title': parent_title, '!parent_author': parent_author }) }}</p>

+++ b/core/themes/bartik/templates/comment.html.twig
@@ -79,13 +74,16 @@
+          {{ "In reply to !parent_title by !parent_author"|t({ '!parent_title': parent_title, '!parent_author': parent_author }) }}

We afaik use single quotes also for strings in templates

sure, now that needs profiling, suppose the main difference would be caused by

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -324,7 +324,8 @@ public static function bundleFieldDefinitions(EntityTypeInterface $entity_type,
   public function hasParentComment() {
-    $parent = $this->get('pid')->entity;
+    // Do not load parent comment entity.
+    $parent = $this->get('pid')->target_id;
     return !empty($parent);

that code

I actually just opened #2328629: Comment::hasParentComment() tries to load comment with ID 0 today, with a similar change for that method.

properly merged head and incorporate #24

Profiling of node with 50 threaded comments

re-roll

Looks like duplicated issue #2002094: Improve performance of comment.html.twig

Seems to work fine in my testing on SimplyTest.me

I created environment, added a node, then proceeded to add comments and child comments.

The code comments look like good additions too. Think this is ready for Core.

I'm not sure that the solution fully addresses @catch's original concerns about calling format_date and drupal_render($username) multiple times on the same parent comment.

the best we can do is to remove 'parent_author' from prepare, but this causes an a11y regression

duplicated comment

Could we just add a static variable so that format_date and drupal_render($username) are stored rather than re-generated for comments?

@catch they are all used, see #16-17

+++ b/core/modules/comment/templates/comment.html.twig
@@ -43,21 +43,16 @@
- * - comment_parent: Full parent comment entity (if any).
+ * - parent_comment: Full parent comment entity (if any).

Has separate issue #2010196: Rename comment parent theme variable

Rerolled patch with correction of comment indentation.

I removed a major chunk of code from the patch, and found the comments to be working fine for me. There is proper indentation for child comments and the parent comment seems to be fine. No need for this code in the comment.module file.
Please reply, if I'm on the wrong track

Suppose it ready

On my phone but I am pretty sure this misses updating Classy's comment template.

@Cottser please take a look at related #2031883: Markup for: comment.html.twig

+++ b/core/themes/classy/templates/content/comment.html.twig
@@ -98,12 +93,14 @@
+      <p class="parent visually-hidden">{{ 'In reply to !parent_title by
+		!parent_author'|t({ '!parent_title': parent_title, '!parent_author':
+        parent_author }) }}</p>

indent problem (seems tab)

I'm going to organize all this render/performance issues in one meta.
Comments are improved in #2031883-34: Markup for: comment.html.twig
Also bigger performance gain in #2318579-22: Remove comment_prepare_thread()

RTBC, because this is a most independent issue

Reroll attached, just 1 fuzz and 2 line shift, no real changes.

great!

So the patch and the issue summary and title seem completely at odds - what am I missing?

Adding some tags

Attached patch against Drupal 9.5.x
Attached reroll patch

Last patch had some failures.

Also issue summary update was requested and still needs to happen it appears.