Remove comment_prepare_thread()

It looks a right way, otoh I think about #1920044: Move comment field settings that relate to rendering to formatter options and a way to skip this for non-threaded comments that's are used for forums now #2301247: Make comments for forum non-threaded by default

It's really interesting how that could work at all!
CommentViewBuilder does not know anything about comment field that holds setting about threading

+++ b/core/modules/comment/comment.module
@@ -661,7 +627,6 @@ function comment_node_update_index(EntityInterface $node, $langcode) {
-          comment_prepare_thread($comments);

+++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
@@ -156,7 +156,6 @@ public function viewElements(FieldItemListInterface $items) {
-          comment_prepare_thread($comments);

nice, but this is a places that "knows" about thread and have to pass this to "builder"

+++ b/core/modules/comment/src/CommentViewBuilder.php
@@ -88,6 +89,8 @@ public function buildComponents(array &$build, array $entities, array $displays,
+    self::buildDepth($build, $entities);

@@ -281,6 +284,48 @@ protected static function buildLinks(CommentInterface $entity, EntityInterface $
+  protected static function buildDepth(array &$build, array $comments) {

@@ -303,8 +349,10 @@ protected function alterBuild(array &$build, EntityInterface $comment, EntityVie
+        $build['#suffix'] = str_repeat('</div>', (int) $build['#comment_divs_final']);

this could be called for comments from different fields/entities so needs to make sure that thread is consistent

Rerolled

1. +++ b/core/modules/comment/src/CommentViewBuilder.php
   @@ -106,6 +107,8 @@ public function buildComponents(array &$build, array $entities, array $displays,
   +    ¶
   

   Tabs that don't belong :)

2. +++ b/core/modules/comment/src/CommentViewBuilder.php
   @@ -301,6 +304,46 @@ protected static function buildLinks(CommentInterface $entity, EntityInterface $
   +
   

   All of this is indented incorrectly, I'm afraid, with 4-space instead of 2-space indentation.

3. +++ b/core/modules/comment/src/CommentViewBuilder.php
   @@ -324,8 +368,10 @@ protected function alterBuild(array &$build, EntityInterface $comment, EntityVie
   +      if ($is_threaded
   +	 && !empty($build['#comment_divs_final'])
   +	 && is_numeric($build['#comment_divs_final'])) {
   +	$build['#suffix'] = str_repeat('</div>', (int) $build['#comment_divs_final']);
   

   More tabs, and code style violation.

Fixed identation issues.

Could you please provide an interdiff? See https://www.drupal.org/documentation/git/interdiff.

Interdiff attached.

I don't get the reason to make check isset(divs) && is_numeric(divs) they are always a numbers so maybe !empty() is enough

1. +++ b/core/modules/comment/src/CommentViewBuilder.php
   @@ -44,10 +45,7 @@ class CommentViewBuilder extends EntityViewBuilder {
      public static function createInstance(ContainerInterface $container, EntityTypeInterface $entity_type) {
        return new static(
   -      $entity_type,
   -      $container->get('entity.manager'),
   -      $container->get('language_manager'),
   -      $container->get('csrf_token')
   +        $entity_type, $container->get('entity.manager'), $container->get('language_manager'), $container->get('csrf_token')
   

   unneeded change, please to not change this

2. +++ b/core/modules/comment/src/CommentViewBuilder.php
   @@ -167,7 +166,7 @@ public function buildComponents(array &$build, array $entities, array $displays,
   -      if ($this->moduleHandler->moduleExists('history') &&  \Drupal::currentUser()->isAuthenticated()) {
   +      if ($this->moduleHandler->moduleExists('history') && \Drupal::currentUser()->isAuthenticated()) {
   

   current_user should be injected

3. +++ b/core/modules/comment/src/CommentViewBuilder.php
   @@ -302,6 +301,47 @@ protected static function buildLinks(CommentInterface $entity, EntityInterface $
   +   * Calculates the indentation level of each comment in a comment thread.
   +   */
   +  protected static function buildDepth(array &$build, array $comments) {
   

   needs doc-block fix about arguments

4. +++ b/core/modules/comment/src/CommentViewBuilder.php
   @@ -349,18 +388,18 @@ protected function alterBuild(array &$build, EntityInterface $comment, EntityVie
   -      ->getCountNewComments(entity_load($context['entity_type'], $context['entity_id']));
   +        ->getCountNewComments(entity_load($context['entity_type'], $context['entity_id']));
   ...
   -      ->load($context['entity_id']);
   +        ->getStorage($context['entity_type'])
   ...
   -      ->getNewCommentPageNumber($entity->{$field_name}->comment_count, $new, $entity);
   +        ->getStorage('comment')
   

   indent wrong

5. +++ b/core/modules/comment/src/CommentViewBuilder.php
   @@ -368,7 +407,7 @@ public static function attachNewCommentsLinkMetadata(array $element, array $cont
   -            'new_comment_count' => (int)$new,
   +            'new_comment_count' => (int) $new,
   

   unneeded change

Working on it now.

The patch is no longer applicable. Will reroll first.

Just rerolling for now.

Not sure if I did it right. I simply followed the procedure mentioned at documentation for patch reroll.

I think the function is not needed in builder.

Here's a new patch:
1) removes usage and function
2) stores "threading" in #comment_threaded key of the entity build array - DX++ for contrib to not query field settings
3) removes "commented entity" cache hitting because in 99% there's only one commented entity otherwise threading makes no sense (or we should implement threading as separate render function)

Removed commented line.

Forget that interdiff. This is the good one!

+++ b/core/modules/comment/src/CommentViewBuilder.php
@@ -65,7 +65,6 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
-    /** @var \Drupal\comment\CommentInterface $entity */

This is a type-hint not a commented line, this allows to easily use interface defined methods

+1 to #comment_threaded approach, patch at #22 looks good to go to me

This is so much better! This no longer sets random properties on Entity objects just to pass information around; this now sets render array properties, as it should. Great work, thanks!

Only nitpicks:

1. +++ b/core/modules/comment/src/CommentViewBuilder.php
   @@ -18,14 +24,54 @@
   +   * Constructs a new FeedViewBuilder.
   

   s/Feed/Comment/

2. +++ b/core/modules/comment/src/CommentViewBuilder.php
   @@ -58,25 +104,31 @@ public function buildComponents(array &$build, array $entities, array $displays,
   +    // A counter that helps track how indented we are.
   

   s/that helps/to/
   s/how indented we are/the indentation level/

3. +++ b/core/modules/comment/src/CommentViewBuilder.php
   @@ -58,25 +104,31 @@ public function buildComponents(array &$build, array $entities, array $displays,
   +          // or (negative) amount of divs to close to get to this comment's
   +          // indent level.
   

   The first line of this comment makes is clear, these 2 lines aren't.

fixed, 3 - is tricky to explain

I trust the test coverage in \Drupal\comment\Tests\CommentThreadingTest, we significantly improved that test in #2254181: Comment indentation is incorrect for comments following a replying-comment: don't render cache comments for which threading is enabled.

comment_prepare_thread() is D7 function. Also has anyone done the perf testing to prove that the beta evaluation is correct?

For instructions on how to profile if someone wants to give it a try, see the instructions here for the setup: https://www.drupal.org/contributor-tasks/profiling

Re-roll, comment generation is broken so use the patch #2422101-7: CommentItem should override the generateSampleValue method and provide sample values to generate comments.

Looks good to me! :)

It needs CR and performance testing
My measurements shows less IO but more cpu... somehow

+1 to the change, this depth thingy made render caching difficult as I could not rely on the comment objects being the same as when loaded from the DB.

But yes needs profiling ...

Draft CR posted https://www.drupal.org/node/2466759

With render cache off, the performance is exactly the same, which is expected.

Due to the refactoring the function call counts remains funnily enough exactly the same and we have -0.0% wall time (0.0028 ms) - way within margin of error.

With render cache on:

Sum of 100 runs:

=== SUM: 8_0_x-summary..issue-2318579--comment-prepare=thread-summary compared (552492716a776..55249276b36e7):

ct  : 14,013,012|14,013,012|0|0.0%
wt  : 25,450,536|25,505,755|55,219|0.2%
mu  : 2,848,958,816|2,849,139,976|181,160|0.0%
pmu : 3,137,195,320|3,137,456,032|260,712|0.0%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

### XHPROF-LIB REPORT

+---------------------------------------+------------+------------+------------+------------+------------+
| namespace                             |        min |        max |       mean |     median |       95th |
+---------------------------------------+------------+------------+------------+------------+------------+
| Calls                                 |            |            |            |            |            |
|                                       |            |            |            |            |            |
| issue-2318579--comment-prepare=thread |    140,100 |    143,112 |    140,130 |    140,100 |    140,100 |
| 8_0_x                                 |    140,100 |    143,112 |    140,130 |    140,100 |    140,100 |
|                                       |            |            |            |            |            |
| Wall time                             |            |            |            |            |            |
|                                       |            |            |            |            |            |
| issue-2318579--comment-prepare=thread |    244,442 |    311,507 |    255,058 |    254,599 |    261,721 |
| 8_0_x                                 |    244,122 |    317,064 |    254,505 |    254,394 |    259,514 |
|                                       |            |            |            |            |            |
| Memory usage                          |            |            |            |            |            |
|                                       |            |            |            |            |            |
| issue-2318579--comment-prepare=thread | 28,483,320 | 28,904,792 | 28,491,400 | 28,483,320 | 28,488,272 |
| 8_0_x                                 | 28,481,640 | 28,902,544 | 28,489,588 | 28,481,864 | 28,482,049 |
|                                       |            |            |            |            |            |
| Peak memory usage                     |            |            |            |            |            |
|                                       |            |            |            |            |            |
| issue-2318579--comment-prepare=thread | 31,366,472 | 31,789,704 | 31,374,560 | 31,366,472 | 31,372,910 |
| 8_0_x                                 | 31,364,152 | 31,786,504 | 31,371,953 | 31,364,152 | 31,364,580 |
|                                       |            |            |            |            |            |
+---------------------------------------+------------+------------+------------+------------+------------+

Again the function calls remain the same, which means this is done before the #pre_render is called, which is fine as such that data is always correct.

--

Patch looks good too and removes setting arbitrary properties on objects.

=> RTBC

So #39 shows that we're not getting a performance improvement. We need to update the beta evaluation and provide another reason why we should make this change.

Is DX as indicated by @Fabianx at #39 a suitable reason?

Back to RTBC.

The justification is that we should avoid dirtying entities, else they cannot be re-loaded lazily, which needlessly limits the usage of placeholders (as seen in #2469431-5: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts).

While that issue is just in prototype stage, even now dirtying entities will make the job of contrib modules like my render_cache much harder and as the patch shows without any reason.

Okay @Fabianx - I agree that we should be avoiding dirty entities especially in the render pipeline. I think the disruption of removing comment_prepare_thread() is worth the improvement and we've shown that there is no effect on performance. Committed 418da36 and pushed to 8.0.x. Thanks!