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

Drupal 8 core content entities class diagram

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

Shortcuts
As part of #2721313: Upgrade path between revisionable / non-revisionable entities these will become revisionable mainly as a trial entity type, and also because core ships with shortcut entities, which makes for a good test.
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.

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

Files: 
CommentFileSizeAuthor
#34 drupal-8-core-content-entities.png31.52 KBvijaycs85

Comments

yoroy created an issue. See original summary.

dixon_’s picture

In a meeting with alexpott, catch, timmillwood and I on Thursday June 9 we agreed that the following entity types should NOT have revisions:

  • Files
  • Contact Messages
  • Users
  • Comments

This means that the following entity types will have revisions:

  • Nodes (already the case)
  • Block Content (already the case, but UI needs improvements)
  • Terms (UI needed?)
  • Menu Links (UI not needed?)
  • Shortcuts (UI not needed?)
  • Aggregator items (UI not needed?)

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?

effulgentsia’s picture

+1 to #2.

Menu Links (UI not needed?)
Shortcuts (UI not needed?)

If there's no revision UI for these, then how would you get these into a Workflow workspace?

Aggregator items (UI not needed?)

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?

yoroy’s picture

Issue tags: +DevDaysMilan
xjm’s picture

Title: Which core entities get revisions and what does their respective UI look like? » Which core entities get revisions?
Issue tags: -Usability, -Needs design +Needs followup

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

xjm’s picture

coleman.sean.c’s picture

In a meeting with alexpott, catch, timmillwood and I on Thursday June 9 we agreed that the following entity types should NOT have revisions:

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

timmillwood’s picture

Issue summary: View changes
Status: Active » Needs review

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

webchick’s picture

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

timmillwood’s picture

Issue summary: View changes

Updated issue summary with @webchick's suggestion from #9.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

timmillwood’s picture

As #2721313: Upgrade path between revisionable / non-revisionable entities gets closer to being ready I think we need to nail down what we make revisionable.

effulgentsia’s picture

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

Berdir’s picture

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

realityloop’s picture

Can I just check that files means actual binary files, I'm hoping that media entities will be revisionable?

effulgentsia’s picture

I'm hoping that media entities will be revisionable?

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

I don't see why you would want to revert to an old [aggregator item] 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

If others agree with this logic, then I suggest moving "Aggregator items" from Should to Won't.

Berdir’s picture

Yes, 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)

effulgentsia’s picture

As to why I'm harping on Aggregator items...

From the issue summary:

Aggregator items
These don't need to be revisionable, but is there any reason for them not to be?

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.

Gábor Hojtsy’s picture

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

dixon_’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Related issues: +#2834440: [Discussion, no patch] Decide if Aggregator Items needs to be revisionable

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

catch’s picture

Can 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'.

dixon_’s picture

dixon_’s picture

catch’s picture

Issue summary: View changes
effulgentsia’s picture

Issue summary: View changes

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

effulgentsia’s picture

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

catch’s picture

The main release management question for me is whether any of the existing revision bugs are a blocker to applying revisions to more entities.

xjm’s picture

Title: Which core entities get revisions? » [policy, no patch] Which core entities get revisions?

https://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?

xjm’s picture

Status: Reviewed & tested by the community » Needs review

@catch and I agreed on NR for #28.

dawehner’s picture

Menu Links
These often relate to revisionable content so should be revisionable themselves.

Making Menu links revisionable could be a bit tricky, but I think this is totally fine, as long we are aware of it.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dixon_’s picture

dixon_’s picture

Issue summary: View changes
vijaycs85’s picture

Issue summary: View changes
FileSize
31.52 KB

Adding current core content entities state diagram.

vijaycs85’s picture

Issue summary: View changes
amateescu’s picture

Opened dedicated issues for each entity type conversion:

#2880149: [PP-3] Convert taxonomy terms to be revisionable and publishable
#2880151: [PP-3] Convert shortcuts to be revisionable and publishable
#2880152: [PP-3] Convert custom menu links to be revisionable
#2880154: [PP-3] 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?

andypost’s picture

I 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

vijaycs85’s picture

It'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.

timmillwood’s picture

Is there any reason not to enable revisions on comments?

Note: enabling revisions != creating revisions. The patch in #2880154: [PP-3] 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.

jibran’s picture

RE#36: Personally, I can't come up with a use-case where a revisionable/publishable shortcut makes any sense.

As part of #2721313: Upgrade path between revisionable / non-revisionable entities these will become revisionable mainly as a trial entity type, and also because core ships with shortcut entities, which makes for a good test.

I think this argument from issue summary is obsolete now.
I think #2860654: [PP-1] FIeld storage CRUD operations must use the last install entity type and field definitions provides a proof for that.