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

Files: 
CommentFileSizeAuthor
#22 interdiff.txt788 bytesandypost
#22 drupal8.history-module.2078753-22.patch2.81 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,880 pass(es).
[ View ]
#17 interdiff.txt872 bytesandypost
#17 drupal8.history-module.2078753-17.patch2.86 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,469 pass(es).
[ View ]
#14 interdiff.txt1.72 KBandypost
#14 drupal8.history-module.2078753-14.patch2.84 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,211 pass(es).
[ View ]
#11 interdiff.txt586 bytesandypost
#11 drupal8.history-module.2078753-11.patch1.8 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 59,214 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#9 interdiff.txt1019 bytesandypost
#9 drupal8.history-module.2078753-9.patch1.8 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 59,222 pass(es), 4 fail(s), and 8 exception(s).
[ View ]
#4 interdiff.txt435 bytesandypost
#4 drupal8.history-module.2078753-4.patch1.46 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,229 pass(es).
[ View ]
#2 drupal8.history-module.2078753-2.patch1.39 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,046 pass(es).
[ View ]
#1 drupal8.history-module.2078753-1.patch1.35 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,449 pass(es).
[ View ]

Comments

Component:base system» history.module
Status:Active» Needs review
Issue tags:+API addition
StatusFileSize
new1.35 KB
PASSED: [[SimpleTest]]: [MySQL] 58,449 pass(es).
[ View ]

While on that, here's simple patch

PS: Also I'm trying to implement manager #2081585: Introduce HistoryRepository service

StatusFileSize
new1.39 KB
PASSED: [[SimpleTest]]: [MySQL] 58,046 pass(es).
[ View ]

Fixed doc-block comment

+++ b/core/modules/history/history.module
@@ -41,6 +41,48 @@ function history_read($nid) {
+  $nodes_to_read = array();
+  foreach ($nids as $nid) {
+    if (isset($history[$nid])) {
+      $return[$nid] = $history[$nid];
+    }
+    else {
+      $nodes_to_read[$nid] = 0;
+    }
+  }
+
+  if (empty($nodes_to_read)) {
+    return $return;
+  }

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

StatusFileSize
new1.46 KB
PASSED: [[SimpleTest]]: [MySQL] 59,229 pass(es).
[ View ]
new435 bytes

The 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";'

Ahh, I totally missed that you're filling two different arrays, there. $return != $nodes_to_read. I get it now.

Forgot to add: Code looks quite good. I don't feel comfortable RTBCing this myself, however.

Status:Needs review» Needs work

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

+++ b/core/modules/history/history.module
@@ -41,6 +41,49 @@ function history_read($nid) {
+      // Initialize value if current user has not been viewed the node.

s/has not been viewed/has not viewed/

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

Status:Needs work» Needs review
StatusFileSize
new1.8 KB
FAILED: [[SimpleTest]]: [MySQL] 59,222 pass(es), 4 fail(s), and 8 exception(s).
[ View ]
new1019 bytes

I 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

Status:Needs review» Needs work

The last submitted patch, drupal8.history-module.2078753-9.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.8 KB
FAILED: [[SimpleTest]]: [MySQL] 59,214 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new586 bytes

This allows to test new function! #9 shows that we have test coverage

Status:Needs review» Needs work

The last submitted patch, drupal8.history-module.2078753-11.patch, failed testing.

Looks great, thanks! But for the test fails, this looks ready to go.

Status:Needs work» Needs review
StatusFileSize
new2.84 KB
PASSED: [[SimpleTest]]: [MySQL] 59,211 pass(es).
[ View ]
new1.72 KB

Fix test failures (json need int value)
Used new function in controller

Just some minor nitpicks

  1. +++ b/core/modules/history/history.module
    @@ -30,14 +30,51 @@
    + * Retrieves the timestamp for the current user's last view of a nodes.

    'Retrieves the last viewed timestamp for each of the passed node IDs.' perhaps?

  2. +++ b/core/modules/history/history.module
    @@ -30,14 +30,51 @@
    + *   A node IDs.

    An array of node IDs

  3. +++ b/core/modules/history/history.module
    @@ -30,14 +30,51 @@
    +    $nodes_to_read[$row->nid] = (int)$row->timestamp;

    (int) $row->timestamp = needs a space after the cast

  1. +++ b/core/modules/history/history.module
    @@ -100,7 +101,10 @@ function history_write($nid, $account = NULL) {
    +    // Update static cache.
    +    $history = &drupal_static('history_read', array());
    +    $history[$nid] = REQUEST_TIME;

    Ohh, that's very savvy. Nice!

I'd re-roll for #15 but then I can't RTBC :-P

StatusFileSize
new2.86 KB
PASSED: [[SimpleTest]]: [MySQL] 59,469 pass(es).
[ View ]
new872 bytes

Status:Needs review» Reviewed & tested by the community

Looks good. I don't have anything to complain about.

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review

Suppose 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 function

I don't think we need a BC layer for that.

StatusFileSize
new2.81 KB
PASSED: [[SimpleTest]]: [MySQL] 58,880 pass(es).
[ View ]
new788 bytes

So here's suggested __FUNCTION__

Status:Needs review» Reviewed & tested by the community

Thanks!

Nicely done — HistoryController now looks much better :)

Title:history_read() should take an array of node IDsChange notice: Add history_read_multiple()
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committed/pushed to 8.x, thanks!

Could use a very short change notice for the API addition.

Status:Active» Needs review

Title:Change notice: Add history_read_multiple()Add history_read_multiple()
Status:Needs review» Fixed

Issue tags:-Needs change record

fix tags

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