Currently in core there are only two revisionable entity types, Node and BlockContent. There are other issues (such as #2705433: Node should implement RevisionLogInterface) which look at making these implement RevisionLongInterface, because extending RevisionableContentEntityBase could be difficult or API breaking.

This issue will look at converting the following entity types to be revisionable and extend RevisionableContentEntityBase:

  • Comment
  • Feed (actual feed information)
  • File
  • Item (Feed item)
  • MenuLinkContent
  • Message (Contact form message)
  • Shortcut
  • Term

This first depends on #2707255: All content entities should inherit baseFieldDefinitions to make sure the relevant base fields are added once these entity types are updated.

Note: We are not planning to make the User entity revisionable. Users are a special case in Drupal as they can be both content, and the account that you authenticate against. This means that making them revisionable could result in more complications than are in scope of this issue.

To be done

  • Figure out how the data migration should work

Background

Multiversion module currently makes all content entities revisionable by a series of hooks. It however does not make use of RevisionableContentEntityBase.

CommentFileSizeAuthor
#42 interdiff.txt5 KBtimmillwood
#42 all_content_entities-2705389-42.patch36.88 KBtimmillwood
#39 interdiff.txt4.24 KBtimmillwood
#39 all_content_entities-2705389-39.patch36.54 KBtimmillwood
#36 all_content_entities-2705389-36.patch38.34 KBtimmillwood
#36 interdiff.txt3.74 KBtimmillwood
#33 all_content_entities-2705389-33.patch34.61 KBtimmillwood
#33 interdiff.txt2.82 KBtimmillwood
#31 interdiff.txt13.06 KBtimmillwood
#31 all_content_entities-2705389-31.patch32.85 KBtimmillwood
#29 interdiff.txt12.04 KBtimmillwood
#29 all_content_entities-2705389-29.patch32.84 KBtimmillwood
#26 interdiff.txt3.01 KBtimmillwood
#26 all_content_entities-2705389-26.patch25.3 KBtimmillwood
#24 all_content_entities-2705389-24.patch22.51 KBtimmillwood
#24 interdiff.txt2.99 KBtimmillwood
#21 interdiff.txt4.6 KBtimmillwood
#21 all_content_entities-2705389-21.patch21.73 KBtimmillwood
#19 interdiff.txt3.44 KBtimmillwood
#19 all_content_entities-2705389-19.patch17.75 KBtimmillwood
#15 all_content_entities-2705389-15.patch14.37 KBtimmillwood
#13 all_content_entities-2705389-13.patch14.43 KBtimmillwood
#11 all_content_entities-2705389-11.patch12.09 KBtimmillwood
#9 all_content_entities-2705389-8.patch10.08 KBtimmillwood
extend-RevisionableContentEntityBase.patch11.14 KBtimmillwood
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

timmillwood created an issue. See original summary.

Status: Needs review » Needs work

The last submitted patch, extend-RevisionableContentEntityBase.patch, failed testing.

timmillwood’s picture

This doesn't work because many entities baseFieldDefinitions method doesn't inherit fields from the parent, and therefore doesn't get a revision_id field added. Upon fixing that I got the following error:

[Wed Apr 13 12:17:37.880682 2016] [:error] [pid 6021] [client 127.0.0.1:53166] Uncaught PHP Exception Drupal\\Core\\Database\\DatabaseExceptionWrapper: "SQLSTATE[42S22]: Column not found: 1054 Unknown column 'data.link' in 'field list': SELECT revision.*, data.title AS title, data.weight AS weight, data.link AS link\nFROM \n{shortcut_field_revision} revision\nLEFT OUTER JOIN {shortcut_field_data} data ON (revision.id = data.id)\nWHERE  (revision.id IN  (:db_condition_placeholder_0, :db_condition_placeholder_1)) AND (revision.revision_id IN  (:db_condition_placeholder_2, :db_condition_placeholder_3)) \nORDER BY revision.id ASC; Array\n(\n    [:db_condition_placeholder_0] => 1\n    [:db_condition_placeholder_1] => 2\n    [:db_condition_placeholder_2] => 1\n    [:db_condition_placeholder_3] => 2\n)\n" at /home/timmillwood/Code/drupal1/core/lib/Drupal/Core/Database/Connection.php line 671

As I said in the original post, this is no mean feat.

timmillwood’s picture

dixon_’s picture

Issue summary: View changes
daffie’s picture

timmillwood’s picture

dixon_’s picture

Issue tags: +Workflow Initiative
timmillwood’s picture

Status: Postponed » Needs review
FileSize
10.08 KB

This patch makes add a revision key to all content entities and switches them to use RevisionableContentEntityBase.
It also makes a couple of the fields on Shortcut revisionable.

Status: Needs review » Needs work

The last submitted patch, 9: all_content_entities-2705389-8.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
12.09 KB

Fixing a bunch of tests.

Status: Needs review » Needs work

The last submitted patch, 11: all_content_entities-2705389-11.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
14.43 KB

Fixing a bunch more tests.

Status: Needs review » Needs work

The last submitted patch, 13: all_content_entities-2705389-13.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
14.37 KB

Switching Users to being non-revisionable.

timmillwood’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 15: all_content_entities-2705389-15.patch, failed testing.

jojototh’s picture

Issue summary: View changes
timmillwood’s picture

Assigned: Unassigned » timmillwood
Priority: Normal » Major
FileSize
17.75 KB
3.44 KB

Adding a proof of concept upgrade path solution.

\Drupal\system\EntitySchemaUpdater service uses the storage schema to generate missing revision tables, and entityDefinitionUpdateManager to add missing revision fields.

Only an update hook for comment module has been added so far, I also need to look at how to copy the data from the non-revision tables.

timmillwood’s picture

Status: Needs work » Needs review
timmillwood’s picture

The last submitted patch, 19: all_content_entities-2705389-19.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 21: all_content_entities-2705389-21.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
2.99 KB
22.51 KB

Working towards more passing tests.

Status: Needs review » Needs work

The last submitted patch, 24: all_content_entities-2705389-24.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
25.3 KB
3.01 KB

Making fields revisionable seems to fix a load of issues, testing here to see how many.

Status: Needs review » Needs work

The last submitted patch, 26: all_content_entities-2705389-26.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
32.84 KB
12.04 KB

Very much a work on progress patch this time with some hacky "fixes" as I try to understand the bugs / issues we're facing.

Status: Needs review » Needs work

The last submitted patch, 29: all_content_entities-2705389-29.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
32.85 KB
13.06 KB

Status: Needs review » Needs work

The last submitted patch, 31: all_content_entities-2705389-31.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
2.82 KB
34.61 KB

Adding revision_created, revision_user, and revision_log_message fields in update hook.
Also adding making all translatable fields revisionable.

Status: Needs review » Needs work

The last submitted patch, 33: all_content_entities-2705389-33.patch, failed testing.

timmillwood’s picture

One thing I can't understand is I am not touching Node or BlockContent, but they are playing a part of the After all updates ran, entity schema is up to date. failure.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
3.74 KB
38.34 KB

Just wanted to upload my latest developments.

Status: Needs review » Needs work

The last submitted patch, 36: all_content_entities-2705389-36.patch, failed testing.

timmillwood’s picture

Well that was full of fail!

timmillwood’s picture

Status: Needs work » Needs review
FileSize
36.54 KB
4.24 KB

Think I'm making progress now.

This patch adds a load of debug stuff that won't be needed in the final version, but just want to show my working.

Status: Needs review » Needs work

The last submitted patch, 39: all_content_entities-2705389-39.patch, failed testing.

The last submitted patch, 39: all_content_entities-2705389-39.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
36.88 KB
5 KB

Fixing due to core moving MenuLinkTreeTest

Status: Needs review » Needs work

The last submitted patch, 42: all_content_entities-2705389-42.patch, failed testing.

timmillwood’s picture

Assigned: timmillwood » Unassigned
Status: Needs work » Postponed

Switching this back to postponed. Going to try and get just an upgrade path service working in #2721313: Upgrade path between revisionable / non-revisionable entities, then go back to making things revisionable.

effulgentsia’s picture

I know this issue is postponed, but just linking in a related discussion from #2745619: [policy, no patch] Which core entities get revisions?.

  1. +++ b/core/modules/comment/src/Entity/Comment.php
    @@ -57,7 +58,7 @@
    -class Comment extends ContentEntityBase implements CommentInterface {
    +class Comment extends RevisionableContentEntityBase implements CommentInterface {
    ...
    +++ b/core/modules/file/src/Entity/File.php
    @@ -25,13 +25,14 @@
    -class File extends ContentEntityBase implements FileInterface {
    +class File extends RevisionableContentEntityBase implements FileInterface {
    

    In #2745619-2: [policy, no patch] Which core entities get revisions?, the proposal is for comments and files to not be revisionable.

  2. +++ b/core/modules/aggregator/src/Entity/Item.php
    @@ -29,12 +29,13 @@
    -class Item extends ContentEntityBase implements ItemInterface {
    +class Item extends RevisionableContentEntityBase implements ItemInterface {
    

    In #2745619-3: [policy, no patch] Which core entities get revisions?, I asked if Items need to be independently revisionable. Or if instead, fid needs to be converted from an entity_reference to an entity_revision_reference. We don't have the latter in core yet, though perhaps this is a use case that would require it.

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.

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.

catch’s picture

When we convert taxonomy terms we're going to have to deal with the issue of taxonomy hierarchy (both the vocabulary drag and drop UI and the parent field on term forms). See the equivalent issue for book module #2858431: Book storage and UI is not revision aware, changes to drafts leak into live site.

amateescu’s picture

@catch, like your proposed solutions from #2858431: Book storage and UI is not revision aware, changes to drafts leak into live site, we could choose to do things in two steps for taxonomy terms as well:

1) leave the 'parent' field as non-revisionable in the initial coversion patch/issue and make sure that a draft term can not change its parent
2) work out the complexity of getting entity reference revisions in core :)

timmillwood’s picture

Title: All content entities should extend RevisionableContentEntityBase » All content entities should extend EditorialContentEntityBase
Related issues: +#2825973: Introduce a EditorialContentEntityBase class for revisionable and publishable entity types
timmillwood’s picture

Title: All content entities should extend EditorialContentEntityBase » Selected content entities should extend EditorialContentEntityBase or RevisionableContentEntityBase

Maybe not all content entities.

catch’s picture

@amateescu I'm not sure how entity reference revisions would work for term hierarchy - if revision A references term 2 and revision B references term 3, the issue is that the reference itself is not versioned with the referencing entity, not with versions of the referenced entity. The main issue with {term_hierarchy} and versioning is that it's not an entity reference at all. We should possibly open a new issue for this, but it'd be PP-4 or PP-5 or something...

amateescu’s picture

Status: Postponed » Closed (duplicate)

We now have dedicated issues for each entity type conversion, so I'm going to close this one out as a duplicate.

#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

@catch, The number you were looking for at the time was more like PP-8, but now it got to a more manageable 3 :) Also, I mentioned your concern from #48 in the dedicated issue.