Posted by oddbit on January 17, 2011 at 12:27pm
16 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | base system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | reviewed & tested by the community |
| Issue tags: | Entity system |
Issue Summary
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
Comments
#1
I have made a proof of concept and posted in a forum post: http://drupal.org/node/1029862
#2
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.
#3
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.
#4
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.
#5
#6
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.
#7
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)
#8
There is no reason why this issue must wait on that issue. They are independent decisions.
#9
#1120928: Move "history" table into separate History module is rtbc - anyone working on this? could use it in #731724: Decouple comment.module from node by turning comment settings into a field
#10
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: Add test coverage and/or one real use of PSR-3 based watchdog logger object to store this info?
#11
This feature really useful but having just API for this is useless. This require some settings form at least
#12
Initial patch
#13
fixed tests
#14
The last submitted patch, 1029708-history-13.patch, failed testing.
#15
Fixed direct queries
#16
fixes fo forum. something wrong with upgrade path
#17
The last submitted patch, 1029708-history-16-dep.patch, failed testing.
#18
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
#19
Should be green. Suppose better move this update function to "head2head" a kind of module
EDIT Patch just removes
history_update_8000()#20
The last submitted patch, 1029708-history-19.patch, failed testing.
#21
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
#22
+++ 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.
#23
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
#24
+++ 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.
#25
Just for performance reasons. there could be a changes in size of field but index should stay short (same used in locale.module schema)
#26
#24.1 used in locale module to minimize index size
#24.2 changed to one-line as we have all over core (see interdiff)
#27
+++ 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.
#28
+++ 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?
#29
@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
#30
http://drupal.org/node/1845806 possibly should be updated after this patch commited
#31
Great job @andypost.
#32
Nice patch. Unfortunately we're ridiculously over thresholds atm, but hopefully that will be rectified soonish.
#33
#29: 1029708-history-29.patch queued for re-testing.
#34
The last submitted patch, 1029708-history-29.patch, failed testing.
#35
#29: 1029708-history-29.patch queued for re-testing.
#36
The last submitted patch, 1029708-history-29.patch, failed testing.
#37
#29: 1029708-history-29.patch queued for re-testing.
#38
Edit: I didn't review this patch; just restoring the previous status since there was a testbot error on the retest.
#39
#29: 1029708-history-29.patch queued for re-testing.
#40
still green!