Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#6 | comment_links_drupalsettings-2496399-6.patch | 5.2 KB | Wim Leers |
Comments
Comment #1
Wim LeersComment #2
Wim Leers#2495779: Make #theme => links take cacheability metadata as an argument fixes part of this.
Comment #3
larowlanAnyway we can add a test for this?
Comment #4
Wim LeersYes: assert that the expected drupalSettings are present.
Comment #5
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis 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.
Comment #6
Wim LeersAnd test coverage.
Comment #7
andypostLet's get a test run on new patch before closing as duplicate, otoh #lazy_builder issue is big enough so better separate here
Comment #9
Wim LeersComment #10
Wim LeersMany of the changes here would need to be undone again. That's why.