Problem

  • The "history" information, which stores who has read what and when, supports nodes only.
  • The code is buried into Node module, and no one cared for it in the last 6-8 years.
  • The functionality can be needless/superfluous for many sites and use-cases (for example, single user sites, etc).
  • There might very well be better, alternative architectural solutions for the functionality, but Drupal hard-codes this single one into Node module.

Goal

  • Move the "history" functionality into an own module.

Todo

  1. Move {history} schema definition from system.install to node.install
  2. Find a proper name for the new module (history.module would mean something else)
  3. Move the functionality into a new module
  4. Make it work for all entities: #1029708: History table for any entity
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Tempted to mark this RTBC as a first step, but ultimately, we need to remove the limitation on nodes and make it work for all entities -- which in turn might mean that we might move it back to system module (unless there is a history.module or entity.module).

sun’s picture

catch’s picture

I'd be fine with moving it to node, then eventually entity.module #1018602: Move entity system to a module.

catch’s picture

Or a history module is interesting, not all sites need this kind of history tracking.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Alright, let's do a history.module in a second step. Perfectly in line with our framework campaign. :)

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Committed to 7.x and 8.x. Moving back to 'needs work' so we can work on phase 2 of this patch?

sun’s picture

Component: system.module » node.module
Status: Needs work » Needs review
FileSize
14.81 KB

*SCNR*

sun’s picture

FileSize
15.09 KB

Thanks to Berdir, this one includes the (final?) hack to enable the module in an update without re-installing the schema.

sun’s picture

Assigned: Unassigned » sun

Status: Needs review » Needs work

The last submitted patch, drupal8.history.8.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
15.1 KB

umph, stupid me.

andypost’s picture

@sun I think it's better to have one table per entity type otoh we get a terrible bottle neck.

Forum module should just depends on history. Is there any roadmap about history.module API? Probably it's just a kind of field

sun’s picture

I disagree. But since you didn't provide evidence for your opinion, I won't do either.

Anyway, the remaining goal of this issue and patch is to move the existing functionality into an own module. Yes, the patch isn't moving 1:1 and already contains a start of an abstraction, but those changes are unimportant and going to be required either way in the end, regardless of how the future will look like.

In other words, concrete proposals and ideas for how to fundamentally change or improve History module should happen in separate follow-up issues. Let's make progress.

catch’s picture

There are good reasons to have separate tables per-entity, and I'm not convinced that should be a separate discussion to the original patch. The only place we currently have shared tables between entities for data is field tables - which for a start are split per-field, and are also pluggable. So a single table for all entities is introducing a completely new pattern to core.

Reasons we should probably avoid a single table:
- especially with history, the potential for a large number of records is extremely high (entities * users).
- The table gets a merge query every time any user views a node currently, the more rows are in there, the slower this will be.
- by adding entity_type here, we have to index on varchar + int instead of just int. varchar indexes are less efficient for inserts/updates/deletes - and there are lots of these on {history}. I don't know exactly how much of a performance issue that is (and a google search for benchmarks came up short), but I'd prefer to know before we introduce this kind of pattern instead of afterwards when it's always much harder to change - especially for volatile data that's not tied to entity crud.

sun’s picture

@catch: I understand. However,

  1. as of now, there is no concept, nor an API, in Drupal/core for dynamically creating and deleting database tables based on the appearance or disappearance of entity types yet. Field API implements the only remotely comparable functions and procedures, but those are restricted to field storage tables, and even more importantly, they are able to rely on a guaranteed trigger. Unlike fields, entity types are stateless - they are merely collected in an info hook and not tracked in any way; they can appear and vanish without notice.

    Figuring out how to introduce reliable states for entity types (or collated info in general?), and potentially rewriting field storage API to be not bound to fields and work in general, will require lots of time and major architectural changes.

  2. detaching hidden, built-in functionality from Node module into separate modules will be a huge help to make substantial progress on standardizing core entities and deleting lots of custom code. I could not resist to do the patch because it is extremely low-hanging fruit for this higher level goal. History module itself is not my actual interest.

    If you'd be more comfortable if we'd remove the added 'entity_type' column and just simply keep and move the current functionality (limited to nodes), then I'd be happy to revert those changes.

sun’s picture

FileSize
13.13 KB

Alright - @catch and me discussed in IRC and agreed on to just simply move the existing functionality without further changes for now.

Reason for that being:

  1. Ideally, we'd use separate tables per entity type.
  2. We can't do separate tables for entity types yet; requires some really big impact changes for configuration/info/schema management for Drupal core, which we'll try to work on anyway.
  3. We don't want to introduce a possible performance problem.
  4. Merely moving the functionality into an own module will raise awareness big time that we have some code here that's in core and hasn't been touched for ages. There might even be more intelligent approaches for tracking who's read what nowadays, but no one notices and cares, because this functionality is deeply hidden in Node module currently.
sun’s picture

FileSize
13.22 KB

Fixed a small oversight in Forum's unread query.

podarok’s picture

subscribe

catch’s picture

Category: bug » task

I should have posted this as a task.

mirocow’s picture

subscribe

quicksketch’s picture

Title: {history} table is owned by system module » Move "history" table into separate History module

Updating title to reflect the current task. The "history" table was moved to be owned by node module in #6. I'd love to see this included as I know very few sites that actually require this information. Reducing the query count (and an INSERT at that) by at least 1 on every page view of the site seems irresistible.

sun’s picture

+++ b/modules/forum/forum.module
@@ -838,18 +838,22 @@ function forum_forum_load($tid = NULL) {
+ * Calculate the number of nodes the user has not yet read and are newer than HISTORY_READ_LIMIT.
+ *
+ * @todo Might be a bit stupid to run this query without History module.
  */
 function _forum_topics_unread($term, $uid) {

This seems to be only remaining required @todo.

+++ b/modules/history/history.module
@@ -0,0 +1,77 @@
+ * @todo
+ * - Generic helper for _forum_user_last_visit() + node_last_viewed().
+ * - Generic helper for node_mark().

...whereas these ones may be moved to follow-up patches/issues.

1 days to next Drupal core point release.

joachim’s picture

Not that I want to start a bikeshed, but is 'history' the best name for this?

> We can't do separate tables for entity types yet; requires some really big impact changes for configuration/info/schema management for Drupal core, which we'll try to work on anyway.

That pattern will be needed for entity revision history too -- the entity schema should magically figure out to make a revisions table.

catch’s picture

I think forum module could depend on history module, new and updated topics is pretty fundamental to forums.

joachim is right that 'history' might not be the best name - at least not for the human readable name. Something like 'read tracking' or similar. Part of me wants to say this is close to statistics module, but another part is shouting "Nnoooo" at the thought of it.

joachim’s picture

> Part of me wants to say this is close to statistics module, but another part is shouting "Nnoooo" at the thought of it.

Same here.

It seems to me it's also a very incomplete user activity log -- it only records reads, not creation or editing.

xjm’s picture

Tagging issues not yet using summary template.

sun’s picture

Wrote a proper issue summary.

xjm’s picture

Alright, so the next step in sun's summary is just to bikeshed a name.

I'll start!

read_tracker?
content_view_tracker?

Hum. Both these risk confusion with tracker.module.

Actually, read counts kinda are the logical counterpart to the tracker.

view_counter?
read_counter?

What's our scope here for putting "other stuff" in there? Entity views? If so should it:

  1. Be entity_something.module?
  2. Be in entity.module?

entity_view_counter? entity_read_counter? entity_counter? entity_statistics? (risking confusion but related functionality again)

quicksketch’s picture

I think forum module could depend on history module, new and updated topics is pretty fundamental to forums.

As another completely different idea, what about the possibility of killing "History" module in core entirely, and bundling this "new post" functionality into Forum module exclusively? It'd be an intentional narrowing of functionality in Drupal to the situation that is most likely to need it, then kick generic stats/history tracking to contrib. Then it would remove the need to explain to users what the heck "History" (or "View Counter" or "Read Counter") do, while giving all other sites that don't need this functionality at all a performance boost rather than having it baked-in like it is now.

Michelle’s picture

If you do that, then how are we to know if there is a new response on an issue? Or a group discussion? Etc... This really isn't a forum-specific functionality. Pretty much anyplace that has commenting needs a way for people to know if there is a new comment.

Michelle

mototribe’s picture

I agree with Michelle that most sites with commenting need a way to see if there are new comments.
It wouldn't matter to me if it's out of core and in a contrib module. It might evolve better as a contrib module.

For example I would like to have a "new comment" indicator (similar to Facebook or Stackoverflow) on top of my site that tells me the total number of new comments that I have. For this to be performant it will probably need another table to keep track of the counts by user. This could be a feature of the new "View counter" module - or whatever the name will be ...

Michelle’s picture

My only objection to moving this to contrib is that it's pretty basic functionality. I can just imagine people saying, "You mean I have to download another module just to see if there's anything new on my site?" Having something stripped down and totally basic in core that can be enhanced in contrib is fine... Totally taking it out of core? I'm not so sure about that.

Michelle

sun’s picture

Status: Needs review » Needs work

The last submitted patch, patform.history.33.patch, failed testing.

andypost’s picture

Suppose this will require to add entity_type key for history table

tim.plunkett’s picture

Priority: Major » Normal

Nothing in this issue summary makes it seem major.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
13.85 KB

Added history as dependency of forum module...
i think it makes sense as #24

Status: Needs review » Needs work

The last submitted patch, 1120928-move_history_to_its_own_module-37.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
14.28 KB

It should pass testes without the dependency now

Status: Needs review » Needs work

The last submitted patch, 1120928-move_history_to_its_own_module-39.patch, failed testing.

andypost’s picture

+++ b/core/modules/forum/forum.module
@@ -1319,10 +1319,10 @@ function _forum_user_last_visit($nid) {
-  return isset($history[$nid]) ? $history[$nid] : NODE_NEW_LIMIT;
+  return isset($history[$nid]) ? $history[$nid] : 0;

Any reason to change this to 0?

+++ b/core/modules/history/history.api.php
@@ -0,0 +1,6 @@
+ * @file
+ * API documentation for History module.
+ */

API should describe it's hook

+++ b/core/modules/history/history.info
@@ -0,0 +1,6 @@
+name = History
+description = Records which user has read which content.
+package = Core
+version = VERSION
+core = 8.x
+dependencies[] = node

Probably node.module should depend on history in this case

+++ b/core/modules/node/node.module
@@ -1297,7 +1260,7 @@ function node_show(Node $node, $message = FALSE) {
-  node_tag_new($node);
+  module_invoke('history', 'update', $node->nid);
 

Hook should be described in history.api.php

sun’s picture

Assigned: sun » Unassigned

Thanks for moving this forward! :)

I think we can and should try to make the tests pass, but we also still have to find a proper name for the new module. "History" would indeed be misleading.

Out of the existing suggestions above, "Activity" would indeed come closest (even though it's only a bare minimum activity tracker), but choosing that name would make the activity module maintainers very unhappy, I guess ;)

To re-iterate over the existing suggestions:

- #24: read tracking
- #25: a very incomplete user activity log -- it only records reads, not creation or editing
- #28: read_tracker, content_view_tracker, view_counter, read_counter, entity_counter

Temporarily unassigning myself, since you are working on this currently.

mdupont’s picture

Assigned: Unassigned » sun

(Bikeshedding on the name)
If the module is to log access to entities, why not name it "entity_access_log" or plain "access_log"? Similar but not quite the same than statistics.

mdupont’s picture

Assigned: sun » Unassigned

Crossposting.

ParisLiakos’s picture

thanks andypost for the review!

+name = History
+description = Records which user has read which content.
+package = Core
+version = VERSION
+core = 8.x
+dependencies[] = node

Probably node.module should depend on history in this case

I dont get it, why?

And one more..If history does not fit us and we are gonna change it, should i change tables name as well?
It would make sense i guess.

Access log, is misleading, one could think that it logs lets say when someone logged in or out.
I would go with view_tracker

And patch @39 is green for me in local

dagmar’s picture

Status: Needs work » Needs review
FileSize
19.63 KB

Re-rolled, moved views integration into history module. Node, Comment, Tracker and Forum tests pass locally.

Status: Needs review » Needs work

The last submitted patch, 1120928-move_history_to_its_own_module-46.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentCSSTest.php
 class CommentCSSTest extends CommentTestBase {
...
+  public static $modules = array('comment', 'node', 'history');

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentLinksTest.php
+  public static $modules = array('comment', 'node', 'history');

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentNewIndicatorTest.php
+  public static $modules = array('comment', 'node', 'history');

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTokenReplaceTest.php
+  public static $modules = array('comment', 'node', 'history');

CommentTestBase specifies node and comment already, they do not have to be re-specified again, since $modules is collected from all classes.

+++ b/core/modules/forum/forum.info
@@ -1,6 +1,7 @@
+dependencies[] = history

+++ b/core/modules/forum/forum.module
@@ -884,13 +884,16 @@ function forum_forum_load($tid = NULL) {
 function _forum_topics_unread($term, $uid) {
+  if (!module_exists('history')) {
+    return 0;
+  }

@@ -1311,19 +1314,19 @@ function template_preprocess_forum_submitted(&$variables) {
 function _forum_user_last_visit($nid) {
...
+  if (empty($history) && module_exists('history')) {

The module_exists() is unnecessary, since the .info file defines a dependency on history module already.

Adding that dependency to Forum module makes sense, I think, so let's just remove the module_exists() conditions.

+++ b/core/modules/node/node.install
@@ -712,6 +682,22 @@ function node_update_8008() {
+function node_update_8009() {
+  // Create a fake entry in the installed modules list to not re-install the
+  // {history} schema.
+  $schema_store = drupal_container()->get('keyvalue')->get('system.schema');
+  $module_config = config('system.module');
+  $module_config
+          ->set("enabled.history", 0)
+          ->set('enabled', module_config_sort($module_config->get('enabled')))
+          ->save();
+  module_enable(array('history'));
+}

This should use update_module_enable().

+++ b/core/profiles/standard/standard.info
@@ -3,6 +3,7 @@ description = Install with commonly used features pre-configured.
+dependencies[] = history

Not sure whether we want to enable history module for standard profile...

dagmar’s picture

Fixed the remaining tests from #46

dagmar’s picture

Applied changes suggested in #49.

Regarding

Not sure whether we want to enable history module for standard profile...

I think it makes sense, since it was included in the node module in drupal 7, so it would be a regression not have it in drupal 8 by default in the standard profile.

cweagans’s picture

I'm not sure that I would consider that a regression, but I think that if we're not going to include it in the Standard profile, then we should just go ahead and remove it altogether.

sun’s picture

@cweagans: Nah, it is still a dependency for Forum module, and for forums, that's actually critical functionality. (Likewise for tracker[s], just like our issue trackers on d.o.) Removing the functionality was never on the table to begin with.

@dagmar:
Great to see that the tests are passing now! Awesome job! :)

Looks like we're primarily blocked on the module name now... :-/ Existing suggestions thus far, see #42

Out of those choices, I'd think that the following would make most sense:

1) entity_tracker
2) read_tracker
3) view_tracker

Ordered by (my) preference. Some reasoning:

- It's not about "counts" and no count appears anywhere. Only a "new" marker happens to appear. But both "marker" and "new" are irrelevant in terms of functionality/module-naming, too.

- "read" and "view" borderline contain too much technical implementation details; it shouldn't matter how exactly the read history is tracked.

- "entity" or alternatively "content" would definitely be good to have, since a follow-up step would be to decouple this code from Node module and make it available to all content entities.

- This functionality is usually called "Mark as read" in the outside world; e.g., e-mail clients and so on.

dagmar’s picture

I don't really like entity_tracker, because we already have another module called tracker, so...

I was thinking in one of this names:

  • New content
  • Not viewed content
  • Not readed yet

Also remember that the description of history says: "Records which user has read which content."

sun’s picture

Silly question: Can we just simply go with History for now?

Decoupling this functionality from Node module was and still is the primary objective. The "name" of this functionality is the least of all problems we have with it, really. This naming discussion has a relatively big touch of misguided perfectionism. It can be happily and very easily renamed later. There's no reason for blocking this issue on that.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Well, if "we're primarily blocked on the module name now." and "Can we just simply go with History for now?" and the green, clean patch calls it history then it's a go.

Lars Toomre’s picture

As a follow-up issue, shouldn't we convert HISTORY_READ_LIMIT to be set from a config file?

larowlan’s picture

could really use this in #731724: Convert comment settings into a field to make them work with CMI and non-node entities (which needs reviews) particularly if we made it entity_type generic.

catch’s picture

Status: Reviewed & tested by the community » Needs work

node_last_viewed() only makes sense with the history module installed, can't we move that to history module?

dagmar’s picture

Status: Needs work » Needs review
FileSize
21.78 KB

Re-rolled. Moved node_last_viewed() to history_node_last_viewed() in the history module.

Status: Needs review » Needs work

The last submitted patch, 1120928-move_history_to_its_own_module-59.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
FileSize
398 bytes
21.8 KB

Status: Needs review » Needs work

The last submitted patch, 1120928-move_history_to_its_own_module-61.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
FileSize
442 bytes
21.8 KB
sun’s picture

FWIW, in #1835496-4: Factor {users_field_data}.access out of the table and entity caches, the idea was mentioned to perhaps move {users}.access + possibly even {users}.login into a generic user activity tracking module, decoupling that entire stuff from User module.

That shouldn't hold up this issue IMHO, just mentioning a potential (in no way agreed-on) path forward. TBD over there.

Reviewed this patch once more — final issues:

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTokenReplaceTest.php
@@ -11,6 +11,7 @@
 class CommentTokenReplaceTest extends CommentTestBase {
+

Let's revert this white-space change — such unintended changes are guaranteed to always cause re-rolls somewhere else. ;)

+++ b/core/modules/history/history.module
@@ -0,0 +1,99 @@
+/**
+ * Updates 'last viewed' timestamp of the specified entity for the current user.
...
+function history_update($nid, $account = NULL) {
...
+/**
+ * Retrieves the timestamp for the current user's last view of a specified node.
...
+function history_node_last_viewed($nid) {

+++ b/core/modules/node/node.module
@@ -1111,7 +1054,9 @@ function node_show(Node $node, $message = FALSE) {
   // Update the history table, stating that this user viewed this node.
-  node_tag_new($node);
+  if (module_exists('history')) {
+    history_update($node->nid);
+  }

Would it make sense to rename these two to:

history_write()
history_read()

?

+++ b/core/modules/node/node.module
@@ -356,16 +298,17 @@ function node_mark($nid, $timestamp) {
+  $history_module_enabled = module_exists('history');
+  if (!$user->uid || !$history_module_enabled) {
     return MARK_READ;
   }
...
+  if (!isset($cache[$nid]) && $history_module_enabled) {

The added $history_module_enabled condition in the second if is unnecessary, since the code path is never reached.

This also means we can eliminate the variable and inline the module_exists() into the first if condition.

+++ b/core/modules/node/node.module
@@ -1519,10 +1460,6 @@ function node_user_predelete($account) {
-  // Clean history.
-  db_delete('history')
-    ->condition('uid', $account->uid)
-    ->execute();

We're missing a history_user_delete() implementation.

mitchell’s picture

Re #42 / #25 (activity log): There's Message too, and on a related subject: #1836598: Convert dblog record[-s/-ing] to Entity API.

dagmar’s picture

Addressed issues from #64

sun’s picture

Status: Needs review » Reviewed & tested by the community

Awesome. I think this looks excellent. I like how history_read() actually maps to the HISTORY_READ_LIMIT constant now.

@dagmar: If this won't be committed upfront, it would be great to perform a final sanity tweak: Move the history_read() function up in history.module so it is grouped with history_write() (I'd expect _read to come before _write).

dagmar’s picture

Moved history_read at the beginning of history.module.

Tagging, we should document this minor API change.

sun’s picture

Thank you! :)

dagmar’s picture

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

catch’s picture

Title: Move "history" table into separate History module » Change notice: Move "history" table into separate History module
Priority: Normal » Critical
Status: Fixed » Active

Needs a change notice.

Also, nice to see this one in :)

sun’s picture

Title: Change notice: Move "history" table into separate History module » Move "history" table into separate History module
Priority: Critical » Normal
Status: Active » Fixed

Change notice: http://drupal.org/node/1845806

We should still create a follow-up issue to find a better name than history.module though. Ideally, summarizing all existing suggestions from this issue in the issue summary. Any takers?

Status: Fixed » Closed (fixed)

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

firebird’s picture

Version: 8.x-dev » 7.x-dev
Status: Closed (fixed) » Active

Is this going to be backported to D7?

catch’s picture

Version: 7.x-dev » 8.x-dev
Status: Active » Closed (fixed)

No it's too much of an API change to backport.

catch’s picture

Issue summary: View changes

Updated issue summary.