Postponed on #1120928: Move "history" table into separate History module

Currently the node history table is only supporting the Node entity type although the table is defined in system module. I figure this is just a bug on the paradigm change since there was only one entity type in version < 7.x. The table definition should either belong in the Node module or support general entity types.

The table would be something like

$schema['history'] = array(
  'description' => 'A record of which {users} have read which entities.',
  'fields' => array(
    'uid' => array(
      'description' => 'The {users}.uid that read the entity id.',
      'type' => 'int',
      'not null' => TRUE,
      'default' => 0,
    ),
    'entity_type' => array(
      'type' => 'varchar',
      'length' => 32,
      'not null' => TRUE,
      'default' => ''
    ),
    'entity_id' => array(
      'description' => 'The id of the entity.',
      'type' => 'int',
      'not null' => TRUE,
      'default' => 0,
    ),
    'timestamp' => array(
      'description' => 'The Unix timestamp at which the read occurred.',
      'type' => 'int',
      'not null' => TRUE,
      'default' => 0,
    ),
  ),
  'primary key' => array('entity_type', 'uid', 'eid'),
  'indexes' => array(
    'eid' => array('eid'),
  ),
);

Follow-ups:
Update change notice http://drupal.org/node/1845806
File issue to extend views integration

Files: 
CommentFileSizeAuthor
#70 interdiff.txt2.69 KBandypost
#69 1029708-history-68.patch10.46 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,432 pass(es).
[ View ]
#52 interdiff.txt708 bytesandypost
#52 1029708-history-52.patch12.15 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,890 pass(es).
[ View ]
#51 1029708-history-50.patch12.09 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,296 pass(es).
[ View ]
#47 1029708-history-47.patch12.06 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1029708-history-47.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#29 1029708-interdiff-29.txt1.02 KBandypost
#29 1029708-history-29.patch11.44 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1029708-history-29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#26 1029708-interdiff-26.txt1002 bytesandypost
#26 1029708-history-26.patch11.53 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 52,193 pass(es).
[ View ]
#23 1029708-interdiff-23.txt3.29 KBandypost
#23 1029708-history-23.patch11.57 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 51,338 pass(es).
[ View ]
#21 1029708-interdiff-21.txt2.6 KBandypost
#21 1029708-history-21.patch10.79 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 51,372 pass(es).
[ View ]
#19 1029708-history-19.patch9.56 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 51,509 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#16 1029708-interdiff-16.txt901 bytesandypost
#16 1029708-history-16-nodep.patch10.65 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,946 pass(es), 25 fail(s), and 25 exception(s).
[ View ]
#16 1029708-history-16-dep.patch10.64 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,924 pass(es), 25 fail(s), and 25 exception(s).
[ View ]
#15 1029708-interdiff-15.txt3.23 KBandypost
#15 1029708-history-15.patch9.95 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 51,008 pass(es), 42 fail(s), and 25 exception(s).
[ View ]
#13 1029708-interdiff-13.txt1.06 KBandypost
#13 1029708-history-13.patch7.07 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 48,766 pass(es), 55 fail(s), and 28 exception(s).
[ View ]
#12 1029708-history.patch6.15 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 48,781 pass(es), 53 fail(s), and 29 exception(s).
[ View ]

Comments

I have made a proof of concept and posted in a forum post: http://drupal.org/node/1029862

Version:7.x-dev» 8.x-dev

This is a very logical cleanup. I'm moving this to D8 as it is a new feature. Once implemented then discussion of the possibility of a backport could happen.

Can we please make it even more usefull?

Why only "viewed" in the history table. I would like to provide "edited" informations as well. In combination with views. I could provide blocks with:

* new but unviewed nodes for current user
* last 10 viewed by current user
* last 10 edited by current user
* node X was were edited by these 10 users
* node X was viewed by these 10 users

This are usefull informations - at least I think so. A serialised data field would be nice as well. Could be used for nice informations - for example what an edit to a node changed in the node. Pretty advanced - but usefull.

Issue tags:+history

I guess that could make sense also. In versions < 7.x it was easy to find all recent updates since all this information could be found in a combination of node, comment and users table. Although, since version 7 you can not guarantee that a new custom entity type keeps track of changes.

Issue tags:+Entity system

Making this table handle edits is halfway toward an activity stream implementation and I'm not sure we want to go there. We have a few Contrib and they can each make sense depending on your use case. See http://groups.drupal.org/node/15207. The blocks described are already possible as we store last edit date in entity tables.

I think we should focus on View for now.

Status:Active» Postponed
Issue tags:-history

Actually, this issue is blocked on #1120928: Move "history" table into separate History module, which is the very very first and small step. (getting to know who has read what is not everyone's interest)

Status:Postponed» Active

There is no reason why this issue must wait on that issue. They are independent decisions.

I think this would be a lot more interesting if the history records were entities, like in Message.

But, why not reuse dblog or #1289536: Switch Watchdog to a PSR-3 logging framework to store this info?

This feature really useful but having just API for this is useless. This require some settings form at least

Status:Active» Needs review
StatusFileSize
new6.15 KB
FAILED: [[SimpleTest]]: [MySQL] 48,781 pass(es), 53 fail(s), and 29 exception(s).
[ View ]

Initial patch

StatusFileSize
new7.07 KB
FAILED: [[SimpleTest]]: [MySQL] 48,766 pass(es), 55 fail(s), and 28 exception(s).
[ View ]
new1.06 KB

fixed tests

Status:Needs review» Needs work

The last submitted patch, 1029708-history-13.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.95 KB
FAILED: [[SimpleTest]]: [MySQL] 51,008 pass(es), 42 fail(s), and 25 exception(s).
[ View ]
new3.23 KB

Fixed direct queries

StatusFileSize
new10.64 KB
FAILED: [[SimpleTest]]: [MySQL] 50,924 pass(es), 25 fail(s), and 25 exception(s).
[ View ]
new10.65 KB
FAILED: [[SimpleTest]]: [MySQL] 50,946 pass(es), 25 fail(s), and 25 exception(s).
[ View ]
new901 bytes

fixes fo forum. something wrong with upgrade path

Status:Needs review» Needs work

The last submitted patch, 1029708-history-16-dep.patch, failed testing.

Oh... all upgrade path tests are broken. All of them shows that history_update_8000() is not run according #1239370: Module updates of new modules in core are not executed

Status:Needs work» Needs review
StatusFileSize
new9.56 KB
FAILED: [[SimpleTest]]: [MySQL] 51,509 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Should be green. Suppose better move this update function to "head2head" a kind of module

EDIT Patch just removes history_update_8000()

Status:Needs review» Needs work

The last submitted patch, 1029708-history-19.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new10.79 KB
PASSED: [[SimpleTest]]: [MySQL] 51,372 pass(es).
[ View ]
new2.6 KB

So moving this to node.install right before history.module is enabled solves the last problem
Just needs to care about views handles which seems fine

+++ b/core/modules/history/history.moduleundefined
@@ -22,34 +22,43 @@
+function history_read($entity_id, $entity_type = 'node') {
...
+function history_write($entity_id, $entity_type = 'node', $account = NULL) {

I'm wondering whether we should switch the order of the parameters to be more consistent with all other parts in core?

+++ b/core/modules/history/history.moduleundefined
@@ -93,6 +104,7 @@ function history_user_cancel($edit, $account, $method) {
+        ->condition('entity_type', 'user')
@@ -104,5 +116,6 @@ function history_user_cancel($edit, $account, $method) {
+    ->condition('entity_type', 'user')

You want to remove both entries of other entity types and the actual entries of the user module.

+++ b/core/modules/history/history.views.incundefined
@@ -24,8 +24,9 @@ function history_views_data() {
+      'field' => 'entity_id',
...
+        array('field' => 'entity_type', 'value' => 'node'),

This looks great, though as we discussed in irc, it will be hard to integrate it dynamically for all entity types, as there isn't a created/changed column on all of them.

StatusFileSize
new11.57 KB
PASSED: [[SimpleTest]]: [MySQL] 51,338 pass(es).
[ View ]
new3.29 KB

Changed order of parameters.
Removed useless conditions.

Patch supposes that contrib module that wants to track "views of the entity" should care about insert/update/delete hooks same as views integration

+++ b/core/modules/history/history.installundefined
@@ -31,9 +38,16 @@ function history_schema() {
+      array('entity_type', 32),
...
+        array('entity_type', 32),

Is there a specific reason why we are doing that here?

+++ b/core/modules/history/history.moduleundefined
@@ -22,34 +22,43 @@
+    ))
+      ->fetchObject();

That code looks odd, though I'm not sure about the preferred codestyle.

array('entity_type', 32),
Is there a specific reason why we are doing that here?

Just for performance reasons. there could be a changes in size of field but index should stay short (same used in locale.module schema)

StatusFileSize
new11.53 KB
PASSED: [[SimpleTest]]: [MySQL] 52,193 pass(es).
[ View ]
new1002 bytes

#24.1 used in locale module to minimize index size
#24.2 changed to one-line as we have all over core (see interdiff)

+++ b/core/modules/history/history.install
@@ -13,17 +13,24 @@ function history_schema() {
+      'entity_id' => array(
...
+      'entity_type' => array(

This is sort of nit-picky, but I find it more intuitive to have the entity type come first and then the entity ID this also applies to some the queries in the other parts of the patch. I personally prefer "entity_type = 'node' AND entity_id = 1" over "entity_id = 1 AND entity_type = 'node'".

I was sort of expecting this patch to remove the dependency of history.module on node.module. Is that held up by anything else?

Otherwise looks great.

+++ b/core/modules/history/history.moduleundefined
@@ -22,34 +22,38 @@
+function history_read($entity_type, $entity_id) {

if we make this history_read($entity_id, $entity_type = 'node') this function remains backwards compatible for contrib. Thoughts?

+++ b/core/modules/history/history.moduleundefined
@@ -22,34 +22,38 @@
+function history_write($entity_type, $entity_id, $account = NULL) {

same here

+++ b/core/modules/history/history.views.incundefined
@@ -24,8 +24,9 @@ function history_views_data() {
+        array('field' => 'entity_type', 'value' => 'node'),

No views support for other entity types? Is there a follow up for that?

StatusFileSize
new11.44 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1029708-history-29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.02 KB

@larowlan History is a new module/API so there's no contrib that used it. Also I intentionally set entity_type first to point that entity_ID make no sense without its type and history_write() will fail anyway because $account would get an unpredictable value so BC is broken.
And if there's a some code that uses current API the breakage should cause fatal by getting int for string will point the error.

New patch addressed the #27

The follow-up for views could be done once patch RTBC

http://drupal.org/node/1845806 possibly should be updated after this patch commited

Issue summary:View changes

Updated issue summary.

Status:Needs review» Reviewed & tested by the community

Great job @andypost.

Nice patch. Unfortunately we're ridiculously over thresholds atm, but hopefully that will be rectified soonish.

Issue tags:-Entity system

#29: 1029708-history-29.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1029708-history-29.patch, failed testing.

Status:Needs work» Needs review

#29: 1029708-history-29.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1029708-history-29.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Entity system

#29: 1029708-history-29.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Edit: I didn't review this patch; just restoring the previous status since there was a testbot error on the retest.

#29: 1029708-history-29.patch queued for re-testing.

still green!

Given that we use 'history.uid', it may be more consistent to use 'history.eid' and 'history.type' (vs 'history.entity_id' and 'history.entity_type')? It's very minor, subject to debate, and shouldn't hold up this patch. :)

@Dries we have entiti_id and entity_type for all core tables so this notion is preserved here too

I agree with #41. 'uid' is simply an artefact of the old-style 'users.uid'. If we ever get to around to changing that, we should change this with it.
RTBC++

Category:feature» task

Issue tags:-Entity system

#29: 1029708-history-29.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Entity system

The last submitted patch, 1029708-history-29.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new12.06 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1029708-history-47.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Merge for current HEAD

Component:base system» history.module

Issue tags:-Entity system

#47: 1029708-history-47.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Entity system

The last submitted patch, 1029708-history-47.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new12.09 KB
PASSED: [[SimpleTest]]: [MySQL] 57,296 pass(es).
[ View ]

Chasing head

StatusFileSize
new12.15 KB
PASSED: [[SimpleTest]]: [MySQL] 57,890 pass(es).
[ View ]
new708 bytes

fix doc-block

StatusFileSize
new12.35 KB
PASSED: [[SimpleTest]]: [MySQL] 57,769 pass(es).
[ View ]
new752 bytes

and bit more

If we're touching history, turn it into a proper service and mark the functions @deprecated, just wrapping the service.

Awesome idea! so planing to change the implementation in favour of service.

The only questionable part is now to inject history service in _entity_view in 'full' view mode.

Drupal\Core\Entity\Controller\EntityViewController implements ControllerInterface already, so we can just add to the ::create and _construct methods to include the service definition.

I've lost track of all of the magic controllers in Entity API. Is that a for-reals Controller (as in, thing called by the kernel/router) or something else? Because IMO history tracking does not belong in the entity system at all; it's something that should be in the for-reals Controller, or in a hook/event called by it. Not in the rendering system anywhere.

its a for-real kernel/routing controller

So what is a recommended implementation?
service or conroller?

StatusFileSize
new12.5 KB
PASSED: [[SimpleTest]]: [MySQL] 58,444 pass(es).
[ View ]

Merge HEAD, I think better to wrap history to service (HistoryManager) and mark current read&write as deprecated anyway we need change API in #2078753: Add history_read_multiple()

StatusFileSize
new12.27 KB
FAILED: [[SimpleTest]]: [MySQL] 58,937 pass(es), 24 fail(s), and 7 exception(s).
[ View ]

re-rolled

Status:Needs review» Needs work

The last submitted patch, 1029708-history-63.patch, failed testing.

History manager is mostly ready in #2081585: Introduce HistoryRepository service
Should we close this one in favor of it? or just move back here when its ready?

Issue summary:View changes

Updated issue summary.

Issue summary:View changes
Status:Needs work» Needs review
Related issues:+#2081585: Introduce HistoryRepository service
StatusFileSize
new10.02 KB
PASSED: [[SimpleTest]]: [MySQL] 59,398 pass(es).
[ View ]

Patch just updates schema, so history API is not changed, planing to fix usage in #2081585: Introduce HistoryRepository service

Status:Needs review» Needs work
  1. +++ b/core/modules/history/history.install
    @@ -13,13 +13,20 @@ function history_schema() {
    +        'description' => 'The entity_type of the entity was read.',

    s/entity_type/type/
    s/entity was/entity that was/

  2. +++ b/core/modules/history/history.install
    @@ -13,13 +13,20 @@ function history_schema() {
    +        'description' => 'The entity_id that was read.',

    The ID of the entity that was read.

  3. +++ b/core/modules/node/node.install
    @@ -720,9 +720,37 @@ function node_update_8011() {
    +  // Drop all keys to properly rename constraints.
    +  db_drop_primary_key('history');
    +  db_drop_index('history', 'nid');
    +  // Add new column.
    +  db_add_field('history', 'entity_type', array(
    +    'type' => 'varchar',
    +    'not null' => TRUE,
    +    'default' => 'node',
    +    'length' => 255,
    +    'description' => 'The entity_type of the entity was read.',
    +  ));
    +  db_change_field('history', 'nid', 'entity_id', array(
    +    'description' => 'The entity_id that was read.',
    +    'type' => 'int',
    +    'not null' => TRUE,
    +    'default' => 0,
    +  ));
    +  // Create new indexes.
    +  db_add_primary_key('history', array(
    +    'uid',
    +    'entity_id',
    +    array('entity_type', 32),
    +  ));
    +  db_add_index('history', 'history_entity', array(
    +    'entity_id',
    +    array('entity_type', 32),
    +  ));

    Is this even necessary, now that we're dropping hook_update_N() in favor of Migrate in core?

  4. +++ b/core/modules/history/history.install
    @@ -13,13 +13,20 @@ function history_schema() {
    +        'description' => 'The {users}.uid that read the entity_id.',

    s/read the entity_id/read the {history}.entity_id entity/.

  5. +++ b/core/modules/history/history.install
    @@ -13,13 +13,20 @@ function history_schema() {
    +        'default' => 'node',

    I don't think this should have a default value?

Status:Needs review» Needs work

d.o--

Status:Needs work» Needs review
StatusFileSize
new10.46 KB
PASSED: [[SimpleTest]]: [MySQL] 59,432 pass(es).
[ View ]

Fixed #67 and removed default values for entity_type & entity_id

Added @todo about migrate, I think we still need this upgrade path while migrate in progress

PS: also going to re-roll #2081585: Introduce HistoryRepository service with the same changes

StatusFileSize
new2.69 KB

interdiff

Suppose we could close this one as duplicate of #2081585: Introduce HistoryRepository service

Status:Needs review» Closed (duplicate)

Duplicate of #2081585: Introduce HistoryRepository service which is green again