Problem/Motivation

This is a bug that got introduced somewhere along the way. #2151459: Enable node render caching was the patch that originally introduced this:

+      if ($view_mode == 'teaser' && \Drupal::moduleHandler()->moduleExists('history') && \Drupal::currentUser()->isAuthenticated()) {
+        $node_links['comment__' . $field_name]['#attached']['library'][] = array('comment', 'drupal.node-new-comments-link');
+
+        // Embed the metadata for the "X new comments" link (if any) on this node.
+        $node_links['comment__' . $field_name]['#post_render_cache']['history_attach_timestamp'] = array(
+          array('node_id' => $node->id()),
+        );
+        $node_links['comment__' . $field_name]['#post_render_cache']['Drupal\comment\CommentViewBuilder::attachNewCommentsLinkMetadata'] = array(
+          array('entity_type' => $node->getEntityTypeId(), 'entity_id' => $node->id(), 'field_name' => $field_name),
+        );
+      }

… but no test coverage was included. These #post_render_cache callbacks are currently not being invoked at all. Fortunately, the only consequence of that is worse front-end performance, because we need to make requests to get that data now.

Proposed resolution

Fix it.

Remaining tasks

Write test coverage.

User interface changes

None.

API changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Title: drupalSettings being attached for comment.module's node links are lost » Comment module's node links' front-end perf-optimizing drupalSettings are missing
Status: Active » Needs review
FileSize
3.48 KB
Wim Leers’s picture

larowlan’s picture

Anyway we can add a test for this?

Wim Leers’s picture

Yes: assert that the expected drupalSettings are present.

effulgentsia’s picture

Status: Needs review » Closed (duplicate)

This fix is few lines of code, but some of them would need to be undone by #2478483: Introduce placeholders (#lazy_builder) to replace #post_render_cache, so I think it makes sense to incorporate the entire fix there directly.

Wim Leers’s picture

Issue tags: -Needs tests
FileSize
1.77 KB
5.2 KB

And test coverage.

andypost’s picture

Status: Closed (duplicate) » Needs review

Let's get a test run on new patch before closing as duplicate, otoh #lazy_builder issue is big enough so better separate here

Status: Needs review » Needs work

The last submitted patch, 6: comment_links_drupalsettings-2496399-6.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Closed (duplicate)
Wim Leers’s picture

Many of the changes here would need to be undone again. That's why.