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.

CommentFileSizeAuthor
#108 node_history_markers-1991684-108.patch57.16 KBWim Leers
#102 node_history_markers-1991684-102.patch57.51 KBWim Leers
#102 interdiff.txt2.66 KBWim Leers
#96 node_history_markers-1991684-96.patch54.91 KBWim Leers
#96 interdiff.txt1015 bytesWim Leers
#94 node_history_markers-1991684-94.patch54.91 KBWim Leers
#94 interdiff.txt1.78 KBWim Leers
#92 node_history_markers-1991684-92.patch54.84 KBWim Leers
#90 node_history_markers-1991684-90.patch54.87 KBWim Leers
#90 interdiff.txt17.66 KBWim Leers
#87 node_history_markers-1991684-87.patch69.99 KBWim Leers
#87 interdiff.txt3.47 KBWim Leers
#82 node_history_markers-1991684-82.patch69.85 KBWim Leers
#82 interdiff.txt1008 bytesWim Leers
#80 node_history_markers-1991684-80.patch68.99 KBWim Leers
#80 interdiff.txt952 bytesWim Leers
#76 node_history_markers-1991684-76.patch69.09 KBWim Leers
#73 node_history_markers-1991684-73.patch69.39 KBWim Leers
#73 interdiff.txt10.97 KBWim Leers
#64 node_history_markers-1991684-64.patch67.67 KBmsonnabaum
#64 interdiff.txt1.48 KBmsonnabaum
#39 node_history_markers-1991684-39.patch69.78 KBWim Leers
#37 node_history_markers-1991684-37.patch69.8 KBWim Leers
#36 node_history_markers-1991684-36.patch69.87 KBWim Leers
#36 interdiff.txt10.92 KBWim Leers
#34 node_history_markers-1991684-34.patch63.97 KBWim Leers
#26 node_history_markers-1991684-26.patch62.22 KBamateescu
#22 node_history_markers-1991684-22.patch64.56 KBWim Leers
#22 interdiff-minorfixes.txt2.33 KBWim Leers
#22 interdiff-tests.txt22.6 KBWim Leers
#14 node_history_markers-1991684-14.patch45.07 KBWim Leers
#11 node_history_markers-1991684-11.patch19.94 KBWim Leers
#4 added-data-date-attribute-1991684-3.patch635 bytesrcaracaus
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

Issue summary: View changes

Updated issue summary.

moshe weitzman’s picture

Priority: Normal » Major

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

larowlan’s picture

Issue tags: +JavaScript
larowlan’s picture

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

rcaracaus’s picture

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.

catch’s picture

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?

dcam’s picture

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

Crell’s picture

Status: Active » Needs review

Status: Needs review » Needs work

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

Wim Leers’s picture

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.

catch’s picture

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

Wim Leers’s picture

Title: Comment new mark forces render caching to be per user » Node 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
FileSize
19.94 KB

#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.

Wim Leers’s picture

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

Wim Leers’s picture

Issue tags: +Needs tests
FileSize
45.07 KB

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.

Wim Leers’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Issue tags: +sprint, +Spark

Status: Needs review » Needs work

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

jessebeach’s picture

+++ 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!

andypost’s picture

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

Wim Leers’s picture

#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!

Wim Leers’s picture

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.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
22.6 KB
2.33 KB
64.56 KB

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.

Wim Leers’s picture

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.

Wim Leers’s picture

It should just needs reroll! :)

amateescu’s picture

Status: Needs work » Needs review
FileSize
62.22 KB

Rerolled.

Status: Needs review » Needs work

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

nod_’s picture

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.

Wim Leers’s picture

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 :)

Anonymous’s picture

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.

thedavidmeister’s picture

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

Crell’s picture

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.

catch’s picture

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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
63.97 KB

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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
10.92 KB
69.87 KB

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.

Wim Leers’s picture

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

Status: Needs review » Needs work

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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
69.78 KB

Missed one spot.

Wim Leers’s picture

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.

catch’s picture

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.

Wim Leers’s picture

#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?

Crell’s picture

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

amateescu’s picture

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).

Anonymous’s picture

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.

catch’s picture

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.

Anonymous’s picture

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...

catch’s picture

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.

Wim Leers’s picture

#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.

thedavidmeister’s picture

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!

Wim Leers’s picture

#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.

thedavidmeister’s picture

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.

catch’s picture

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.

thedavidmeister’s picture

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

thedavidmeister’s picture

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.

thedavidmeister’s picture

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.

Wim Leers’s picture

#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.

thedavidmeister’s picture

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.

thedavidmeister’s picture

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"?

Wim Leers’s picture

#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.

thedavidmeister’s picture

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.

thedavidmeister’s picture

#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.

andypost’s picture

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"

msonnabaum’s picture

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.

thedavidmeister’s picture

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.

msonnabaum’s picture

#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.

thedavidmeister’s picture

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

Anonymous’s picture

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

Wim Leers’s picture

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

Issue summary updated.

Who wants to RTBC?

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

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.

Berdir’s picture

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() ?

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
10.97 KB
69.39 KB

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.

Wim Leers’s picture

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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
69.09 KB
Berdir’s picture

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.

andypost’s picture

+++ 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

webchick’s picture

Status: Reviewed & tested by the community » Needs work

That sounds like a needs work.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
952 bytes
68.99 KB

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.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1008 bytes
69.85 KB

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 :)

webchick’s picture

Assigned: Wim Leers » catch

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

catch’s picture

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.

catch’s picture

catch’s picture

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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
3.47 KB
69.99 KB

#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.

catch’s picture

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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
17.66 KB
54.87 KB
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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
54.84 KB

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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.78 KB
54.91 KB

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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1015 bytes
54.91 KB

Should be green.

Status: Needs review » Needs work

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

Wim Leers’s picture

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.

Wim Leers’s picture

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

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

Wim Leers’s picture

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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.66 KB
57.51 KB

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.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +sprint, +Spark, +D8 cacheability
catch’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

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

Patch no longer applies.

alexpott’s picture

Issue tags: +sprint, +Spark

Adding tags back in

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
57.16 KB

Chasing HEAD.

Status: Reviewed & tested by the community » Needs work

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

catch’s picture

Issue tags: +Avoid commit conflicts
Wim Leers’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

Title: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user » Change 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.

Wim Leers’s picture

Title: Change notice:Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user » Node 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.

moshe weitzman’s picture

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

Wim Leers’s picture

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

Issue tags: +Performance

Adding "Performance" tag for tracking purposes.

Wim Leers’s picture

Issue summary: View changes

Updated issue summary.