Updated: Comment #68

Problem/Motivation

The comment new mark is based on the user's last viewed timestamp, which means it's forced to be per-user. This makes comments only cacheable per-user (so do comment links but that's a separate issue).

Proposed resolution

  1. Set drupalSettings.user.uid to the current user's ID, so that we can set the by-viewer class on comments on the client-side.
  2. Attach JavaScript that adds the by-viewer class (no extra HTTP requests needed).

and

  1. Embed metadata that is the same for all users in the HTML as data- attributes: the node's last comment timestamp, the node's ID, the comment user ID, each comment timestamp.
  2. Attach JavaScript that adds the "new" and "updated" markers, which has to perform a HTTP request to know the latest time the current user read a node (and hence also its comments). Use client-side (localStorage) caching to not perform such an HTTP request in most scenarios.
  3. (See #40 for a very detailed explanation of when a HTTP request happens, how HTTP requests are avoided in most scenarios.)

Remaining tasks

None.

User interface changes

None.

API changes

API addition: drupalSettings.user.uid (if this qualifies as an API).

#1605290: Enable entity render caching with cache tag support

Original report by @catch

The comment new mark is based on the user's last viewed timestamp, which means it's forced to be per-user. This makes comments only cacheable per-user (so do comment links but that's a separate issue).

Proposed resolution

Put the node history timestamp into drupalsettings, put the comment timestamp into the markup, then compare the two in js and add the mark if necessary.

Files: 
CommentFileSizeAuthor
#108 node_history_markers-1991684-108.patch57.16 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 59,114 pass(es).
[ View ]
#102 node_history_markers-1991684-102.patch57.51 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,931 pass(es).
[ View ]
#102 interdiff.txt2.66 KBWim Leers
#96 node_history_markers-1991684-96.patch54.91 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 58,870 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#96 interdiff.txt1015 bytesWim Leers
#94 node_history_markers-1991684-94.patch54.91 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 57,870 pass(es), 583 fail(s), and 307 exception(s).
[ View ]
#94 interdiff.txt1.78 KBWim Leers
#92 node_history_markers-1991684-92.patch54.84 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 57,978 pass(es), 602 fail(s), and 854 exception(s).
[ View ]
#90 node_history_markers-1991684-90.patch54.87 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node_history_markers-1991684-90.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#90 interdiff.txt17.66 KBWim Leers
#87 node_history_markers-1991684-87.patch69.99 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 57,251 pass(es), 580 fail(s), and 307 exception(s).
[ View ]
#87 interdiff.txt3.47 KBWim Leers
#82 node_history_markers-1991684-82.patch69.85 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,533 pass(es).
[ View ]
#82 interdiff.txt1008 bytesWim Leers
#80 node_history_markers-1991684-80.patch68.99 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 58,504 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#80 interdiff.txt952 bytesWim Leers
#76 node_history_markers-1991684-76.patch69.09 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,443 pass(es).
[ View ]
#73 node_history_markers-1991684-73.patch69.39 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node_history_markers-1991684-73.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#73 interdiff.txt10.97 KBWim Leers
#64 node_history_markers-1991684-64.patch67.67 KBmsonnabaum
PASSED: [[SimpleTest]]: [MySQL] 58,289 pass(es).
[ View ]
#64 interdiff.txt1.48 KBmsonnabaum
#39 node_history_markers-1991684-39.patch69.78 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,273 pass(es).
[ View ]
#37 node_history_markers-1991684-37.patch69.8 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 57,886 pass(es), 63 fail(s), and 797 exception(s).
[ View ]
#36 node_history_markers-1991684-36.patch69.87 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,153 pass(es).
[ View ]
#36 interdiff.txt10.92 KBWim Leers
#34 node_history_markers-1991684-34.patch63.97 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 58,245 pass(es), 19 fail(s), and 0 exception(s).
[ View ]
#26 node_history_markers-1991684-26.patch62.22 KBamateescu
FAILED: [[SimpleTest]]: [MySQL] 57,484 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#22 node_history_markers-1991684-22.patch64.56 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node_history_markers-1991684-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#22 interdiff-minorfixes.txt2.33 KBWim Leers
#22 interdiff-tests.txt22.6 KBWim Leers
#14 node_history_markers-1991684-14.patch45.07 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 57,105 pass(es), 38 fail(s), and 1 exception(s).
[ View ]
#11 node_history_markers-1991684-11.patch19.94 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node_history_markers-1991684-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#4 added-data-date-attribute-1991684-3.patch635 bytesrcaracaus
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch added-data-date-attribute-1991684-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Issue summary:View changes

Updated issue summary.

Priority:Normal» Major

I changed your suggestion into a Proposed Resolution in the issue summary since I think it is a great idea.

Issue tags:+JavaScript

So @rcaracaus and I had a look into this today.
We figured the first step is getting the timestamp of the comment into a data attribute (rob will post the patch for that).
However the $comment->new property as used in comment_prepare_thread to evaluate whether a comment is indeed new is actually an entity-ng magic property.
see core/modules/comment/lib/Drupal/comment/FieldNewValue.php
So this field is lazy loaded and only evaluated when $comment->new->value is called in comment_prepare_thread().
In turn this calls node_mark() which requires the global $user.
So I'm assuming you're talking about caching the 'additions' chunk of the render array from comment_node_page_additions without the new mark, and then add the timestamp from {history} for the given uid and nid when rendering the node?
So I assume this would entail a cid for the thread (specific to the nid and pager page), and methods to clean it when a comment is added to that node.
Ie we are dealing with caching the comment thread, not the whole page. The node still needs to be cached per user so we can attach their history timestamp?
Can you confirm this sounds about right before we proceed? Rob will handle the js/css and I'll tackle the caching etc.
Also not sure how we can test this (the front end chunk - the cache can be tested), as existing tests rely on the presence of the mark.

Lee

StatusFileSize
new635 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch added-data-date-attribute-1991684-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I was handed this task as a possible easy win, but it looks pretty complex. I added a data-date attribute to template_preprocess_comment though. It could also be called data-created I guess, not sure what the standards are for these attributes. Also, I tried to simply set the array value, but it was giving me an error so I had to set array = array() ..

"Notice: Indirect modification of overloaded element of Drupal\Core\Template\Attribute has no effect in template_preprocess_comment"

Sure that is a standard error, but thought I would post anyway.

Caching is added by #1605290: Enable entity render caching with cache tag support so no need to add that here.

In terms of getting the current user's last visited timestamp, that would indeed mean we're back to per-user caching. History module could add that to Drupal.settings in hook_page_build() maybe?

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

Status:Active» Needs review

Status:Needs review» Needs work

The last submitted patch, added-data-date-attribute-1991684-3.patch, failed testing.

Assigned:Unassigned» Wim Leers

Working on this! I've got it working for the comment "new" indicator. Currently working on the "x new comments" links.

Not comment module, but iirc there's new and updated marks for nodes as well which could use the same treatment.

Title:Comment new mark forces render caching to be per userNode history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user
Component:comment.module» history.module
Status:Needs work» Needs review
StatusFileSize
new19.94 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node_history_markers-1991684-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

#10: yep, that's also something we need to work on. Expanding scope of issue.


In this initial patch only covers the first use case: comment "new" indicators. Consequently, it also gets rid of $variables['new'], so that won't be available in the Twig template anymore. Similarly, the by-visitor class was problematic, because that too is user-specific. So, that won't be set anymore either, and to be able to do that, I've pulled the "user id" part out of the patch at #2005644: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for in-place editing metadata, introduce drupalSettings.user.permissionsHash. The overall way this is handled is heavily based on how #914382: Contextual links incompatible with render cache was solved.

As a consequence of the above, I was able to cut out a bunch of code:

  • calculating of the first new comment on the server side
  • CommentNewItem
  • CommentNewValue

One of the trickiest aspects was supporting comment links like /node/1#new, because <a id="new" /> cannot be set anymore either, because that too is user-specific, hence the JS must calculate it. A nice side-effect is that the solution to this (scroll to the right position on the page using JS) can now take the toolbar into account, so that the top of the comment is no longer hidden underneath the toolbar! So it even results in a nice small UX win :)

Currently it is mimicking the same behavior as the comment module had: only show a "new" indicator, not an "updated" indicator. But, really, without additional cost, we can support an "updated" indicator as well now.

Finally: tests will not pass yet, I haven't updated them.


Currently working on the "x new comments" link. Discussed a strategy to handle that as elegantly as possible with msonnabaum & effulgentsia. Stay tuned.

Status:Needs review» Needs work

The last submitted patch, node_history_markers-1991684-11.patch, failed testing.

HEAD has changed too much already apparently. #11 was on top of 3091a40783785a5a363644244a079a9036befa9e.

Issue tags:+Needs tests
StatusFileSize
new45.07 KB
FAILED: [[SimpleTest]]: [MySQL] 57,105 pass(es), 38 fail(s), and 1 exception(s).
[ View ]

Changes in this reroll

  • "x new comments" link: done!
  • Tracker module: done!

There is now a "JavaScript API for the History module". This prevents a lot of code duplication. The comment and tracker modules use this. It's tiny.

TODO

  1. Reviews.
  2. Tests. (We shouldn't waste time writing tests for this unless we're certain this is the approach we want.

We should have tests for the server-side part of this: 3 new routes, all returning JSON. Ideally, we would also have client-side tests for this in the http://drupal.org/project/fat module.

Out of scope for this patch

Not done, and should not be done:

  • the "recent content" block: it does not make sense to convert this because it also has "edit" and "delete" links (styled in the most hideous way possible FWIW, in the current state we'd better remove it), which makes it inherently uncacheable
  • the node_new_comments Views field handler is also inherently uncacheable, because it ties its query into the View's query

Not yet done, and should be an (optional) follow-up:


Note: I'll be on vacation for two weeks starting next week, so it'd be great if somebody else could update existing tests and write the additional test coverage.

Status:Needs work» Needs review

Issue tags:+sprint, +Spark

Status:Needs review» Needs work

The last submitted patch, node_history_markers-1991684-14.patch, failed testing.

+++ b/core/modules/history/js/history.jsundefined
@@ -0,0 +1,98 @@
+  needsServerCheck: function (nodeID, contentTimestamp) {
+    // First check if the content is older than 30 days, then we can bail early.
+    if (contentTimestamp < thirtyDaysAgo) {
+      return false;
+    }
+    var minLastReadTimestamp = parseInt(storage.getItem('history:read:' + currentUserID + ':' + nodeID) || 0, 10);
+    return contentTimestamp > minLastReadTimestamp;

Let's namespace this entry key with Drupal: Unlikely we'll clash, but better to keep in our code safe and separated.

function show($placeholder) {
  return $placeholder
    // Find the parent <li>.
    .closest('.comment-new-comments')
    // Find the preceding <li>, if any, and remove its 'last' class, if any.
    .prev().removeClass('last')
    // Go back to the parent <li> and hide it.
    .end().show();
}

copy-pasta in the comment // Go back to the parent <li> and hide it..

function processNodeNewCommentLinks($placeholders) {
...
// Perform an AJAX request to retrieve node view timestamps.
  var nodeIDs = Object.keys($placeholdersToUpdate);
  if (nodeIDs.length === 0) {
    return;
  }
...

Added an issue to the IE8 project to polyfill Object.keys(): #2044365: Polyfill Object.keys()

The code looks really good. Great solution to a non-obvious problem for cache-invalidation. Nice work Wim!

Once we have #1029708: History table for any entity RTBC suppose better to make #entity_type-new support per entity

#19: oh, nice! I'd been wondering already why the hell this is node-specific! :D It looks like this might go in sooner though; there's been very little activity. It'll be trivial to update the code introduced here to make it work with all entity types, so no problems there. Thanks for notifying us here!

Technically I'm on vacation, but I will finish this patch nevertheless: I'm working on adjusting existing tests and adding additional test coverage. I'm about half way.

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new22.6 KB
new2.33 KB
new64.56 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node_history_markers-1991684-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Let's namespace this entry key with Drupal: Unlikely we'll clash, but better to keep in our code safe and separated.

Great point. I now made it consistent with the localStorage entries for toolbar.module. See interdiff-minorfixes.txt.

Added an issue to the IE8 project to polyfill Object.keys(): #2044365: Polyfill Object.keys()

Lovely, thanks!


Et voila, tests fixed. Additional test coverage added. Test coverage should be complete. See interdiff-tests.txt.

I'll be AFK for the next 7 days, so please push this forward in my absence — it's almost there!

Built on 1cb65032844cb12907ed484e3c17ded1528ea44f.

Assigned:Wim Leers» Unassigned

Unassigning myself as per my AFKness explained in #22.

Status:Needs review» Needs work

The last submitted patch, node_history_markers-1991684-22.patch, failed testing.

It should just needs reroll! :)

Status:Needs work» Needs review
StatusFileSize
new62.22 KB
FAILED: [[SimpleTest]]: [MySQL] 57,484 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Rerolled.

Status:Needs review» Needs work

The last submitted patch, node_history_markers-1991684-26.patch, failed testing.

Not really happy when .end() is used in the JS but I didn't make a proper review so I'll stop by again when it's green.

Assigned:Unassigned» Wim Leers

I'll probably reroll this tomorrow.

#28: If that's all you have to nitpick about, then I'm very happy :)

reading through this patch, i'm not at all sure why we need ajax requests.

stepping back from the implementation a bit, when viewing a node, if we have the comment timestamps and the last-viewed timestamp for the user in the DOM, then we can figure out the rest from there?

same for the created by user stuff, if we have the user id and the comment author id?

i'd like to have a go at making this work without more http requests if that doesn't make anyone who has already worked on this patch too unhappy.

+1 to not using AJAX when we can just use timestamps + SJAX

We already have 3 HTTP requests per page for an authenticated user. Let's bring that number down, not up. We can absolutely do this with just client-side logic.

Yeah I'm confused why this would need AJAX as well. The only reason to avoid that would be to make <head> itself cacheable more than per-user but that's far from possible at the moment. Also comment history is a different category from contextual links or the toolbar since it's accessible by any authenticated user.

Status:Needs work» Needs review
StatusFileSize
new63.97 KB
FAILED: [[SimpleTest]]: [MySQL] 58,245 pass(es), 19 fail(s), and 0 exception(s).
[ View ]

Straight reroll of #26.

Working on implementing the feedback provided in #30–#33, with which I of course wholeheartedly agree: fixing render cache is priority number one at the moment, but of course the less HTTP requests the better. I *hope* I just was a moron earlier and didn't do this already, i.e. I hope there isn't a particular reason preventing this that #30–#33 are forgetting about :)

Status:Needs review» Needs work

The last submitted patch, node_history_markers-1991684-34.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new10.92 KB
new69.87 KB
PASSED: [[SimpleTest]]: [MySQL] 58,153 pass(es).
[ View ]

In this reroll:

  1. $request->attributes->get('account') -> $request->attributes->get('_account'), this fixes the many new test failures
  2. Finally found the cause for ThemeInfoStylesTest failing: because it was written with silly assumptions. Fixed it.
  3. Better/more strict test coverage for tracker.module.

So, now that this patch is complete from the original POV, and is (should be) green, we can properly discuss it.

StatusFileSize
new69.8 KB
FAILED: [[SimpleTest]]: [MySQL] 57,886 pass(es), 63 fail(s), and 797 exception(s).
[ View ]

Chasing HEAD. (#36 was rolled on 07298d66e40f0ade1d63e05129826d710f228773.)

Status:Needs review» Needs work

The last submitted patch, node_history_markers-1991684-37.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new69.78 KB
PASSED: [[SimpleTest]]: [MySQL] 58,273 pass(es).
[ View ]

Missed one spot.

Crell: those 3 HTTP requests can go down once we have #2005644: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for in-place editing metadata, introduce drupalSettings.user.permissionsHash, which will enable smarter client-side caching.


same for the created by user stuff, if we have the user id and the comment author id?

That is already the case; no HTTP request is performed in this case. ;) See comment-by-viewer.js.

If it helps, I'm happy to split that off into a separate patch.


Less HTTP requests, more embedded metadata please!

stepping back from the implementation a bit, when viewing a node, if we have the comment timestamps and the last-viewed timestamp for the user in the DOM, then we can figure out the rest from there?

The two crucial questions for this are:

  1. How do you know which nodes are on the current page?
  2. What function/callback/method/event/entry point do we implement to insert this metadata into the DOM (presumably using drupalSettings)?

Because… you can't inject user-specific metadata in the DOM as part of the node rendering process itself — that is precisely the costly process we want to avoid by having an operational entity render cache: #1605290: Enable entity render caching with cache tag support!

I can think of two ways of supporting this:

Option 1: #post_render_cache callback that uses introspection

Introspection.
We can introspect the data in the render cache today; i.e. we cannot know which nodes are being displayed to the end user. To do this, we can use $element['#cache']['keys'] and $element['#cache']['tags']. Yay!
Note that this metadata is not cached itself, but it is rebuilt on every page load, to be able to know which cache entry to retrieve.
Render cache retrieval callback.
When a render cache entry is retrieved, we have no way of reacting to it. So how are we then supposed to #attach the user-specific metadata?
I double-checked this with amateescu, just to make sure: https://twitter.com/wimleers/status/368489238808887296. I also reviewed drupal_render() code's #cache handling and confirmed this is indeed not possible.
Once we have a #post_cache_render callback or something like that, we can make sure that it receives everything under the render array's #cache key, which contains sufficient metadata.
Render cache retrieval callback firing hooks.
Assuming we have a render cache retrieval callback. Now what about e.g. hook_node_view(), which is where it is decided that we should have "new" markers — i.e. if the comment module is disabled, we don't want this metadata, only when a certain module is configured in a certain way, we want this metadata.

Option 2: allow the render array build process (which invokes the right hooks) set #uncached_attached

Fact is that we build the whole $build = array('#cache' => array(…), '#ENTITY_TYPE' => OBJECT, '#attached' => array(), '#more_stuff' => array()) structured array on each page load, even when we use the render cache. Which means all of comment.module's hooks are fired. So … why not just attach user-specific data here? Well, because #attached is going to be cached too, so then user-specific metadata ends up being served to all users with the same roles.

Conclusion

So … a way to attach things that should not be cached is another way to achieve this. It is technically possible.

Again, this is definitely not yet possible. If we want to support this, it should be a follow-up. There we can then bikeshed to choose between option 1 or option 2.


Progress, please!

Once we have something like a #post_cache_render callback or #uncached_attached, I'm happy to roll a patch to change the code being introduced here to use that instead. And then properly evaluate the pros and cons.

But it'd be a mistake to postpone *this* issue, because this is a blocker for #1605290: Enable entity render caching with cache tag support. Let's not allow perfect to be the enemy of good here. This is a step forward.

If you're not convinced yet it's a step forward, let me explain you why it is not at all as bad as some of you might think :)


The number of HTTP requests for each scenario

Anonymous users: 0 HTTP requests. Authenticated users: see below.

None of the nodes on the front page has any comments?
0 HTTP requests
>0 nodes on the front page have >0 comments?
0 or 1 HTTP request, for all nodes, with the following heuristics/optimizations:
  1. If the comment was made >30 days ago, the system assumes it has been read already; if all comments were made >30 days ago, then no HTTP request is performed at all.
  2. For the remaining nodes (those that have comments <=30 days ago), we check if we already have data in localStorage. If the "last read" timestamp in localStorage for the node is later than the last comment time, it is disregarded in a similar way as comments >30 days ago. If all comments are older than their corresponding "last read" timestamps, then no HTTP request is performed at all.
  3. For all remaining comments: an HTTP request is performed, the retrieved "last read" timestamps for the nodes are cached in localStorage. The page is updated accordingly.
So, consequently, the majority of all page loads will trigger 0 additional HTTP requests. A HTTP request to retrieve "last read" timestamps is only performed if and only if the user does not visit any of the nodes that have new comments. One could reason it is possible that once we have any "last read" timestamp in localStorage that that is sufficient, because when the user visits the node to read the comments, then the localStorage entry is updated. This is correct as long as the user only uses one and only device; if the user uses a second device to read the comments, then the first device's localStorage would never be updated accordingly. So: a HTTP request is performed on the front page on each page load if and only if the user does not go and >=1 of the nodes with new comments.
Full node page with 0 comments?
0 HTTP requests to render the page
1 HTTP request after the page had loaded, to inform the server that the user has read the node.
(Note that this will also have updated the node's localStorage entry, so when the user looks at the front page, there already is a localStorage entry!)
Full node page with >0 comments?
0 or 1 HTTP request to render the page, with the following heuristics/optimizations (almost the same, slightly simpler):
  1. If the latest comment was made >30 days ago, the system assumes it has been read already, and no HTTP request is performed at all.
  2. Otherwise, we check if we already have data in localStorage. If the "last read" timestamp in localStorage for the node is later than the last comment time, it is disregarded in a similar way as a latest comment >30 days ago, and no HTTP request is performed at all.
  3. If we arrive here, then a HTTP request is performed, the retrieved "last read" timestamp is cached in localStorage and the page is updated accordingly.
1 HTTP request after the page had loaded, to inform the server that the user has read the node.
(Note that this will also have updated the node's localStorage entry, so when the user looks at the front page, there already is a localStorage entry!)

As you can see, a lot of effort is made to ensure as few HTTP requests as possible are made.

Hmm. On a full node page, we know which node it is - so the last read timestamp we could easily get inline for the main node at least.

I see the problem with node listings - we specifically don't know what they are if they're render cached unless we add that render cache post callback thingy and figure it out there. That's also potentially the place we could post-process this in PHP for sites that would prefer that.

#41: Given that:

  1. It should be comment_node_view() that attaches the last read timestamp.
  2. The last read timestamp is user-specific.

combined with the explanation in #40 would imply that we'd be spoiling the render cache with user-specific values.

Or did you have something else in mind?

As an alternative: If we can somehow track the "primary object" for a page (which is something Dave Reid has asked about for a number of his modules), could we instead have a view listener that adds the last-viewed timestamp of that object to the HTML head (either as JS settings or attributes on an html or body tag or something else) before the page is rendered, but after the body is rendered? (Wim, this is what I was trying to get at this morning, unsuccessfully.) We'll be loading the object anyway, but this would let us add the data we need without impacting the rendering of the body string at all.

Edit: See #2068471: Normalize Controller/View-listener behavior with a Page object

Just a thought: a search results page does not have any "primary object".. in our case, it could return comments from various primary objects (nodes).

the best time to inexpensively get at the list of nids is when we are building the cache the first time.

so - can we store metadata with cached objects? the list of nids, comment timestamps etc can be stored with the rendered output without polluting it.

i'm not sure if that will require too much change at this point though.

Don't see why we couldn't store that the same was as #attached. That doesn't handle the last viewed timestamps though since those need to be fetched when the cache items is viewed rather than cached.

yay to #attached.

last viewed - i'm not sure i follow that bit? i think we are comparing 'last viewed by this user' to 'comment created at'? or something like that - we can get at the latter when building the cached item and stuff it wherever we put the metadata, and the former we look up for each user, and stuff in the DOM. or maybe i'm missing something here...

Comment created at - this should be in a data- attribute I'd think but yeah it could also be in metadata if we wanted.

Last viewed on the node - this where it gets harder. Say you have a view that lists nodes, and the comments / new comments link is set to display for each node. Right now we don't have a way to get the list of node IDs cleanly from the cache entry - because that'd be cached HTML, not a list of nids.

We can stick the nids in #attached though, then at some point after it's retrieved, we'd then combine the nids + user id to query the history table. Right now that's happening in the AJAX request but if we want to do it inline in PHP we need to figure out where it happens.

#43: that is flawed, see #44.

#45: see #40, we already have the "which entities" metadata in #cache. The last comment timestamp already is being cached in the render cache, since that's the same for all users. It is the user-specific metadata that is the problem.
I think you may want to read #40 again.

#48: yes, that is possible — I describe two ways of doing that in #40 (option 1: #post_render_cache callback, option 2: #uncached_attached). But the problem is that it is impossible to do today. Supporting this will surely lead to another bikeshed. I don't want this issue and thus entity render caching in general to be blocked on that detail.


The majority of the code in this patch is going to be the same, with or without HTTP requests. It'll be trivial to get rid of the HTTP requests and update the JS to just use metadata embedded somewhere in the HTML. This patch gets us closer to where we need to be. Isn't that what matters most?
If we can agree to get this in first, then I'd be happy to work on a new #post_render_cache vs. #uncached_attached issue.

My personal preference:

1. No HTTP request
2. HTTP request(s) + render caching
3. no render caching

I think after reading over the recent comments here #49 is probably an accurate summary of where things are at, so it looks like 2 is the best we can do for now.

Getting the timestamp the current user last looked at a node, when the node is just a string of markup... tricky stuff!

#50: I totally agree with that preference order. #49 says: "let's do 2 now because it can happen today, then let's move it to 1".

P.S.: if anybody was wondering: I don't think anybody here hates "just 1 more HTTP request" more than I do. Look at my website's frontpage — you'll see a total of 5 requests: 1 for HTML, 1 for CSS (with 2 images and an optimized font embedded as data URIs), 2 for images, 1 for Google Analytics.
I hate hate hate hate overhead and slowness — so much, that I hacked core to work around Drupal 7's stupidity to *always* attach jQuery's JS.

could we make a js array of nids that are new for the current user and pass it through without an extra request? js can match nids in the array with nids in the markup.

I don't think it's possible to generate that meaningfully - for example take an active contributor on Drupal.org - there'll be hundreds of nodes that they viewed within a three month period which is the threshold for node history. The current patch caches timestamps for recently viewed nodes in local storage which is a bit similar, but pre-loading all values seems like too much.

make a (configurable?) limit that's smaller? how was the original threshold on what's "new" decided? is it holy?

what about a radioactivity style decay of "newness" with a 100 node limit, rather than a flat time cutoff? nodes with a single comment drop off the list faster than those with more activity..

I suspect we could come up with an alternate algorithm for "newness" that would sit better with what we're trying to do here, if we allowed ourselves to.

also, (sorry for the noise) an active contributor is either looking at the nodes regularly and keeping the list short through the very fact that they're active, or not really using that functionality anyway much so they won't miss it if we tighten it up a bit.

#54–55: changing the definition of "newness" is interesting. It's also massive scope creep. We could easily discuss different algorithms endlessly.

Let us please not change the definition of "newness" in this issue.

even if we don't redefine "newness" what is a realistic upper limit on how big the list of nids can get?

what iiiif, we say, "send a list of up to X nids that are new for this user along with the page. If the list is max-length, once we process what we have then issue a http request to see if we need another X nids" and loop that with a little timeout until we've got everything we need.

The requests that we're doing can be cached as they come through.

also, why is discussing introducing #post_render_cache vs. #uncached_attached "just a followup" (they both sound like pretty big API additions/changes to me) but saying that we could introduce a single admin setting putting an upper limit on how many/long nodes can be "new" for each user is "massive scope creep"?

#58: Here on d.o I'm actively following HUNDREDS of nodes (issues) that are being changed all the time. And it's not just new nodes. It's also nodes that get new comments. And it's updated nodes. I get notified when nodes that haven't been touched in years are commented upon. I get notified when an old docs page is modified for the first time in years.
All of that is useful.

Changing that is IMHO clearly far beyond what this issue is trying to do.

This issue is just about avoiding the render cache from breaking, it's not about what the patch has become - the patch *could* be change newness so that it's easier to avoid breaking the render cache. Multiple people have voiced a serious concern over extra http requests - I'm just brainstorming ideas :)

This isn't about removing useful data... even if you're following hundreds of nodes, are there hundreds of "new" tags in your feed at once? By the time you navigate through the pagination of the feed, the current patch will have put all the hundreds of new nodes in localStorage anyway, so that's no different to loading them up-front IIRC.

Even if it's hundreds, what's the real-world perf hit of one array with a few hundred entries?

catch is talking about a 3 month period, you're talking about near-instantaneous feedback. I think we can get away with shortening it from 3 months if that leads to problems, I don't see how anything I'm suggesting has much impact at all on anyone who is an active user or tracking near-realtime feedback.

#60 - If you're responding directly to #58 and saying that useful data will be missing, I don't think you read it properly, or I explained it badly.

#58 suggests a way to not change newness at all, and maybe avoid more requests by "warming" the localStorage cache a bit with the nodes we think are most likely to generate an extra request during normal site navigation - the newest ones.

I'm saying, pick a number X we can "warm" the localStorage cache with.

Say X is 30 (it could be 300, or even 3000 if profiling allows it, X is sort of arbitrary but would need to comfortably cover at least one full page of "new" nodes for the "average" user on a "normal" node summary page).. send the 30 newest "new" nids (cached globally per-user) that can immediately be merged into the localStorage cache *as though* we had previously seen them and received their data with an extra request.

That way, for anyone with less than X new nodes for this session, they would need no extra request, ever, saving a maximum of X page requests per user. For anyone with more than X new nodes, they will indeed need to start building out their localStorage as described in #40 through AJAX requests, but at least we can say we tried :).

If we wanted to, we could even say that if we received X nids we suspect there will be more on the server, so do an AJAX request to get the next X nids that we can stick in our localStorage cache "in the background" after a couple of seconds since the last AJAX hit and keep doing this until we get less than X new "new" nids from the server - at this point we know that we have them all.

I think there's no "primary viewed object" but we knowing when node or any entity is viewed in full|default view mode.
If node a book page so when book page rendered in full mode (no matter how many of them are rendered by current router) then each of objects should be supposed as "seen by user"

StatusFileSize
new1.48 KB
new67.67 KB
PASSED: [[SimpleTest]]: [MySQL] 58,289 pass(es).
[ View ]

I don't understand why this thread has descended into bike shedding like it has. The approach here is simple. The extra http request is not desirable, but we've already discussed a plan to remove them in a followup, therefore there's nothing we need to optimize for in this issue.

Getting render cache working reliably at all is complex enough, let's just get this in and improve it as needed in followups.

This patch is just #39 with some minor cleanups. The rest looked good to me.

but we've already discussed a plan to remove them in a followup,

Mmm, the followup plan of adding a new "post cache render callback/phase" involves a reasonably large API change/addition that is not approved by a Core maintainer AFAICS. I don't have much faith that it will get in based on what I've read and where we're at in the release cycle.

Having a discussion about how we can improve the patch in the context that the proposed followup plan may never happen isn't bike-shedding.

Suggesting a new approach when somebody previously said something was impossible is not bike-shedding.

That said, everything from #52 through to #62 could definitely be moved to a followup issue and generally, I'm happy enough with the approach being taken here and the decision to triage the issue so we can get *something* in.

#post_cache_render is an API addition, which is not effected by API freeze. Also, performance is one of the areas where exceptions are likely to be made even if we are breaking APIs (which I dont see that we are). I am very confident we can get that in.

ok. There's a few things we can try to improve this after it gets in, I'm sure *something* will get done :)

ok, sorry i got in the way here. let's proceed with #64.

Issue summary:View changes

Updated issue summary.

So we seem to have consensus to get this in. Yay!

Issue summary updated.

Who wants to RTBC?

Status:Needs review» Reviewed & tested by the community

Everybody seems to be in agreement that this is a step forward, so rtbc.

I've opened #2073535: Node history markers require an HTTP request and I'm happy with doing that in a follow-up once the mechanism is in place here.

Not committing this yet because I still need to do a review of the whole patch again - that shouldn't stop another committer if they get here first, otherwise I'll try to look again as soon as I've got more time.

Status:Reviewed & tested by the community» Needs work
  1. +++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.php
    @@ -132,4 +133,39 @@ public function commentPermalink(Request $request, CommentInterface $comment) {
    +        'first_new_comment_link' => url("node/$node->nid", array('query' => $query, 'fragment' => 'new')),

    This needs to use 'node/' . $node->id() instead, or it will explode when the BC decorator is removed.

  2. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentNewIndicatorTest.php
    @@ -64,17 +105,30 @@ public function testCommentNewCommentsIndicator() {
    +    $expected = array($this->node->nid => array(
    ...
    +      'first_new_comment_link' => url('node/' . $this->node->nid, array('fragment' => 'new')),

    Same here.

  3. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentNewIndicatorTest.php
    @@ -64,17 +105,30 @@ public function testCommentNewCommentsIndicator() {
    +    $this->renderNewCommentsNodeLinks(array($this->node->nid));

    And another one ;)

  4. +++ b/core/modules/history/lib/Drupal/history/Controller/HistoryController.php
    @@ -0,0 +1,69 @@
    +    if ($request->attributes->get('_account')->isAnonymous()) {

    Should this switch to use the service now?

  5. +++ b/core/modules/history/lib/Drupal/history/Tests/HistoryTest.php
    @@ -0,0 +1,155 @@
    +   * @param $node_id
    +   *   A node ID.

    I'm looking only at the added code, so no idea if this is moved or added, but it's missing a type.

  6. +++ b/core/modules/history/lib/Drupal/history/Tests/HistoryTest.php
    @@ -0,0 +1,155 @@
    +    $nid = $this->test_node->nid;

    Yeah :)

  7. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeInfoStylesTest.php
    @@ -47,18 +47,19 @@ function testStylesheets() {
    +    $this->assertTrue(1 === count($this->xpath("//link[contains(@href, '$base/base-add.css')]")), "$base/base-add.css found");
    +    $this->assertTrue(1 === count($this->xpath("//link[contains(@href, '$base/base-override.css')]")), "$base/base-override.css found");
    +    $this->assertTrue(0 === count($this->xpath("//link[contains(@href, 'base-remove.css')]")), "base-remove.css not found");

    Why not use assertIdentical() ?

Status:Needs work» Needs review
StatusFileSize
new10.97 KB
new69.39 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node_history_markers-1991684-73.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

All fixed, and found even more instances of ->nid :)

Status:Needs review» Needs work

The last submitted patch, node_history_markers-1991684-73.patch, failed testing.

#2024963: Move baseFieldDefinitions from storage to entity classes just got committed, hence it no longer applies.

Status:Needs work» Needs review
StatusFileSize
new69.09 KB
PASSED: [[SimpleTest]]: [MySQL] 58,443 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Interdiff looks good. The node BC decorator got in fact removed in the meantime, so we know that this is good now :)

Changing the CommentController to use ControllerBase is outside the scope of this issue and easily possibly later on.

+++ b/core/modules/tracker/tracker.pages.inc
@@ -67,23 +69,23 @@ function tracker_page($account = NULL, $set_title = FALSE) {
-      $mark_build = array(
-        '#theme' => 'mark',
-        '#status' => node_mark($node->id(), $node->getChangedTime()),
...
+      $mark_build = array('#theme' => 'mark', '#status' => '');

empty mark? so no reason to render it. also #status should be one of constants, see #2062097: Remove calls to deprecated global $user in theme.inc - I think theme_mark() should not make access checking an moved to node module

Status:Reviewed & tested by the community» Needs work

That sounds like a needs work.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new952 bytes
new68.99 KB
FAILED: [[SimpleTest]]: [MySQL] 58,504 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Good catch, easy fix — it was indeed no longer necessary.

Thanks andypost!

Status:Reviewed & tested by the community» Needs work

The last submitted patch, node_history_markers-1991684-80.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1008 bytes
new69.85 KB
PASSED: [[SimpleTest]]: [MySQL] 58,533 pass(es).
[ View ]

The change in #80 (to address #78) caused this. The resulting HTML now doesn't have a space anymore after the title followed by nothing (yay!), which in turn breaks the test … because it was already specifically accounting for this space, which is something theme_mark() always outputs.

Or… how a single space broke a test :)

Assigned:Wim Leers» catch

Sorry, catch, your list is getting long. :(

Status:Reviewed & tested by the community» Needs work
  1. +++ b/core/modules/history/lib/Drupal/history/Controller/HistoryController.php
    @@ -0,0 +1,69 @@
    +    // Only handle up to 100 nodes.

    Why only up to 100?

  2. +++ b/core/modules/tracker/js/tracker-history.js
    @@ -0,0 +1,122 @@
    + * Attaches behaviors for the Tracker module's History module integration.

    I'm a bit confused why this has to be implemented separately for tracker module and comment module as opposed to the logic just living in history module and those two calling it. Some of the code is almost identical.

Also there's no hunks for forum module here, but that's exactly the same as tracker in terms of how nodes are marked (or very close). Tracker module doesn't prevent entity render caching (at least not until we have a table row view mode or something), so could we remove that for now maybe? At the moment it's duplicating code and inconsistent.

What I think would be nice would be having this replacement work for Views - since we can cache rendered HTML for those and history marks prevent it at the moment, but again that can all be done in a follow-up - doesn't block anything unlike the comment marks.

Also the module_exists() here should be converted per #2045931: Replace all module_exists() deprecated function calls..

Status:Needs work» Needs review
StatusFileSize
new3.47 KB
new69.99 KB
FAILED: [[SimpleTest]]: [MySQL] 57,251 pass(es), 580 fail(s), and 307 exception(s).
[ View ]

#84:

Why only up to 100?
To prevent malicious users from easily DoSsing Drupal sites by requesting thousands of node IDs at once. I'm happy to remove that line though.
I'm a bit confused why this has to be implemented separately for tracker module and comment module as opposed to the logic just living in history module and those two calling it. Some of the code is almost identical. →→→ "Remove Tracker module changes"
I share your concern (and hatred) for duplication. But I ask you to reconsider this.
If you look at the first 20 LoC or so of comment-new-indicator.js, node-new-comments-link.js and tracker-history.js (those are the 3 key files), they all look very similar. But that's precisely because I introduced history.js, which contains the parts that really are identical. If you look further down those 3 files, you'll see that they're vastly different.

In words: it is effectively impossible to apply the same logic, here's what they do:

  1. comment-new-indicator.js: given a full node page with many comments, compare each comment's timestamp with the "last node time read" timestamp, add "new" marker if appropriate.
  2. node-new-comment-link.js: given a listing of node teasers that contain the last comment timestamp, set value of "x new comments" link for each node if appropriate. It comes with show/hide/remove logic because of the way the resulting HTML must be built.
  3. tracker-history.js: given a table listing of nodes, show
    1. "new" or "updated" markers next to node titles as appropriate (completely different from comment-new-indicator.js!)
    2. "x new replies" link for each node if appropriate. It comes without node-new-comment-link.js's show/hide/remove logic because it is not needed here. Also: the DOM structure is completely different, so the different data- attributes are in completely different locations. Finally, the code for retrieving "x new replies" is indeed very similar to that in node-new-comment-links.js, but the only way to make it shared/reusable is by passing a whole lot of callbacks (at least 3 AFAICT) into such shared/reusable code, which would make reuse possible, but would yield at least as many lines of code, making the reuse rather pointless.
"support forum module"
Hah — everybody forgot about that. But then again, it is like you say not necessary to fix that to unblock entity render caching.
"support Views"
From #14:
the node_new_comments Views field handler is also inherently uncacheable, because it ties its query into the View's query
Also the module_exists() here should be converted […]
Done!

Given these answers, please let me know whether you still feel the tracker module changes should be removed from this patch or not.

I think we should split tracker out - and either have one or three issues to support tracker, forum and views tables (one might be OK?).

To prevent malicious users from easily DoSsing Drupal sites by requesting thousands of node IDs at once. I'm happy to remove that line though.

max_input_vars is in PHP 5.3.9 and defaults to 1,000 - I think it's fine to rely on that.

Status:Needs review» Needs work

The last submitted patch, node_history_markers-1991684-87.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new17.66 KB
new54.87 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node_history_markers-1991684-90.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
I think we should split tracker out
Done.
either have one or three issues to support tracker, forum and views tables (one might be OK?).
I'd much prefer three. Tracker is complete, only Forum needs to be done. Views, as I already said, is seemingly impossible to support so shouldn't hold up either. Here they are:
  1. #2082315: Tracker history markers ("new" and "updated" markers, "x new replies" links) forces render caching to be per user
  2. #2082317: Forum history markers ("new" and "updated" markers, "x new posts" links) forces render caching to be per user
  3. #2082319: Comment's node_new_comments View field history markers ("new" comment marker) forces render caching to be per user
max_input_vars is in PHP 5.3.9 and defaults to 1,000 - I think it's fine to rely on that.
Ah :) Nicer indeed. Removed that from the patch.

Status:Needs review» Needs work

The last submitted patch, node_history_markers-1991684-90.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new54.84 KB
FAILED: [[SimpleTest]]: [MySQL] 57,978 pass(es), 602 fail(s), and 854 exception(s).
[ View ]

Chasing HEAD. (The commit of #2067017: Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants broke the patch.)

Status:Needs review» Needs work

The last submitted patch, node_history_markers-1991684-92.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.78 KB
new54.91 KB
FAILED: [[SimpleTest]]: [MySQL] 57,870 pass(es), 583 fail(s), and 307 exception(s).
[ View ]

Chasing HEAD. (I didn't catch all the s/VERSION/Drupal::VERSION/ changes. Fixed.)

Status:Needs review» Needs work

The last submitted patch, node_history_markers-1991684-94.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1015 bytes
new54.91 KB
FAILED: [[SimpleTest]]: [MySQL] 58,870 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Should be green.

Status:Needs review» Needs work

The last submitted patch, node_history_markers-1991684-96.patch, failed testing.

Status:Needs work» Needs review

#96's testbot run failed on 3 assertions in TrackerTest. But nothing of tracker.module is affected by this patch since #90.

I ran the tests locally, and all tests passed. So… retesting.

Status:Needs review» Needs work
Issue tags:+JavaScript, +sprint, +Spark, +D8 cacheability

The last submitted patch, node_history_markers-1991684-96.patch, failed testing.

Test failures now reproduced locally. Will fix over the weekend.

Status:Needs work» Needs review
StatusFileSize
new2.66 KB
new57.51 KB
PASSED: [[SimpleTest]]: [MySQL] 58,931 pass(es).
[ View ]

If this one doesn't come back green, I'm going to start throwing things.

Status:Needs review» Needs work
Issue tags:-JavaScript, -sprint, -Spark, -D8 cacheability

The last submitted patch, node_history_markers-1991684-102.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+JavaScript, +sprint, +Spark, +D8 cacheability

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work
Issue tags:-sprint, -Spark+Needs reroll

Patch no longer applies.

Issue tags:+sprint, +Spark

Adding tags back in

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new57.16 KB
PASSED: [[SimpleTest]]: [MySQL] 59,114 pass(es).
[ View ]

Chasing HEAD.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, node_history_markers-1991684-108.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

Back to RTBC as per #105. #108 was a straight reroll.

Title:Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per userChange notice:Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user
Category:bug» task
Status:Reviewed & tested by the community» Active

Committed/pushed to 8.x, thanks!

We should add a change notice for the new history js API. Especially once entity render caching is in, we should also add some kind of public service change notice on how to do this.

Title:Change notice:Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per userNode history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user
Assigned:catch» Wim Leers
Category:task» bug
Status:Active» Fixed
Issue tags:-sprint, -Avoid commit conflicts

Change notice written: https://drupal.org/node/2086767.

Thanks for writing a change notice which will guide anyone else who wants to move user specific customizations from PHP to js. Well done.

Status:Fixed» Closed (fixed)

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

Issue tags:+Performance

Adding "Performance" tag for tracking purposes.

Issue summary:View changes

Updated issue summary.