Split off from #2705389: Selected content entities should extend EditorialContentEntityBase or RevisionableContentEntityBase also see #2721313: Upgrade path between revisionable / non-revisionable entities
- Which entity types need their revisions exposed through UI?
- How do we decide which ones do, which ones don't?
- Map out where revision tabs should live for entity types.
Current State
Conclusion
Will
Nodes
Already revisionable in core.
Block Content
Already revisionable in core.
Content Moderation State
Already revisionable in core.
Terms
As these are often user generated content they will benefit from being revisionable.
Menu Links
These often relate to revisionable content so should be revisionable themselves.
Path aliases: #3007661: Modernize the path alias system
Path alias is not a content entity type today, but will need to be converted to a revisionable content entity in order to properly solve various forward revision issues in the long term. (Based on Hard Problems meeting in Baltimore 2017.)
Should
Comments
@webchick from #9:
Would allow some recourse for content administrators to go after a malicious user who filled their comment with a bunch of racist garbage to which other users reacted, and then later went back and edited their comment to something more innocent, to dodge being banned.
Deferred
Users
The user entity is tricky as it's used as both account, profile and authentication. Making it revisionable will cause lots of trouble, especially around security and privacy. Also, the underlying reason we are making more things revisionable is to later introduce the concept of workspaces and replication. And we certainly don't want accounts to be workspace specific, nor be replicate-able. So for now there's no need to make users revisionable.
Aggregator items
These don't need to be revisionable in this first iteration.
Shortcuts
Shortcuts are not really "editorial" content entity types, and a trial conversion for them is not needed anymore.
Won't
Files
Making files revisionable could result in interesting regression issues. (Note that Media entities, once in core, will likely be revisionable, but until that's in core, it's not this issue's responsibility. See #17 for details.)
Contact Messages
There's never a case for creating more than one contact message revision
Comment | File | Size | Author |
---|---|---|---|
#34 | drupal-8-core-content-entities.png | 31.52 KB | vijaycs85 |
Comments
Comment #2
dixon_In a meeting with alexpott, catch, timmillwood and I on Thursday June 9 we agreed that the following entity types should NOT have revisions:
This means that the following entity types will have revisions:
So based on the above list, I'd guess that Nodes, Block Content and Terms needs most of the UX focus in terms of the revision UI?
Comment #3
effulgentsia CreditAttribution: effulgentsia at Acquia commented+1 to #2.
If there's no revision UI for these, then how would you get these into a Workflow workspace?
Should this be Aggregator feeds instead? Do the items need independent revisioning, or only the ability to have separate items that can each correspond to a particular feed revision? I.e., for a given feed revision, is there any sense in multiple item revisions?
Comment #4
yoroy CreditAttribution: yoroy commentedComment #5
xjmIn the revisioning triage session we did last week, we agreed to split this issue into two parts. "Which core entities get revisions" is part of #2725433: WI: Phase A: Use the revision API in more places. This phase will only enable the storages for the specified entity types, not enable revisioning for them nor provide any UI. "What will their UI look like" will be a blocker for a later phase of #2721129: Workflow Initiative.
Based on that rescoping, removing the design and UX tags. (We can and should certainly start the design work separately; it is just not a required blocker for anything going into 8.2.x.)
Comment #6
xjmForgot to mention, there are also already issues for the UI work listed in #2721129: Workflow Initiative:
Comment #7
coleman.sean.c CreditAttribution: coleman.sean.c as a volunteer commentedFor the record, the notes of that meeting and the rationale for those decisions are here: #2721129-42: Workflow Initiative, and catch elaborates on Comments in #2721129-36: Workflow Initiative.
Comment #8
timmillwoodUpdated the issue summary with will, should, and won't entity types to update to revisionable.
Moving this to "needs review" so we can move this issue to be signed off.
Comment #9
webchickThe rest of the list makes sense, but I'd move "comments" under "should," personally. Would allow some recourse for content administrators to go after a malicious user who filled their comment with a bunch of racist garbage to which other users reacted, and then later went back and edited their comment to something more innocent, to dodge being banned.
Comment #10
timmillwoodUpdated issue summary with @webchick's suggestion from #9.
Comment #12
timmillwoodAs #2721313: Upgrade path between revisionable / non-revisionable entities gets closer to being ready I think we need to nail down what we make revisionable.
Comment #13
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThe IS doesn't include the "Content moderation state" entity type. Since that's already revisionable in core, I'm guessing it belongs in the "Will" section.
The IS also doesn't include the "Aggregator feed" entity type. See #3 for my question about both feeds and items. Note that I'm not super familiar with aggregator module, so perhaps my question is based on a flawed understanding of how the module actually works, so please point that out if it is.
Other than that, the rest of the Will, Should, and Won't lists seems great to me.
Comment #14
BerdirThe only data in a feed is the URL and configuration on how frequently it should be updated. I don't think that needs to be revisioned, it is even questionable if that should be a config or a content entity, we went for content etntiy only because sites like d.o can easily have hundreds of those. and we didn't know if that will work with the config system.
And items are synced external feed items, we have no UI to edit them and I don't think it is worth making them revisionable, I don't see why you would want to revert to an old revision, the next update would then just change it again anyway. I think if you need anything like that, you're probably using the feeds module and are importing content into nodes or other entity types that have revision handling.
Comment #15
realityloopCan I just check that files means actual binary files, I'm hoping that media entities will be revisionable?
Comment #16
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThanks for the question. That's the current plan. The proposal in #2801277: [META] Support remote media assets as first-class citizens by adding Media Entity module in core is for a new Media entity type that is in addition to core's File entity type. A file uploaded to the Drupal site via a Media widget would then be represented in the database with 2 entities: a media entity with a reference to a file entity. The media entity (with whatever other fields are on it) would be revisionable while the file entity (the record in the {file_managed} table) would not be. That said, plans might change, based on whatever is learned between now and when that's committed. Since Media entity isn't in core yet, I don't think this issue's summary needs to mention it yet.
If others agree with this logic, then I suggest moving "Aggregator items" from Should to Won't.
Comment #17
BerdirYes, media_entity in contrib supports revisions and #2831274: Bring Media entity module to core as Media module comes with revision support as well.
Media entity will have the same limitions/bugs as using revisionable nodes with files right now of course. We can't revision actual physical files of course, so changing a file in a revision means that we have two separate files both as an entity ans a file. And we still have those bugs around tracking file usage (only difference is that it is then the media_entity that gets the usages)
Comment #18
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAs to why I'm harping on Aggregator items...
From the issue summary:
On the one hand, I can see some value in them being revisionable. You could then build up a time machine of what was at a particular feed url during a particular fetch time of that URL.
On the other hand, I can also see it as problematic. Because if Aggregator feeds are not revisionable, then you can edit a feed entity and change its URL. Now all of your old item entities contain content that potentially never existed at the new feed URL. But those items only reference the feed entity by its ID, not its URL, so you've lost the information about which feed URL actually provided the contents for those old items. This just seems like it would lead to a complete mess.
So, if we want to make aggregator items revisionable, then I think we need to somehow make them store the feed URL that was fetched at that time. Whether that's by adding that URL as a base field to the item entity, or whether it's by making feed entities revisionable, and making item revisions reference the feed revision.
Or, we can do as #14 suggests, and not solve this in Aggregator, but punt the use case to the Feeds module.
Comment #19
Gábor HojtsySo given we agree on nodes, content blocks, terms and menu links as must, that gives plenty to work on as a start? :) Is it important to make decisions on all of them at once?
Comment #20
dixon_There seems to be consensus around everything except Aggregator Items, which understandably require more discussion (it's a tricky edge case).
I have created a follow-up for Aggregator Items, so that we can move forward with the basics :)
Comment #21
catchCan we open up a follow-up for users as well? There is information in the user entity that can/should be revisionable (like e-mail addresses for example, or user name, at the moment we only have current and init for mail and nothing for name). The idea of reverting a user entity revision is frightening, but I don't think this issue conclusively makes some kind of revision for support for users in core a hard 'no'.
Comment #22
dixon_Follow-up created for users: #2834802: [Discussion, no patch] Revision strategy for user/profile data
Comment #23
dixon_Comment #24
catchComment #25
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThanks for #20 and #24.
Also adding "Content Moderation State" to "Will" per #13.
Also mentioning Media entities as a parenthetical to File entities, since it was brought up in #15.
RTBC+1.
Comment #26
effulgentsia CreditAttribution: effulgentsia at Acquia commented@catch and I are both happy with this issue now, so removing the "Needs framework manager review" tag.
Leaving "Needs release manager review" for @catch and/or @xjm to remove when appropriate. Possibly related, I'm not sure what to do with the "Needs followup" tag. It was added in #5, but #6 lists existing issues for (all of? part of?) that. The very beginning of this issue's summary (before the "Conclusions" section) still is mostly focused on UI, so that should be moved from this issue's summary to one of the other issues or a new one.
Comment #27
catchThe main release management question for me is whether any of the existing revision bugs are a blocker to applying revisions to more entities.
Comment #28
xjmhttps://docs.google.com/spreadsheets/d/14lpbLhpu7YqjFwjQ4gCTTlf-gcrOc57C... lists the issues we triaged as blockers last summer; let's go through those and see which of Must and Should are done, and revisit the revisits?
Comment #29
xjm@catch and I agreed on NR for #28.
Comment #30
dawehnerMaking Menu links revisionable could be a bit tricky, but I think this is totally fine, as long we are aware of it.
Comment #32
dixon_Comment #33
dixon_Comment #34
vijaycs85Adding current core content entities state diagram.
Comment #35
vijaycs85Comment #36
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedOpened dedicated issues for each entity type conversion:
#2880149: Convert taxonomy terms to be revisionable
#2880151: Convert shortcuts to be revisionable and publishable
#2880152: Convert custom menu links to be revisionable
#2880154: Convert comments to be revisionable
However, the issue summary is quite outdated and states that shortcuts will be converted as part of #2721313: Upgrade path between revisionable / non-revisionable entities, which is no longer accurate.
Do we want them to be revisionable?
Comment #37
andypostI see no reason to enable revisions for comments by default, for webchick's case makes sense, but we gonna remove forum the main consumer and there's no ui to manage revisions for comments.
The next trouble is fileusage & other references that are often got staled in revisions... and managing comment revisions will make my moderators to quit their jobs
Comment #38
vijaycs85It's also worth checking the possibilities of allowing users to have option to decide if they like to have revision or not. Assuming it's a one time activity and you have to have it on since the site install etc. IMHO, it's going to be necessary (e.g. not all sites want to have comment revisionable). https://www.drupal.org/project/field_sql_norevisions is another example. In workflow point of view, user might want to manage all revisions by workspace instead of individual entity.
Comment #39
timmillwoodIs there any reason not to enable revisions on comments?
Note: enabling revisions != creating revisions. The patch in #2880154: Convert comments to be revisionable doesn't like it adds anything that actually creates new revisions. So each edit would override the initial revision of a comment entity. This would change if, for example, content moderation is enabled, which forces new revisions on all moderated entity types.
Personally I think all content entity types should be revisionable. If we want something definitely not revisionable we do that on a per field basis. Then we decide later where and how a new revision is created.
Comment #40
jibranRE#36: Personally, I can't come up with a use-case where a revisionable/publishable shortcut makes any sense.
I think this argument from issue summary is obsolete now.
I think #2860654: Field storage CRUD operations must use the last installed entity type and field storage definitions provides a proof for that.
Comment #42
yoroy CreditAttribution: yoroy commentedSlight tangent, this is from the ideas queue: #2859995: Add Entity Reference Revisions to core
Comment #44
giorgio79 CreditAttribution: giorgio79 commentedWhat is the performance impact of all this revisioning magic? :) Some reference from the past #2083451: Reconsider the separate field revision data tables
Comment #46
Wim LeersWhere is this at?
Comment #47
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedWaiting for the child issues to be fixed.
Comment #48
Wim LeersThanks!
Comment #50
fgmSurprised this hasn't been mentioned:I'm well aware of the problems with the User entity having multiple duties, but having it be revisionable would be interesting for some auditability situations. An extra problem on top of the access issues, of course, would be the various "weak" (for want of a better name) base fields like "access" and "login", which should not usually not create revisions when updated.
Comment #51
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI just closed #2880151: Convert shortcuts to be revisionable and publishable because we don't really need a trial conversion anymore, we already converted taxonomy terms and menu links in 8.7.0.
That leaves comments as a "simple" conversion: #2880154: Convert comments to be revisionable and path aliases have their own meta issue, which involves a rewrite of the current system: #3007661: Modernize the path alias system.
@fgm, I think we should have a separate issue to discuss the case for users, this one has run its course :)
Comment #52
fgm@amateescu agreed for the users followup.
Comment #54
fgmDidn't find the followup issue for user revisions, does it exist ?
Comment #55
pcambra@fgm, it's linked somewhere here: #540118: Add revision support to users, also found #2688559: Make the User entity revisionable