Support for Revisions on groups

I am interested in this too since I found that panelizer depends on revisioning if you wish to use it to override the layout of the main group page.

Here is an initial patch based on the linked article. I had to completely uninstall group before using this patched version of the module. This has no upgrade path so this should be considered if you wish to try it out. We need an upgrade path in order to use this. Pretty sure we just have to add some columns upon upgrade, namely vid.

So taking some time this morning to document differences after enabling revisions by uninstalling group then installing with the patched version of the module:

The revision_id on this table:

> show columns from groups;
+-------------+------------------+------+-----+---------+----------------+
| Field       | Type             | Null | Key | Default | Extra          |
+-------------+------------------+------+-----+---------+----------------+
| id          | int(10) unsigned | NO   | PRI | NULL    | auto_increment |
| revision_id | int(10) unsigned | YES  | UNI | NULL    |                |*
| type        | varchar(32)      | NO   | MUL | NULL    |                |
| uuid        | varchar(128)     | NO   | UNI | NULL    |                |
| langcode    | varchar(12)      | NO   |     | NULL    |                |
+-------------+------------------+------+-----+---------+----------------+

New tables:

> show columns from group_revision;
+-------------+------------------+------+-----+---------+----------------+
| Field       | Type             | Null | Key | Default | Extra          |
+-------------+------------------+------+-----+---------+----------------+
| id          | int(10) unsigned | NO   | MUL | NULL    |                |
| revision_id | int(10) unsigned | NO   | PRI | NULL    | auto_increment |
| langcode    | varchar(12)      | NO   |     | NULL    |                |
+-------------+------------------+------+-----+---------+----------------+

> show columns from group_field_revision;
+------------------+------------------+------+-----+---------+-------+
| Field            | Type             | Null | Key | Default | Extra |
+------------------+------------------+------+-----+---------+-------+
| id               | int(10) unsigned | NO   | MUL | NULL    |       |
| revision_id      | int(10) unsigned | NO   | PRI | NULL    |       |
| langcode         | varchar(12)      | NO   | PRI | NULL    |       |
| created          | int(11)          | YES  |     | NULL    |       |
| default_langcode | tinyint(4)       | NO   |     | NULL    |       |
+------------------+------------------+------+-----+---------+-------+

And here is what data looks like in those tables:

> select * from groups;      
+----+-------------+------------+--------------------------------------+----------+
| id | revision_id | type       | uuid                                 | langcode |
+----+-------------+------------+--------------------------------------+----------+
|  1 |           3 | department | 58e9375d-81b4-4e9b-9e41-e6f4ca55c665 | en       |
|  2 |           5 | department | fdd2cada-69d7-4bd2-a5b5-e9f6e7a331b1 | en       |
+----+-------------+------------+--------------------------------------+----------+

> select * from group_revision;
+----+-------------+----------+
| id | revision_id | langcode |
+----+-------------+----------+
|  1 |           1 | en       |
|  1 |           2 | en       |
|  1 |           3 | en       |
|  2 |           4 | en       |
|  2 |           5 | en       |
+----+-------------+----------+

> select * from group_field_revision;
+----+-------------+----------+------------+------------------+
| id | revision_id | langcode | created    | default_langcode |
+----+-------------+----------+------------+------------------+
|  1 |           1 | en       | 1482451432 |                1 |
|  1 |           2 | en       | 1482451432 |                1 |
|  1 |           3 | en       | 1482451432 |                1 |
|  2 |           4 | en       | 1482451657 |                1 |
|  2 |           5 | en       | 1482451657 |                1 |
+----+-------------+----------+------------+------------------+

Historically, Group has not supported revisions on its entities. This was in part because we'd be revisioning someone's "key chain" (membership). To avoid confusion or weird access scenarios, we chose not to handle revisions.

However, I'm inclined to add revision support, but I've had to learn the hard way that adding something like that after the fact is far from easy. I went through writing a long update hook to enable translation support a while ago. If we can write an update hook that both enables revision support on groups and makes sure that existing websites will not suffer from it, I will commit it.

We can then still discuss whether we want to allow group content to be revisionable as well.

I've found this issue while looking for a way to allow a group role to revert revisions on only their group content, is that what this issue is about?

1) We need revisions but only for Group. I've been working on it and it seems that my patch works as expected:

See: https://www.drupal.org/docs/8/api/entity-api/converting-a-content-entity...

2) Entity contrib module is required. See #2350939: Implement a generic revision UI

3) I had this problem: #2919648: menu_link_content_entity_predelete() must not fatal if entity URLs cannot be generated. I fixed the error from Group::urlRouteParameters()

4) Implemented group_post_update_set_values_revision_table() because revision_created and revision_user cannot be NULL. Otherwise we have exceptions from Drupal\entity\Controller\RevisionOverviewController::getRevisionDescription()

I have re-rolled the patch.

Adding related Core issue. @kristiaanvandeneynde says:

If that lands, we can apply it for Group 8.1.0. For Group 8.2.0 I'll see if we can add it in out of the box.

Stay tuned...

Here are some modifications:

• Removed the dependency on Entity contrib module, because it is only for the revisions tabs to appear, but it doesn't not affect the workflow itself. I suggest to wait for #2350939: Implement a generic revision UI.
• Removed the upgrade path, waiting for #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data, as Scot mentioned in previous comment.
• Added status property in a similar way as nodes.

That said this patch is only to be used on new sites without existing groups.

For changes in the use declarations of src/Entity/Group.php is not possible to apply this patch alongside with #2797793: Entities identified by strings as group content, so if someone is interested on use both I created this combined patch.

Added granular permissions and revisions page, now that #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data is RTBC I'm going to start working in the upgrade path.

Added content moderation module to tests.

In my opinion, this patch is doing way too much so it's hard to review properly. I would suggest to split it up in at (at least) three parts:

- adding a publishing status to the group_content entity type
- convert the group_content entity type to revisionable
- split up the current entity_id field into two separate fields

If you agree with that and open separate issues for each of those tasks, I can help with reviewing and providing guidance for the first two items :)

Hi @amateescu,

Thank you for your advice, I will follow your suggestions:

I started creating the patch for publishing status using and existing issue #2873212: Add a status to the group (Publish/Unpublish), also I will update this issue as parent for all the issues related to support revisions on Group entity type.

Added patch only for conversion to revisionable on #3029907: Make groups revisionable.

This is the full patch with the patches from the three child issues combined.

Changed status, because it passed all tests.

Can we use the new functionality that was just added to core for this? #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data
See change record: https://www.drupal.org/node/3029997

Although that would mean we are blocked until May 1st 2019 because that's when core 8.7.0 releases :/ I've not reviewed this issue yet, but if it ships with thorough testing (including an update hook test), I may consider committing this to Group 8.1.0. If everything else is cleaned up and this issue is not in an acceptable state yet, we might want to cut Group 8.1.0 and fix this in 8.1.1 using the update path provided by core 8.7.0.

Also, thanks @amateescu for chiming in. Your expertise on this subject matter might prove invaluable!

This patch is already working with #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data based on your recommendation via Slack, the only thing missing are some tests.
Here I'm just including some fixes for the revisions tab and revisions reverting functionality, also included in #3029908: Add revisions tab on groups for easier review.

Hi @jidrone, I read in #14 the patch should not be applied in sites with created groups, in my case I used #9 patch and when I try to apply your last patch I have an error updating, the problem is the columns in group_revision table are different to those created by patch #9.

Do you think once this feature is complete will be a way to conciliate those sites with patch #9?

Note: I have updated the patch to fix the error I found when I try to apply #9 into last dev version, that way I will be able to use revisions.

Hi @fran seva,

The latest patch I uploaded, can be applied in sites with existing groups, because it support the update by using this #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data, for that reason I think is not possible to conciliate those patches because both process the update in a different way.

Added some improvements on revisions tab and some tests.

Fixed some comments in tests.

Hi Everyone,

Now that #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data is in the core from the beginning of this month, we can proceed to review a merge this patch, can someone help review the remaining 2 child issues?

Hello @jidrone,

I updated the hook_update ID and added a validation when a group hasn't a revision.

Hi Igor, can you please provide an interdiff.

Hello @jidrone,

I added the patch again with the interdiff file. I have a question: the Group Revisions works only the Content Moderation is enabled?

Sadly this is duplicating a ton of effort with #2850377: Have a group status field that affects access (which I've only skimmed, but is massively overlapping).

I don't want to mark either one duplicate until I can closely review to decide which approach seems more viable, but it should probably be that one that's marked dup (since it's newer). Would @jidrone and/or @igor.barato be willing to do a closer review and see if the approaches can be merged and #2850377 closed?

Just some food for thought:

I have recently learned how to write a business rule that allows me to work with inline entity forms within the group.

That is, I can add and remove referenced entities directly from the group edit form(s) now and the business rule assures that they get added to the group or removed from it as necessary.

This even works well with translations.

However, the revisions are a problem. For one thing, I'm not even sure what would be a "correct" behaviour when a group is rolled back. Would all the group entities also be rolled back? Do they even have the synchronised revisions? Should they?

I can see two scenarios that make sense here:

1) either all group entities roll back to the same version with the group.
2) Or an easy way is provided for people to work with versions of the group entities - e.g. from the group entities overview page.

Making new version for all group entities every time the group is updated may be interesting for small groups, but this would kill the server in case of big groups with many entities. So it's basically just the second option.

Hi everyone,

These are my thoughts for the last 2 comments:

#41:

This patch not only provides the status property for group entities, but it also adds the revisioning functionality.
The status property added in this case is the standard boolean property needed for all revisionable entities so this is the right way to add this functionality.
Regarding a possible duplicated effort, you can see this comment https://www.drupal.org/project/group/issues/2873212#comment-12951819 there is a similar concern and my answer was that I prefer first to cover the standard status functionality and then we can provide a feature to restrict the group content access based on that.

#42:

This is a similar situation than using paragraphs in groups, it uses an entity reference revisions field typed provided by Entity Reference Revisions which makes possible to generate the relation with the revision, there is an issue related to that #3025709: "Create new revision" option is ignored when updating EntityReferenceRevisionsItem.
In my opinion, if the entities are rendered within the group entity view(like paragraphs) they should generate a new revision of the parent group when they are changed.

Hm, that's an interesting idea. I tried out using the inline entity form with entity reference revision field, but that didn't work. I can try it again...

Hm, that's an interesting idea. I tried out using the inline entity form with entity reference revision field, but that didn't work. I can try it again...

#38 no longer applies to 8.x-1.0-rc5 (current head)

$ patch -p 1 < group-revisions-2829966-38.patch
patching file config/schema/group.schema.yml
patching file group.group.permissions.yml
patching file group.install
Hunk #1 FAILED at 10.
Hunk #2 succeeded at 454 with fuzz 2 (offset -35 lines).
1 out of 2 hunks FAILED -- saving rejects to file group.install.rej
patching file group.links.task.yml
patching file group.post_update.php
Hunk #1 succeeded at 9 (offset 1 line).
Hunk #2 succeeded at 61 with fuzz 2 (offset 14 lines).
patching file group.routing.yml
patching file group.services.yml
Hunk #1 succeeded at 33 with fuzz 2.
Hunk #2 succeeded at 98 (offset 7 lines).
patching file src/Access/GroupLatestRevisionCheck.php
patching file src/Access/GroupRevisionAccessCheck.php
patching file src/Entity/Access/GroupAccessControlHandler.php
patching file src/Entity/Controller/GroupController.php
Hunk #5 succeeded at 167 with fuzz 1 (offset 15 lines).
patching file src/Entity/Controller/GroupListBuilder.php
Hunk #1 succeeded at 170 with fuzz 2 (offset 14 lines).
patching file src/Entity/Form/GroupRevisionDeleteForm.php
patching file src/Entity/Form/GroupRevisionRevertForm.php
patching file src/Entity/Form/GroupRevisionRevertModeratedForm.php
patching file src/Entity/Form/GroupTypeForm.php
patching file src/Entity/Group.php
patching file src/Entity/GroupInterface.php
patching file src/Entity/GroupType.php
patching file src/Entity/GroupTypeInterface.php
patching file src/Entity/Storage/GroupStorage.php
patching file src/Entity/Storage/GroupStorageInterface.php
patching file src/Routing/LatestRevisionRouteSubscriber.php
patching file tests/modules/group_test_config/config/install/group.type.default.yml
patching file tests/src/Functional/EntityOperationsTest.php
patching file tests/src/Functional/GroupAccessTest.php
patching file tests/src/Functional/GroupRevisionableTest.php

This is a manual re-roll with an interdiff-38-46.txt file containing only the hunks that succeeded (i.e. this interdiff-38-46.txt is technically incomplete) along with a pseudo-interdiff-38-46.txt which contains a more accurate (but less readable) output from:

diff group-revisions-2829966-38.patch group-revisions-2829966-46.patch > pseudo-interdiff-38-46.patch

Things that I've changed are superficial:

• Fixed order of use to be alphabetical.
• Bumped the hook_update_N to group_update_8018
• Added single quotes around strings in some of the YML files, to match the formatting used in the surrounding code, eg group.links.task.yml and group.services.yml

Whoops. Problem with manual rerolls is they're hard to get right, even with interdiffs.

This one fixes a few whitespace and syntax errors introduced accidentally in #46.

Also let's hide a bunch of old patches at the top of the issue.

It's a bit sad to see giant patches still being posted here, instead of the smaller, easier to review patches in the child issues. Can we please stop doing that and focus our collective efforts on the child issues, instead?

Thanks!
-Derek

@Dww: The reason is many fold. But primarily, the problem working in the sub-tickets is that the patches are not compatible, and cannot be applied in succession on a codebase, because they generate conflicts.

For example, merging 3029907-group-make-revisionable onto 2873212-group-status creates the following conflicts that must be resolved manually:

diff --cc group.group.permissions.yml
index 5a56fe2,625a3ec..0000000
--- a/group.group.permissions.yml
+++ b/group.group.permissions.yml
@@@ -4,11 -4,10 +4,18 @@@ administer group
    description: 'Administer the group, its content and members'
    restrict access: TRUE
  view group:
++<<<<<<< HEAD
 +  title: 'View published group'
 +view any unpublished group:
 +  title: 'View any unpublished group'
 +view own unpublished group:
 +  title: 'View own unpublished group'
++=======
+   title: 'View group'
+ view group latest version:
+   title: 'View the latest version'
+   description: 'Requires the "View any unpublished group" or "View own unpublished group" permission'
++>>>>>>> 3029907-3-group-make-revisionable
  edit group:
    title: 'Edit group'
    description: 'Edit the group information'
diff --cc src/Entity/Group.php
diff --cc src/Entity/Group.php
index f0727b7,e2bf93f..0000000
--- a/src/Entity/Group.php
+++ b/src/Entity/Group.php
@@@ -55,7 -56,12 +58,16 @@@ use Drupal\user\StatusItem
   *     "langcode" = "langcode",
   *     "bundle" = "type",
   *     "label" = "label",
++<<<<<<< HEAD
 + *     "published" = "status"
++=======
+  *     "revision" = "revision_id",
+  *   },
+  *   revision_metadata_keys = {
+  *     "revision_user" = "revision_user",
+  *     "revision_created" = "revision_created",
+  *     "revision_log_message" = "revision_log_message",
++>>>>>>> 3029907-3-group-make-revisionable
   *   },
   *   links = {
   *     "add-form" = "/group/add/{group_type}",
diff --cc src/Entity/GroupInterface.php
index d3b445d,233a7c0..0000000
--- a/src/Entity/GroupInterface.php
+++ b/src/Entity/GroupInterface.php
@@@ -7,14 -7,14 +7,22 @@@ use Drupal\Core\Entity\ContentEntityInt
  use Drupal\Core\Entity\EntityChangedInterface;
  use Drupal\Core\Session\AccountInterface;
  use Drupal\user\UserInterface;
++<<<<<<< HEAD
 +use Drupal\Core\Entity\EntityPublishedInterface;
++=======
+ use Drupal\Core\Entity\RevisionLogInterface;
++>>>>>>> 3029907-3-group-make-revisionable
  
  /**
   * Provides an interface defining a Group entity.
   *
   * @ingroup group
   */
++<<<<<<< HEAD
 +interface GroupInterface extends ContentEntityInterface, EntityOwnerInterface, EntityChangedInterface, EntityPublishedInterface {
++=======
+ interface GroupInterface extends ContentEntityInterface, EntityOwnerInterface, EntityChangedInterface, RevisionLogInterface {
++>>>>>>> 3029907-3-group-make-revisionable
  
    /**
     * Gets the group creation timestamp.

I'm in the process of rebasing all three sub-issues on latest 8.x-1.x branch, since all need rerolls. Then, I'm going to also reroll patch in #47 on latest 8.x-1.x branch and manually compare the differences between each and then push down any changes that I made above into the sub-issues, if there are any that make sense.

I've found a number of differences between the sub-tickets and the latest patch here. Aside from formatting issues and some minor code order changes that have no effect on functionality, there are some things that have been worked on here that have not ended up in the sub-tickets, including tests and a few smaller chunks of code.

Due to aforementioned conflicts with the sub-issues, I've recreated the patch on this issue to be based on merges with manual conflict resolution of the three sub-issues into a single branch using the following:

$ git checkout 8.x-1.x

$ git checkout -b 2829966-subticket-merge-2873212-3029907-3029908

$ git merge 2873212-group-status --no-ff
Merge made by the 'recursive' strategy.
 group.group.permissions.yml                     |  6 +++++-
 group.install                                   | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/Entity/Access/GroupAccessControlHandler.php | 15 ++++++++++++++-
 src/Entity/Group.php                            | 25 ++++++++++++++++++++++++-
 src/Entity/GroupInterface.php                   |  3 ++-
 tests/src/Functional/GroupAccessTest.php        | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 191 insertions(+), 4 deletions(-)
 create mode 100644 tests/src/Functional/GroupAccessTest.php

$ git merge 3029907-group-make-revisionable 
Auto-merging src/Entity/GroupInterface.php
CONFLICT (content): Merge conflict in src/Entity/GroupInterface.php
Auto-merging src/Entity/Group.php
CONFLICT (content): Merge conflict in src/Entity/Group.php
Auto-merging group.group.permissions.yml
CONFLICT (content): Merge conflict in group.group.permissions.yml
Automatic merge failed; fix conflicts and then commit the result.

[manual fix conflicts]

$ git add group.group.permissions.yml src/Entity/Group.php src/Entity/GroupInterface.php
$ git commit 

$ git merge 3029908-group-revisions-tab 
Auto-merging src/Entity/Group.php
Auto-merging group.services.yml
CONFLICT (content): Merge conflict in group.services.yml
Auto-merging group.group.permissions.yml
CONFLICT (content): Merge conflict in group.group.permissions.yml
Automatic merge failed; fix conflicts and then commit the result.

[manual fix conflicts]

$ git add group.services.yml group.group.permissions.yml
$ git commit

$ git diff 8.x-1.x..2829966-subticket-merge-2873212-3029907-3029908 > group-revisions-2829966-51--merged-2873212-26-3029907-15-3029908-8.patch

@dww, you'll also note on the sub-issues that splitting this issue apart breaks tests that were added in this issue (because one issue requires another).

All three sub-issues are so intertwined that it really makes no sense to pull them apart except literally to make it easier to review. Those sub-issues should not be merged and cannot even be used to write tests.

All this complexity around the issue is going to make it super challenging to get this issue committed in the first place. *sigh*.

To explain a few of the interdependencies:

1. Revision-based permissions in #3029907 depend on status permissions in #2873212.
('View the latest version' 'Requires the "View any unpublished group" or "View own unpublished group" permission')
2. But also 3029907 doesn't apply cleanly after applying 2873212.

3. Revision tests in 3029908 depend on code in 3029907.
(Exception: Error: Call to undefined method Drupal\group\Entity\GroupType::isNewRevision())
4. But also 3029908 doesn't apply cleanly after applying 3029907.

Bumped patch from sub-ticket #2873212 comment #27 to fix syntax error.

Edit: you'll note that tests that don't execute in the sub-tickets run fine once merged together here.

Attached the same patch as #53 with the group_update changed to allow for patching 8.x-1.1.

Here's a reroll for latest , small changes with #54's patch, applies cleanly at 6bc592d4

Given the order jwilson3 gave at #52 (2873212->3029907->3029908) of applying the child patches is correct, I have changed the latest ones on the childs in order to be applied cleanly. I will test them more on the dev and upload them to their issues soon

I have made small changes to allow the patch to work on D9.
Applies at 9b469da.

NW. One of the sub-issues has an RTBC patch that has not been incorporated here yet, see patch on comment #36 of #2873212-36: Add a status to the group (Publish/Unpublish).

Here it is. let's see the tests

Sorry, wrong naming on the patch

Let's try again

Those failures on #62 seem spurious. Not sure what that is on about.

But also: an interdiff between #56 and #62 would be helpful given the size of the patch, and would also allow us to compare with the interdiff on #2873212-36: Add a status to the group (Publish/Unpublish).

The patch on #62 had some issues and are fixed on this patch. Maybe this is the problem for the test fails. Crossing fingers

I made the interdiff using patchutils and it mainly shows some code-formatting changes plus the change on https://www.drupal.org/comment/13913576#comment-13913576

These fails are not relevant to the patch. I think I found the issue. At 1.3 the failing test was added.

Since #2873212: Add a status to the group (Publish/Unpublish) is committed, we need to consider changing this patch. Are we splitting this into three tickets, or continuing working on this?

All of the child issues are fixed now. Just waiting for a few reviews.

Small notes:

• None of this work considered EntityModerationForm. In order to make that one only show up when wanted, we need to bridge the Workflows module too. (Which was out of scope for the sponsored work).
• Currently, nothing happens on user cancel/delete, that might need to change also but want to hear from our client first.

The patch #65 does not apply to v1.4 anymore, as far as I can see in the moment it is because of the applied child issues.

I think this feature is already released. Need to close this ticket.

Yeah, as this was an issue that pasted the patches from all 3 child issues together, I forgot to close it. I can add credit for people who contributed here to one of the child issues if they haven't received credit in any of those three yet. Just let me know if that's the case for you and then we can circle back and close this one.