Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#70 | interdiff.txt | 2.69 KB | andypost |
#69 | 1029708-history-68.patch | 10.46 KB | andypost |
#52 | interdiff.txt | 708 bytes | andypost |
#52 | 1029708-history-52.patch | 12.15 KB | andypost |
#51 | 1029708-history-50.patch | 12.09 KB | andypost |
Comments
Comment #1
oddbit CreditAttribution: oddbit commentedI have made a proof of concept and posted in a forum post: http://drupal.org/node/1029862
Comment #2
rickvug CreditAttribution: rickvug commentedThis 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.
Comment #3
ro-no-lo CreditAttribution: ro-no-lo commentedCan 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.
Comment #4
oddbit CreditAttribution: oddbit commentedI 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.
Comment #5
sunComment #6
moshe weitzman CreditAttribution: moshe weitzman commentedMaking 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.
Comment #7
sunActually, 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)
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedThere is no reason why this issue must wait on that issue. They are independent decisions.
Comment #9
larowlan#1120928: Move "history" table into separate History module is rtbc - anyone working on this? could use it in #731724: Convert comment settings into a field to make them work with CMI and non-node entities
Comment #10
mitchell CreditAttribution: mitchell commentedI 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?
Comment #11
andypostThis feature really useful but having just API for this is useless. This require some settings form at least
Comment #12
andypostInitial patch
Comment #13
andypostfixed tests
Comment #15
andypostFixed direct queries
Comment #16
andypostfixes fo forum. something wrong with upgrade path
Comment #18
andypostOh... 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
Comment #19
andypostShould be green. Suppose better move this update function to "head2head" a kind of module
EDIT Patch just removes
history_update_8000()
Comment #21
andypostSo 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
Comment #22
dawehnerI'm wondering whether we should switch the order of the parameters to be more consistent with all other parts in core?
You want to remove both entries of other entity types and the actual entries of the user module.
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.
Comment #23
andypostChanged 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
Comment #24
dawehnerIs there a specific reason why we are doing that here?
That code looks odd, though I'm not sure about the preferred codestyle.
Comment #25
andypostJust for performance reasons. there could be a changes in size of field but index should stay short (same used in locale.module schema)
Comment #26
andypost#24.1 used in locale module to minimize index size
#24.2 changed to one-line as we have all over core (see interdiff)
Comment #27
tstoecklerThis 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.
Comment #28
larowlanif we make this history_read($entity_id, $entity_type = 'node') this function remains backwards compatible for contrib. Thoughts?
same here
No views support for other entity types? Is there a follow up for that?
Comment #29
andypost@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
Comment #30
podarokhttp://drupal.org/node/1845806 possibly should be updated after this patch commited
Comment #30.0
podarokUpdated issue summary.
Comment #31
larowlanGreat job @andypost.
Comment #32
webchickNice patch. Unfortunately we're ridiculously over thresholds atm, but hopefully that will be rectified soonish.
Comment #33
xjm#29: 1029708-history-29.patch queued for re-testing.
Comment #35
xjm#29: 1029708-history-29.patch queued for re-testing.
Comment #37
dawehner#29: 1029708-history-29.patch queued for re-testing.
Comment #38
xjmEdit: I didn't review this patch; just restoring the previous status since there was a testbot error on the retest.
Comment #39
jibran#29: 1029708-history-29.patch queued for re-testing.
Comment #40
andypoststill green!
Comment #41
Dries CreditAttribution: Dries commentedGiven 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. :)
Comment #42
andypost@Dries we have entiti_id and entity_type for all core tables so this notion is preserved here too
Comment #43
tstoecklerI 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++
Comment #44
alexpottAs this is a followup of the critical task #731724: Convert comment settings into a field to make them work with CMI and non-node entities making into a task
Comment #45
alexpott#29: 1029708-history-29.patch queued for re-testing.
Comment #47
andypostMerge for current HEAD
Comment #48
andypostComment #49
andypost#47: 1029708-history-47.patch queued for re-testing.
Comment #51
andypostChasing head
Comment #52
andypostfix doc-block
Comment #53
andypostand bit more
Comment #54
Crell CreditAttribution: Crell commentedIf we're touching history, turn it into a proper service and mark the functions @deprecated, just wrapping the service.
Comment #55
andypostAwesome 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.
Comment #56
larowlanDrupal\Core\Entity\Controller\EntityViewController implements ControllerInterface already, so we can just add to the ::create and _construct methods to include the service definition.
Comment #57
Crell CreditAttribution: Crell commentedI'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.
Comment #58
larowlanits a for-real kernel/routing controller
Comment #59
andypostSo what is a recommended implementation?
service or conroller?
Comment #60
andypostMerge 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()
Comment #61
andypostStart this wrapping in #2081585-1: [META] Modernize the History module
Comment #62
Wim LeersThis needs a reroll now that #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user got committed.
Comment #63
andypostre-rolled
Comment #65
andypostHistory 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?
Comment #65.0
andypostUpdated issue summary.
Comment #66
andypostPatch just updates schema, so history API is not changed, planing to fix usage in #2081585: [META] Modernize the History module
Comment #67
Wim Leerss/entity_type/type/
s/entity was/entity that was/
The ID of the entity that was read.
Is this even necessary, now that we're dropping
hook_update_N()
in favor of Migrate in core?s/read the entity_id/read the {history}.entity_id entity/.
I don't think this should have a default value?
Comment #68
Wim Leersd.o--
Comment #69
andypostFixed #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
Comment #70
andypostinterdiff
Comment #71
andypostSuppose we could close this one as duplicate of #2081585: [META] Modernize the History module
Comment #72
andypostDuplicate of #2081585: [META] Modernize the History module which is green again