Details: see #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user, particularly the issue summary and comments 40 and 87.

This issue was split off from that issue because the render cache breakage of the Tracker module does not break entity render caching, which is what we were trying to unblock there.

CommentFileSizeAuthor
#19 interdiff.txt1.72 KBWim Leers
#19 tracker_history_markers-2082315-19.patch17.74 KBWim Leers
#17 rebase-interdiff.txt32.71 KBWim Leers
#17 tracker_history_markers-2082315-17.patch17.56 KBWim Leers
#14 tracker_history_markers-2082315-14.patch11.59 KBmrjmd
#10 tracker_history_markers-2082315-10.patch16.26 KBrealityloop
#3 tracker_history_markers-2082315-3.patch13.19 KBWim Leers
#1 tracker_history_markers-2082315-1.patch16.56 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Title: Node history markers on Tracker ("new" and "updated" markers, "x new replies" links) forces render caching to be per user » Tracker history markers ("new" and "updated" markers, "x new replies" links) forces render caching to be per user
Assigned: Unassigned » Wim Leers
Status: Active » Postponed
Issue tags: -sprint
FileSize
tracker_history_markers-2082315-1.patch16.56 KB

This patch was extracted from #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user and depends on code being introduced there (to prevent code duplication, there is a new drupal.history library that contains the common code to do history.module-related things).

Because this patch depends on that one, marking as postponed because it would otherwise surely fail to work.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Postponed » Active
Issue tags: +sprint

#1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user was committed, this can now be worked on again.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Active » Needs review
FileSize
tracker_history_markers-2082315-3.patch13.19 KB

Straight reroll.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

I think this is missing the JS file or was that already committed in the parent patch?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Needs work

No, you're right, the JS file is missing. It was in #1, but I failed to include it in #3. Will reroll.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue tags: -sprint

Will be on my plate in 1 week or so hopefully.

realityloop’s picture

realityloop
he/him
CreditAttribution: realityloop commented
Issue summary: View changes

@Wim Leers
I just tried to reroll this myself without any luck (I did include the js from #1), please note the changes to the Tracker module in https://www.drupal.org/node/140363 when you get to rerolling it, feel free to ping me if you need somnoen to review.

Status: Needs work » Needs review

realityloop queued 3: tracker_history_markers-2082315-3.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 3: tracker_history_markers-2082315-3.patch, failed testing.

realityloop’s picture

realityloop
he/him
CreditAttribution: realityloop commented
FileSize
tracker_history_markers-2082315-10.patch16.26 KB

here is my attempt in case it's useful

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 10: tracker_history_markers-2082315-10.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue tags: +Needs reroll, +Novice, +php-novice
mrjmd’s picture

mrjmd CreditAttribution: mrjmd commented
Status: Needs work » Needs review
Issue tags: -Needs reroll, -Novice, -php-novice
FileSize
tracker_history_markers-2082315-14.patch11.59 KB

Here's a reroll. It had some conflicts that I hopefully fixed properly. I also removed the use of deprecated calls to node_load and drupalGetSettings, and removed the @return comment for assertHistoryMetadata() since there is no return.

Status: Needs review » Needs work

The last submitted patch, 14: tracker_history_markers-2082315-14.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +blocker
Parent issue: » #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!)
Related issues: +#1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user, +#2484611: Tracker responses don't set cache tags & contexts

We need to get this going again

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review
FileSize
tracker_history_markers-2082315-17.patch17.56 KB
rebase-interdiff.txt32.71 KB

Started from #1 again. Notable changes:

  • asset libraries are now defined in a YML file, not in a hook (#1 was that old :P)
  • indentation changes in the JS, to pass eslint
  • some spacing changes in the JS, to pass eslint
  • comment statistics are now on the comment field, not on the entity
  • the implementation of ajaxPageState has been changed, so the test assertions had to be updated also
  • resolved a whole bunch of conflicts, but still only a handful, which is impressive, given that #1 was almost two years old!

(Rebase interdiff included, but it'll be mostly useless.)

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented

I have a couple of really small nitpicks but otherwise this seems to work and it has sufficient test coverage as far as I can see.

  1. +++ b/core/modules/tracker/src/Tests/TrackerTest.php
@@ -168,97 +174,52 @@ function testTrackerUser() {
+    // Verify.
     $this->drupalGet('activity');

    This comment is not useful. Can this be "Verify that the correct history metadata exists" or something along those lines? 

  2. +++ b/core/modules/tracker/src/Tests/TrackerTest.php
@@ -168,97 +174,52 @@ function testTrackerUser() {
+    // Add a comment to the page, make sure it is created after the node.
...
+    sleep(1);

    Can it be documented why the sleep is in here? does it have to be? 

  3. +++ b/core/modules/tracker/src/Tests/TrackerTest.php
@@ -168,97 +174,52 @@ function testTrackerUser() {
+    // Verify.
     $this->drupalGet('activity');

    This comment isn't descriptive either. "Verify that the tracker's dates are updated" or something among those lines would be better I think.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
tracker_history_markers-2082315-19.patch17.74 KB
interdiff.txt1.72 KB

Thanks for the review! All addressed.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented
Status: Needs review » Reviewed & tested by the community

RTBC - already pre-existing solution, so good to go.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented 
+++ b/core/modules/tracker/js/tracker-history.js
@@ -0,0 +1,122 @@
+/**
+ * Attaches behaviors for the Tracker module's History module integration.
+ *
+ * May only be loaded for authenticated users, with the History module enabled.
+ */
+(function ($, Drupal, window) {
+
+  "use strict";
+
+  /**

I'd expect that the markers in #2082317: Forum history markers ("new" and "updated" markers, "x new posts" links) forces render caching to be per user will end up looking very similar. Is it worth putting this into comment module instead so it can be re-used?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

#21: they will indeed be very similar; but that's just because this (Tracker) and the existing code in Comment module (see the drupal.comment-by-viewer, drupal.comment-new-indicator and drupal.node-new-comments-link libraries) are already using the same underlying API: History module's api asset library.

Tracker module's implementation is different and needs to be different from Comment & Forum's, because it presents data very differently.

If anything, the Forum module's support for this (i.e. in #2082317: Forum history markers ("new" and "updated" markers, "x new posts" links) forces render caching to be per user) would just need the Comment module's existing asset libraries to be refactored so they apply both to comments and forum topics/replies. The Tracker module's needs differ the most from those of Comment/Forum.

  • catch committed 8c8bf75 on 8.0.x 
    Issue #2082315 by Wim Leers, realityloop, mrjmd: Tracker history markers...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

OK that's fair enough. Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.