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
- Set
drupalSettings.user.uid
to the current user's ID, so that we can set theby-viewer
class on comments on the client-side. - Attach JavaScript that adds the
by-viewer
class (no extra HTTP requests needed).
and
- 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. - 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. - (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).
Related Issues
#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.
Comment | File | Size | Author |
---|---|---|---|
#108 | node_history_markers-1991684-108.patch | 57.16 KB | Wim Leers |
#102 | node_history_markers-1991684-102.patch | 57.51 KB | Wim Leers |
#102 | interdiff.txt | 2.66 KB | Wim Leers |
#96 | node_history_markers-1991684-96.patch | 54.91 KB | Wim Leers |
#96 | interdiff.txt | 1015 bytes | Wim Leers |
Comments
Comment #0.0
moshe weitzman CreditAttribution: moshe weitzman commentedUpdated issue summary.
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedI changed your suggestion into a Proposed Resolution in the issue summary since I think it is a great idea.
Comment #2
larowlanComment #3
larowlanSo @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
Comment #4
rcaracaus CreditAttribution: rcaracaus commentedI 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() ..
Sure that is a standard error, but thought I would post anyway.
Comment #5
catchCaching 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?
Comment #6
dcam CreditAttribution: dcam commentedhttp://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.
Comment #7
Crell CreditAttribution: Crell commentedComment #9
Wim LeersWorking on this! I've got it working for the comment "new" indicator. Currently working on the "x new comments" links.
Comment #10
catchNot comment module, but iirc there's new and updated marks for nodes as well which could use the same treatment.
Comment #11
Wim Leers#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, theby-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:
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.
Comment #13
Wim LeersHEAD has changed too much already apparently. #11 was on top of 3091a40783785a5a363644244a079a9036befa9e.
Comment #14
Wim LeersChanges in this reroll
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
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:
node_new_comments
Views field handler is also inherently uncacheable, because it ties its query into the View's queryNot yet done, and should be an (optional) follow-up:
node_admin_nodes()
, which is the non-View-fallback for the Views-poweredadmin/content
page, which I can't develop against because of #2021779: Decouple shortcuts from menu links, plus the View version of it actually no longer does this: #2043403: admin/content View does not have "new"/"update" indicatorsNote: 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.
Comment #15
Wim LeersComment #16
Wim LeersComment #18
jessebeach CreditAttribution: jessebeach commentedLet's namespace this entry key with
Drupal:
Unlikely we'll clash, but better to keep in our code safe and separated.copy-pasta in the comment
// Go back to the parent <li> and hide it.
.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!
Comment #19
andypostOnce we have #1029708: History table for any entity RTBC suppose better to make #entity_type-new support per entity
Comment #20
Wim Leers#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!
Comment #21
Wim LeersTechnically 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.
Comment #22
Wim LeersGreat point. I now made it consistent with the
localStorage
entries fortoolbar.module
. Seeinterdiff-minorfixes.txt
.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.
Comment #23
Wim LeersUnassigning myself as per my AFKness explained in #22.
Comment #25
Wim LeersIt should just needs reroll! :)
Comment #26
amateescu CreditAttribution: amateescu commentedRerolled.
Comment #28
nod_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.
Comment #29
Wim LeersI'll probably reroll this tomorrow.
#28: If that's all you have to nitpick about, then I'm very happy :)
Comment #30
Anonymous (not verified) CreditAttribution: Anonymous commentedreading 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.
Comment #31
thedavidmeister CreditAttribution: thedavidmeister commented+1 to not using AJAX when we can just use timestamps + SJAX
Comment #32
Crell CreditAttribution: Crell commentedWe 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.
Comment #33
catchYeah 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.Comment #34
Wim LeersStraight 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 :)
Comment #36
Wim LeersIn this reroll:
$request->attributes->get('account')
->$request->attributes->get('_account')
, this fixes the many new test failuresThemeInfoStylesTest
failing: because it was written with silly assumptions. Fixed it.tracker.module
.So, now that this patch is complete from the original POV, and is (should be) green, we can properly discuss it.
Comment #37
Wim LeersChasing HEAD. (#36 was rolled on 07298d66e40f0ade1d63e05129826d710f228773.)
Comment #39
Wim LeersMissed one spot.
Comment #40
Wim LeersCrell: 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.
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!
The two crucial questions for this are:
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
$element['#cache']['keys']
and$element['#cache']['tags']
. Yay!#attach
the user-specific metadata?drupal_render()
code's#cache
handling and confirmed this is indeed not possible.#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.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.
(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!)
(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.
Comment #41
catchHmm. 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.
Comment #42
Wim Leers#41: Given that:
comment_node_view()
that attaches the last read timestamp.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?
Comment #43
Crell CreditAttribution: Crell commentedAs 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
Comment #44
amateescu CreditAttribution: amateescu commentedJust a thought: a search results page does not have any "primary object".. in our case, it could return comments from various primary objects (nodes).
Comment #45
Anonymous (not verified) CreditAttribution: Anonymous commentedthe 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.
Comment #46
catchDon'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.
Comment #47
Anonymous (not verified) CreditAttribution: Anonymous commentedyay 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 #48
catchComment 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.
Comment #49
Wim Leers#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.Comment #50
thedavidmeister CreditAttribution: thedavidmeister commentedMy 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!
Comment #51
Wim Leers#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.
Comment #52
thedavidmeister CreditAttribution: thedavidmeister commentedcould 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.
Comment #53
catchI 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.
Comment #54
thedavidmeister CreditAttribution: thedavidmeister commentedmake a (configurable?) limit that's smaller? how was the original threshold on what's "new" decided? is it holy?
Comment #55
thedavidmeister CreditAttribution: thedavidmeister commentedwhat 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.
Comment #56
thedavidmeister CreditAttribution: thedavidmeister commentedalso, (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.
Comment #57
Wim Leers#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.
Comment #58
thedavidmeister CreditAttribution: thedavidmeister commentedeven 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.
Comment #59
thedavidmeister CreditAttribution: thedavidmeister commentedalso, 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"?Comment #60
Wim Leers#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.
Comment #61
thedavidmeister CreditAttribution: thedavidmeister commentedThis 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.
Comment #62
thedavidmeister CreditAttribution: thedavidmeister commented#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.
Comment #63
andypostI 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"
Comment #64
msonnabaum CreditAttribution: msonnabaum commentedI 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.
Comment #65
thedavidmeister CreditAttribution: thedavidmeister commentedMmm, 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.
Comment #66
msonnabaum CreditAttribution: msonnabaum commented#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.
Comment #67
thedavidmeister CreditAttribution: thedavidmeister commentedok. There's a few things we can try to improve this after it gets in, I'm sure *something* will get done :)
Comment #68
Anonymous (not verified) CreditAttribution: Anonymous commentedok, sorry i got in the way here. let's proceed with #64.
Comment #68.0
Anonymous (not verified) CreditAttribution: Anonymous commentedUpdated issue summary.
Comment #69
Wim LeersSo we seem to have consensus to get this in. Yay!
Issue summary updated.
Who wants to RTBC?
Comment #70
amateescu CreditAttribution: amateescu commentedEverybody seems to be in agreement that this is a step forward, so rtbc.
Comment #71
catchI'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.
Comment #72
BerdirThis needs to use 'node/' . $node->id() instead, or it will explode when the BC decorator is removed.
Same here.
And another one ;)
Should this switch to use the service now?
I'm looking only at the added code, so no idea if this is moved or added, but it's missing a type.
Yeah :)
Why not use assertIdentical() ?
Comment #73
Wim LeersAll fixed, and found even more instances of
->nid
:)Comment #75
Wim Leers#2024963: Move baseFieldDefinitions from storage to entity classes just got committed, hence it no longer applies.
Comment #76
Wim LeersComment #77
BerdirInterdiff 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.
Comment #78
andypostempty 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
Comment #79
webchickThat sounds like a needs work.
Comment #80
Wim LeersGood catch, easy fix — it was indeed no longer necessary.
Thanks andypost!
Comment #82
Wim LeersThe 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 :)
Comment #83
webchickSorry, catch, your list is getting long. :(
Comment #84
catchWhy only up to 100?
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.
Comment #85
catchAlso opened #2078753: Add history_read_multiple().
Comment #86
catchAlso the module_exists() here should be converted per #2045931: Replace all module_exists() deprecated function calls..
Comment #87
Wim Leers#84:
If you look at the first 20 LoC or so of
comment-new-indicator.js
,node-new-comments-link.js
andtracker-history.js
(those are the 3 key files), they all look very similar. But that's precisely because I introducedhistory.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:
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.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.tracker-history.js
: given a table listing of nodes, showcomment-new-indicator.js
!)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 differentdata-
attributes are in completely different locations. Finally, the code for retrieving "x new replies" is indeed very similar to that innode-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.Given these answers, please let me know whether you still feel the tracker module changes should be removed from this patch or not.
Comment #88
catchI think we should split tracker out - and either have one or three issues to support tracker, forum and views tables (one might be OK?).
max_input_vars is in PHP 5.3.9 and defaults to 1,000 - I think it's fine to rely on that.
Comment #90
Wim LeersComment #92
Wim LeersChasing HEAD. (The commit of #2067017: Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants broke the patch.)
Comment #94
Wim LeersChasing HEAD. (I didn't catch all the s/
VERSION
/Drupal::VERSION
/ changes. Fixed.)Comment #96
Wim LeersShould be green.
Comment #98
Wim Leers#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.
Comment #99
Wim Leers#96: node_history_markers-1991684-96.patch queued for re-testing.
Comment #101
Wim LeersTest failures now reproduced locally. Will fix over the weekend.
Comment #102
Wim LeersIf this one doesn't come back green, I'm going to start throwing things.
Comment #104
Wim Leers#102: node_history_markers-1991684-102.patch queued for re-testing.
Comment #105
catchComment #106
alexpottPatch no longer applies.
Comment #107
alexpottAdding tags back in
Comment #108
Wim LeersChasing HEAD.
Comment #110
catchComment #111
Wim Leers#108: node_history_markers-1991684-108.patch queued for re-testing.
Comment #112
Wim LeersBack to RTBC as per #105. #108 was a straight reroll.
Comment #113
catchCommitted/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.
Comment #114
Wim LeersChange notice written: https://drupal.org/node/2086767.
Comment #115
moshe weitzman CreditAttribution: moshe weitzman commentedThanks for writing a change notice which will guide anyone else who wants to move user specific customizations from PHP to js. Well done.
Comment #116
Wim Leersdixon_ opened a follow-up to stop comment op links from breaking render cache: #2090783: Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user.
Comment #118
Wim LeersAdding "Performance" tag for tracking purposes.
Comment #118.0
Wim LeersUpdated issue summary.