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.
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
Move {history} schema definition from system.install to node.install- Find a proper name for the new module (history.module would mean something else)
- Move the functionality into a new module
- Make it work for all entities: #1029708: History table for any entity
Comment | File | Size | Author |
---|---|---|---|
#70 | 1120928-move_history_to_its_own_module-70.patch | 21.22 KB | dagmar |
#68 | 1120928-move_history_to_its_own_module-68.patch | 21.23 KB | dagmar |
#68 | interdiff.txt | 2.27 KB | dagmar |
#66 | 1120928-move_history_to_its_own_module-66.patch | 21.23 KB | dagmar |
#66 | interdiff.txt | 4.44 KB | dagmar |
Comments
Comment #1
sunTempted 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).
Comment #2
sunComment #3
catchI'd be fine with moving it to node, then eventually entity.module #1018602: Move entity system to a module.
Comment #4
catchOr a history module is interesting, not all sites need this kind of history tracking.
Comment #5
sunAlright, let's do a history.module in a second step. Perfectly in line with our framework campaign. :)
Comment #6
Dries CreditAttribution: Dries commentedCommitted to 7.x and 8.x. Moving back to 'needs work' so we can work on phase 2 of this patch?
Comment #7
sun*SCNR*
Comment #8
sunThanks to Berdir, this one includes the (final?) hack to enable the module in an update without re-installing the schema.
Comment #9
sunComment #11
sunumph, stupid me.
Comment #12
andypost@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
Comment #13
sunI 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.
Comment #14
catchThere 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.
Comment #15
sun@catch: I understand. However,
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.
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.
Comment #16
sunAlright - @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:
Comment #17
sunFixed a small oversight in Forum's unread query.
Comment #18
podaroksubscribe
Comment #19
catchI should have posted this as a task.
Comment #20
mirocow CreditAttribution: mirocow commentedsubscribe
Comment #21
quicksketchUpdating 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.
Comment #22
sunThis seems to be only remaining required @todo.
...whereas these ones may be moved to follow-up patches/issues.
1 days to next Drupal core point release.
Comment #23
joachim CreditAttribution: joachim commentedNot 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.
Comment #24
catchI 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.
Comment #25
joachim CreditAttribution: joachim commented> 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.
Comment #26
xjmTagging issues not yet using summary template.
Comment #27
sunWrote a proper issue summary.
Comment #28
xjmAlright, 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:
entity.module
?entity_view_counter? entity_read_counter? entity_counter? entity_statistics? (risking confusion but related functionality again)
Comment #29
quicksketchAs 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.
Comment #30
MichelleIf 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
Comment #31
mototribe CreditAttribution: mototribe commentedI 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 ...
Comment #32
MichelleMy 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
Comment #33
sunRebased http://drupalcode.org/sandbox/sun/1255586.git/shortlog/refs/heads/platfo... against latest HEAD.
Comment #35
andypostSuppose this will require to add entity_type key for history table
Comment #36
tim.plunkettNothing in this issue summary makes it seem major.
Comment #37
ParisLiakos CreditAttribution: ParisLiakos commentedAdded history as dependency of forum module...
i think it makes sense as #24
Comment #39
ParisLiakos CreditAttribution: ParisLiakos commentedIt should pass testes without the dependency now
Comment #41
andypostAny reason to change this to 0?
API should describe it's hook
Probably node.module should depend on history in this case
Hook should be described in history.api.php
Comment #42
sunThanks 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.
Comment #43
mdupont(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.
Comment #44
mdupontCrossposting.
Comment #45
ParisLiakos CreditAttribution: ParisLiakos commentedthanks andypost for the review!
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
Comment #46
dagmarRe-rolled, moved views integration into history module. Node, Comment, Tracker and Forum tests pass locally.
Comment #48
sunCommentTestBase specifies node and comment already, they do not have to be re-specified again, since $modules is collected from all classes.
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.
This should use update_module_enable().
Not sure whether we want to enable history module for standard profile...
Comment #49
dagmarFixed the remaining tests from #46
Comment #50
dagmarApplied changes suggested in #49.
Regarding
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.
Comment #51
cweagansI'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.
Comment #52
sun@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.
Comment #53
dagmarI don't really like entity_tracker, because we already have another module called tracker, so...
I was thinking in one of this names:
Also remember that the description of history says: "Records which user has read which content."
Comment #54
sunSilly 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.
Comment #55
chx CreditAttribution: chx commentedWell, 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.
Comment #56
Lars Toomre CreditAttribution: Lars Toomre commentedAs a follow-up issue, shouldn't we convert HISTORY_READ_LIMIT to be set from a config file?
Comment #57
larowlancould 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.
Comment #58
catchnode_last_viewed() only makes sense with the history module installed, can't we move that to history module?
Comment #59
dagmarRe-rolled. Moved node_last_viewed() to history_node_last_viewed() in the history module.
Comment #61
dagmarComment #63
dagmarComment #64
sunFWIW, 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:
Let's revert this white-space change — such unintended changes are guaranteed to always cause re-rolls somewhere else. ;)
Would it make sense to rename these two to:
?
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.We're missing a
history_user_delete()
implementation.Comment #65
mitchell CreditAttribution: mitchell commentedRe #42 / #25 (activity log): There's Message too, and on a related subject: #1836598: Convert dblog record[-s/-ing] to Entity API.
Comment #66
dagmarAddressed issues from #64
Comment #67
sunAwesome. 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 withhistory_write()
(I'd expect _read to come before _write).Comment #68
dagmarMoved history_read at the beginning of history.module.
Tagging, we should document this minor API change.
Comment #69
sunThank you! :)
Comment #70
dagmarRe-rolled
Comment #71
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #72
catchNeeds a change notice.
Also, nice to see this one in :)
Comment #73
sunChange 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?
Comment #75
firebird CreditAttribution: firebird commentedIs this going to be backported to D7?
Comment #76
catchNo it's too much of an API change to backport.
Comment #76.0
catchUpdated issue summary.