Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
#1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user moves history fetching from being done once for each node in a teaser, to all at once (but currently in a separate HTTP request), this means looping over history_read() to get the timestamp for each nid.
We could take an array of nids and return just those with a timestamp instead to bring that down to one query.
Comment | File | Size | Author |
---|---|---|---|
#22 | interdiff.txt | 788 bytes | andypost |
#22 | drupal8.history-module.2078753-22.patch | 2.81 KB | andypost |
#17 | interdiff.txt | 872 bytes | andypost |
#17 | drupal8.history-module.2078753-17.patch | 2.86 KB | andypost |
#14 | interdiff.txt | 1.72 KB | andypost |
Comments
Comment #1
andypostWhile on that, here's simple patch
PS: Also I'm trying to implement manager #2081585: [META] Modernize the History module
Comment #2
andypostFixed doc-block comment
Comment #3
tstoecklerI think using array_diff_key() here would make this much more readable. Also, from reading the code it seems because of the " = 0" the empty() check will never return TRUE. I might be mistaken, though.
Comment #4
andypostThe reason to use loop is to initialize array with 0 for nodes that are not been viewed by user so added comment.
So the static cache is filled with a proper return value.
Check for array emptiness could be done without
empty()
but this more readable.php -r '$a = array(1 => 0); if (empty($a)) print "empty"; else print "not empty";'
Comment #5
tstoecklerAhh, I totally missed that you're filling two different arrays, there. $return != $nodes_to_read. I get it now.
Comment #6
tstoecklerForgot to add: Code looks quite good. I don't feel comfortable RTBCing this myself, however.
Comment #7
Wim LeersI doubt having a static cache is valuable in this case. It's extremely unlikely this is being called many times. We only use static caches for frequently called functions.
s/has not been viewed/has not viewed/
Comment #8
tstoecklerRe #7: This code is only re-using the static from history_read(). That way history_read_multiple() doesn't re-fetch items that history_read() already fetched. Thinking about this I think that part could be streamlined by history_read_multiple() maintaining its own static, and history_read() simply being a one-line call to history_read_multiple(array($nid));. That would be sort of in-line with entity_load() and so forth. Also, I think we should definitely use a drupal_static() here to avoid double DB queries. Even if the chances are small. In this particular case a drupal_static() doesn't hurt, and there are efforts to move this to a HistoryManager anyway, which would store this properly in the object state.
Don't know if you guys agree with my proposal but leaving needs work for the second item in #7 at lesat.
Comment #9
andypostI prefer to preserve the name of static cache as 'history_read' because there's could be a code that uses it.
And this static cache should be shared by both functions, same as #8 said
New patch incorporated suggested changes
Comment #11
andypostThis allows to test new function! #9 shows that we have test coverage
Comment #13
tstoecklerLooks great, thanks! But for the test fails, this looks ready to go.
Comment #14
andypostFix test failures (json need int value)
Used new function in controller
Comment #15
larowlanJust some minor nitpicks
'Retrieves the last viewed timestamp for each of the passed node IDs.' perhaps?
An array of node IDs
(int) $row->timestamp = needs a space after the cast
Comment #16
tstoecklerOhh, that's very savvy. Nice!
I'd re-roll for #15 but then I can't RTBC :-P
Comment #17
andypostComment #18
tstoecklerLooks good. I don't have anything to complain about.
Comment #19
catchLooks good except I don't understand why we don't use __FUNCTION__ for the static naming. While that's a tiny API change I'd be very surprised if there's a module fiddling with that static.
Comment #20
andypostSuppose there's no modules dealing with this static but I preserved
history_read
name for BC.__FUNCTION__
should not used because logic for static cache moved to new functionComment #21
catchI don't think we need a BC layer for that.
Comment #22
andypostSo here's suggested
__FUNCTION__
Comment #23
catchThanks!
Comment #24
Wim LeersNicely done —
HistoryController
now looks much better :)Comment #25
catchCommitted/pushed to 8.x, thanks!
Could use a very short change notice for the API addition.
Comment #26
andypostAdded https://drupal.org/node/2094029
Comment #27
catchComment #28
andypostfix tags