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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

oddbit’s picture

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

rickvug’s picture

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.

ro-no-lo’s picture

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.

oddbit’s picture

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.

sun’s picture

Issue tags: +Entity system
moshe weitzman’s picture

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.

sun’s picture

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)

moshe weitzman’s picture

Status: Postponed » Active

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

larowlan’s picture

mitchell’s picture

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?

andypost’s picture

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

andypost’s picture

Status: Active » Needs review
FileSize
6.15 KB

Initial patch

andypost’s picture

fixed tests

Status: Needs review » Needs work

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

andypost’s picture

Status: Needs work » Needs review
FileSize
9.95 KB
3.23 KB

Fixed direct queries

andypost’s picture

fixes fo forum. something wrong with upgrade path

Status: Needs review » Needs work

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

andypost’s picture

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

andypost’s picture

Status: Needs work » Needs review
FileSize
9.56 KB

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
10.79 KB
2.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

dawehner’s picture

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

andypost’s picture

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

dawehner’s picture

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

andypost’s picture

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)

andypost’s picture

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

tstoeckler’s picture

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

larowlan’s picture

+++ 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?

andypost’s picture

@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

podarok’s picture

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

podarok’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Great job @andypost.

webchick’s picture

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

xjm’s picture

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.

xjm’s picture

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.

dawehner’s picture

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

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

xjm’s picture

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.

jibran’s picture

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

andypost’s picture

still green!

Dries’s picture

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. :)

andypost’s picture

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

tstoeckler’s picture

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++

alexpott’s picture

Category: feature » task
alexpott’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
12.06 KB

Merge for current HEAD

andypost’s picture

Component: base system » history.module
andypost’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
12.09 KB

Chasing head

andypost’s picture

FileSize
12.15 KB
708 bytes

fix doc-block

andypost’s picture

FileSize
12.35 KB
752 bytes

and bit more

Crell’s picture

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

andypost’s picture

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.

larowlan’s picture

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

Crell’s picture

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.

larowlan’s picture

its a for-real kernel/routing controller

andypost’s picture

So what is a recommended implementation?
service or conroller?

andypost’s picture

FileSize
12.5 KB

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()

andypost’s picture

Wim Leers’s picture

andypost’s picture

FileSize
12.27 KB

re-rolled

Status: Needs review » Needs work

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

andypost’s picture

History manager is mostly ready in #2081585: [META] Modernize the History module
Should we close this one in favor of it? or just move back here when its ready?

andypost’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

Issue summary: View changes
Status: Needs work » Needs review
Related issues: +#2081585: [META] Modernize the History module
FileSize
10.02 KB

Patch just updates schema, so history API is not changed, planing to fix usage in #2081585: [META] Modernize the History module

Wim Leers’s picture

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?

Wim Leers’s picture

Status: Needs review » Needs work

d.o--

andypost’s picture

Status: Needs work » Needs review
FileSize
10.46 KB

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: [META] Modernize the History module with the same changes

andypost’s picture

FileSize
2.69 KB

interdiff

andypost’s picture

Suppose we could close this one as duplicate of #2081585: [META] Modernize the History module

andypost’s picture

Status: Needs review » Closed (duplicate)

Duplicate of #2081585: [META] Modernize the History module which is green again