Content moderation integration: transition permissions and latest version access

Updated patch for latest Drupal 8.4 Core update

The last patch fixed the issue I had with 8.4 but didn't apply for me. Here's a fixed version and an interdiff.

I get a fatal error with this patch on this page, admin/group/types/manage/mygroup-type/permissions:

Error: Call to undefined method Drupal\workflows\Entity\Workflow::getTransitions() in Drupal\group\Plugin\GroupContentEnablerBase->getGroupModerationPermissions() (line 269 of drupal-8.4.0/html/modules/contrib/group/src/Plugin/GroupContentEnablerBase.php

getTransitions() is defined here:
core/modules/workflows/src/State.php:98
core/modules/workflows/src/StateInterface.php:74
core/modules/content_moderation/src/ContentModerationState.php:110

Ran into the same issue as SocialNicheGuru above, patch and interdiff attached.

Awesome to see this work started! We'll also need to modify how content moderation handles access to the latest version tab.

Some initial thoughts on the patch:

+++ b/src/GroupServiceProvider.php
@@ -0,0 +1,22 @@
+  public function alter(ContainerBuilder $container) {
+    // Hijack the state_transition_validation service to run our own access checks.
+    $definition = $container->getDefinition('content_moderation.state_transition_validation');
+    $definition->setClass(StateTransitionValidation::class);

This will make Group incompatible with other modules that need to override this. Instead of hijacking the service, it would be good to utilize a decorator here. See workflow_participants.services.yml and StateTransitionValidate for an example.

I'll see how far I can get on these remaining issues.

After some initial research, I think it will make more sense to split the latest revision access override into a separate issue. I've added #2935786: Content moderation integration: latest revision access override, and re-titled this one.

It isn't actually possible to separate latest version access out from this, so I've closed #2935786: Content moderation integration: latest revision access override out as a duplicate and will try and tackle this issue.

This patch utilizes service decorators to achieve the ability to override content moderation's access checks at the group level. I've also added some tests, including one that verifies compatibility with other modules that might also be decorating these services.

Oops, clearly needs some work on making these decorations conditional.

It turns out that decorated services cannot be optional, so this changes the approach to utilize GroupServiceProvider::alter() to decorate the content moderation services when that module is enabled.

The last patch had an unnecessary change to group.services.yml causing it to not cleanly apply to rc1.

Small issue with the title arguments not being replaced on the permissions form.

1. +++ b/tests/modules/group_test_content_moderation/group_test_content_moderation.services.yml
   @@ -0,0 +1,10 @@
   +services:
   

   Since the real decorators were changed to be applied by the ServiceProvider due to the conditional troubles, should these test ones also be moved to mimic that same approach?

2. +++ b/tests/src/Functional/ContentModerationIntegrationTest.php
   @@ -0,0 +1,141 @@
   +    // The group member should have not have access.
   

   Comment phrasing: The group member should have access, if we're checking for 200s.

Since the real decorators were changed to be applied by the ServiceProvider due to the conditional troubles, should these test ones also be moved to mimic that same approach?

Since the test module can require content_moderation I think it's clearer to utilize the service YAML file. The tests also ensure that both decorators are applied, so we know that core's service registration is working either way.

This patch fixes the typo mentioned in #22, and caught another one :)

Wow, for some reason #2927746: Update Symfony components to 3.4.* broke this approach. I'm digging in now to figure out why.

The failures were due to services now defaulting to private in Symfony 3.4. I left a comment on #2927746-93: Update Symfony components to 3.4.*.

Wow, perfect timing. :)

I haven't manually tested this out so I won't mark it RTBC, but I've done a full code review, so from that perspective RTBC+1.

I'm about to review this, but I could see this as being useful for group_membership group content where there are states and a workflow that ties into whether a member is considered an outsider or not (Pending/Banned) even if they have a membership relationship to the group.

I have essentially re-written https://www.drupal.org/project/group/issues/2752603 with configuration entities similar to roles as states for memberships but Core Drupal 8 content moderation would be favorable IMO, and I am considering abandoning that approach.

@jhedstrom, i found a minor issue in this patch, when a content is created in a group, the StateTransitionValidation is not merging the global permissions with the group level permissions.

Submitting a patch for the same with an interdiff.

Seems the test coverage is missing the usecase with adding a content within a group, will add tests for this shortly.

Thanks
Sukanya

@sukanya.ramakrishnan good find! We'll need to update the tests to catch this, but the fix looks solid.

I tested the patch at #30 and it looks solid. It did the job as expected with no noticeable errors.

Thank you!

The tests are no longer passing against with the latest core (8.6.x) so some api's must have changed. Config import for GroupTypeImportTest::testImport no longer works but that is not related to this issue.

The patch in #30 isn't working for me. I still just get the save button without a moderation state selector on group/1/content/create/group_node:foo when I enable moderation for a content type and set the permissions for the user in group permissions. I haven't tried to debug it yet. Maybe tomorrow.

I was wondering if this functionality should be a sub-module? Maybe group_content_moderation?

Re-roll fixes the current set of tests.

The getPluginId() method of this patch assumes a derivative plugin with bundles but that may not be true. I have a custom entity plugin which is generic to all bundles. My group uses this plugin with moderation states. I'm attaching a patch which looks up the plugin ID with the content enabler manager. It prioritizes a bundle plugin then falls back to a generic one then back to just returning FALSE.

As a side note, this is the first I've seen of Symfony service decorators. I'm sure I'll use this patch as a reference for them in the future.

Here's an updated patch for the 8.6.2 security update. It adds an isTransitionValid() method to StateTransitionValidationInterface which causes the previous patch to crash the site. I added an implementation of that method.

StateTransitionValidationInterface::isTransitionValid() doesn't pass in the current entity so this patch fetches it from the route. Not 100% sure that will work in all cases....

FYI, I've opened a core issue because of that change.
#3007716: Security update introduces breaking changes to content moderation

Based on the current direction of the core issue, here is a revised patch. Since this is a different approach from #37, the interdiff is based on #36.

Also there's some test tweaks to fix a problem with the patch applying and minor documentation fixes to the plugin ID changes from #36.

Oops, apparently there was also an implementation in a test.

Oops, that doesn't quite work. Since it is called within the context of entity validation, the entity's moderation state is actually the new one, not the current one, which borks up the list of valid transitions. This version ensures that the current moderation state is actually used.

Hi, I think this code won't be compatible with php 7.2. You cannot add an argument to an inherited method's signature.
As explained in this comment https://www.drupal.org/project/drupal/issues/2927173#comment-12363396 you will get a fatal error in php 7.2

Ok never mind, adding arguments seems to be permitted as long the next class in line to inherit doesn't remove the new argument in it's own signature. I am leaving my comment online so it can be corrected if I'm wrong.

So I hadn't figured out that patch 43 only workded with the patch of Drupal Core in the other issue. I connected the dots in my head and tested #43. It is working nicely!

Thanks for your work ;)

Really nice work and support (BC breaks around content_moderation are really a pain)

I have updated the patch so that for new entities, the default moderation state is used rather than that of the validated entity that is passed into StateTransitionValidation::isTransitionValid() as that will be using the new state.

+++ b/src/Access/LatestRevisionCheck.php
@@ -0,0 +1,139 @@
+class LatestRevisionCheck implements AccessInterface {

+++ b/src/GroupServiceProvider.php
@@ -0,0 +1,49 @@
+      $latest_revision_definition = new Definition(LatestRevisionCheck::class, [
+        new Reference('group.latest_version_access.inner'),
+        new Reference('entity_type.manager'),
+      ]);
+      $latest_revision_definition->setPublic(TRUE);
+      $latest_revision_definition->setDecoratedService('access_check.latest_revision');

Per #3035113: Replace content moderation specific access check in JSON:API with generic revision access check, please make this patch subclass Drupal\content_moderation\Access\LatestRevisionCheck. That would signal that they provide a customized version of the same behavior and provide the same guarantees.

(If you want to know why the JSON:API module is disallowing AccessInterface implementations and requiring that particular class — or subclasses thereof, see #3035113-3: Replace content moderation specific access check in JSON:API with generic revision access check and #3035113-6: Replace content moderation specific access check in JSON:API with generic revision access check.)

With 8.7.x I've been encountering an issue where nodes are always saved as draft when using this patch. This seems to be happening specifically because of presave validation that traces back to layout_builder_entity_presave(), so it may be layout builder specific.

The issue is that Drupal\group\StateTransitionValidation::isTransitionValid() sets the entity state like this:

      // As this may be occurring during validation, the moderation state on the
      // entity may be the new state, rather than the current state, so make
      // sure we're working with the current version.
      if ($entity->isNew()) {
        $inital_moderation_state = $workflow->getTypePlugin()->getInitialState($entity);
        $entity->set('moderation_state', $inital_moderation_state->id());

        $original_entity = $entity;
      }

but then it never resets it. Previously this was fine (I guess because it never saved the entity) but since it's getting invoked in presave in this case it actually changes the state in the process of checking whether it's valid. I've attached a patch which remembers and resets the state after it has finished the check.

The issue noted by Wim Leers above remains so I'm leaving this at "Needs Work".

This is a great patch and almost gets me where I need to go. I would also like to add the ability to View, Edit, Delete group nodes in a specific Workflow State.

ex: Group node (CT) - "Edit" Latest Version in transition state "Draft" workflow.

Essentially making it where a user must be a member of a group, have an editor user role, and the node within the group must be in a specific workflow state in order for that user to make edits. Currently Workflow_Access and Group Permissions appear to override each other.

Has anyone tried this additional level of granular permission control?

In using patch #49, I got the following error when trying to clear the caches with Drush:

PHP Fatal error: Uncaught TypeError: Argument 1 passed to Drupal\jsonapi\Access\EntityAccessChecker::setLatestRevisionCheck() must be an instance of Drupal\content_moderation\Access\LatestRevisionCheck, instance of Drupal\group\Access\LatestRevisionCheck given in /var/www/html/group-dev/htdocs/docroot/core/modules/jsonapi/src/Access/EntityAccessChecker.php:150
Stack trace:
#0 [internal function]: Drupal\jsonapi\Access\EntityAccessChecker->setLatestRevisionCheck(Object(Drupal\group\Access\LatestRevisionCheck))
#1 /var/www/html/group-dev/htdocs/docroot/core/lib/Drupal/Component/DependencyInjection/Container.php(329): call_user_func_array(Array, Array)
#2 /var/www/html/group-dev/htdocs/docroot/core/lib/Drupal/Component/DependencyInjection/Container.php(502): Drupal\Component\DependencyInjection\Container->createService(Array, 'jsonapi.entity_...')
#3 /var/www/html/group-dev/htdocs/docroot/core/lib/Drupal/Component/DependencyInjection/Container.php(237): Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters in /var/www/html/group-dev/htdocs/docroot/core/modules/jsonapi/src/Access/EntityAccessChecker.php on line 150
[warning] Drush command terminated abnormally. Check for an exit() in your Drupal site.

It looks like Group's instance of LatestRevisionCheck is being passed to those that need it, but it isn't the instance they are expecting, and it is throwing an error.

We've been using the patch at #47 for sometime now and the update to #49 seems to have kept things working also.

We had an issue with workflow transitions though. It turned out that the moderation_state that was used in $this->getValidTransitions() was not working for us. We have a number of transitions and they're not all available to all users. We were finding that the moderation_state that was being used was the value of the state we were transitioning to, rather than the state that was currently there. This resulted in the wrong transitions being returned.

This addition to the patch appears to fix this. In our case we were also working with translations and the node being worked on was actually a translation rather than the original default language. This feels like we should be working with the translated node to me, but that would be the $entity that was passed in, isn't it? With the way this part of the code is being used the $original_entity is only used for lookup purposes and is not returned or passed in by reference.

I'm not 100% on this but it seems to work. More eyeballs on this would be appreciated.

I will leave this in a Needs Work state as it doesn't address the points in note #50.

If I can find the time (and figure out how) I'll add tests for this later.

Additional: I have been unable to replicate the issue at #52

#52 - this patch will help - https://www.drupal.org/project/drupal/issues/3035113#comment-13092187

I had the same problem with moderated content in groups that is multilingual as @Gold but handled it a little different.
I am not setting anything in the moderation, I just make sure that the right revision with the right language is loaded.

Because here is exactly this problem. In my, and @Gold's case, the $original_entity is loaded with the default language, and in case we are editing a different language in the $original_entity->moderation_state->value we get the value from the default language.

My patch is practically exactly like #49 and #53 with small differences.

Patch 55 is working for me - in that it allows me to manage content moderation by group rather than a site-wide permission role.
it also gives my users the Latest Revision button to access unpublished drafts... but the Revisions tab that lists all the revisions of a node is missing. it appears that permission to View revisions by content type is still only in site permissions.

I would love to see that permission moved into groups as well.

With patch #55 I see content moderation related permissions (ex.: XY group content - Use Create New Draft transition from YZ content moderation workflow.) in group level, even if the XY group content is not part of the YZ content moderation workflow.

The direction of this patch overrides the StateTransitionValidation class provided by core. In so doing it is causing an ajax error with the contributed module Workflow Buttons which is unfortunate.

I'm thinking this won't be the only breakage in the moderation space if this particular implementation continues.

I have rerolled the patch from #55 to reference the original core validation class being overridden in this issue. Additionally I have renamed the group module's version of the validation class to prevent confusion.

I have tested the patch from #59 it seems to work but I'm gonna leave this open if anyone else wants to review it.

RTBC +1

I further reroll against current dev.

Can I request some clarifications on "how to" for this thread, specifically the patch #61.

I applied the patch, did cache rebuild and rebuilt permissions, however, I don't see any new permissions in the group_type/permissions or people/permissions.
It was applied on Group 8.x-1.x-dev, and I already had subgroups core patch applied on it.
I read through the comments and other patches. And also went through the workflow config etc.

Do I need to configure something or am I missing something obvious?

Thanks in advance

EDIT: to clarify, Group Node is enabled and my Group Type has Content Type with a workflow enabled. The group cardinality with the content type is 1.

@ggj I have lost some lines in my re-rolling. I have just come back to this.

+++ a/src/Plugin/GroupContentEnablerBase.php
@@ -348,10 +306,6 @@
-      // @TODO: Dependency injection.
-      if (\Drupal::moduleHandler()->moduleExists('content_moderation')) {
-        $permissions += $this->getGroupModerationPermissions();
-      }

There have been a lot of changes in Group permissions and this works will be a Group v1 option and will need updating for Group v2
I'll update with a new patch.

So even adding the permission provider back in the GroupContentProviders have been updated.

It would be possible to add something into GroupContentPermissionProvider that would add workflow permissions for Entities enabled as group content.

However, this would not cover other entities that might have content moderation- e.g. files/images that are referenced on content added to groups.

I'm thinking there may need to be a setting to add a workflow to a group/s so you can see its permissions; which would be better than just adding all workflows to all group if they are relevant or not.

Rebuilt as a sub-module gcontent_moderation with a Group permission_callbacks

Content moderation permissions aren't linked to the Content Plugin directly. So if you add more than one content type to a group and they have the same moderation workflow; the permission will apply across both content types.

Fixing tests and coding standards
Also, use GroupRouteContextTrait

Further fixes to test

Tests don't install dependencies so include content_moderation in the list.

Tests also need to have the test module enabled

Renamed the decorator services which fixes the issue in the test and made two functions in the service public as I am decorating this service in my module and want to use

$this->inner->allowedTransitions($user, $entity, $groups)

The patch uses getInitialState() incorrectly. The actual entity should be passed, not the workflow. Corrected patch attached.

On a brand new site install with Group 1.1 and the patch from 74 installed, I get this error block:

The website encountered an unexpected error. Please try again later.
TypeError: Argument 1 passed to Drupal\jsonapi\Access\EntityAccessChecker::setLatestRevisionCheck() must be an instance of Drupal\content_moderation\Access\LatestRevisionCheck, instance of Drupal\gcontent_moderation\Access\LatestRevisionCheck given in Drupal\jsonapi\Access\EntityAccessChecker->setLatestRevisionCheck() (line 150 of core/modules/jsonapi/src/Access/EntityAccessChecker.php).

Drupal\jsonapi\Access\EntityAccessChecker->setLatestRevisionCheck(Object)
call_user_func_array(Array, Array) (Line: 276)
Drupal\Component\DependencyInjection\Container->createService(Array, 'jsonapi.entity_access_checker') (Line: 449)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array) (Line: 237)
Drupal\Component\DependencyInjection\Container->createService(Array, 'jsonapi.include_resolver') (Line: 173)
Drupal\Component\DependencyInjection\Container->get('jsonapi.include_resolver', 1) (Line: 434)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array) (Line: 237)
Drupal\Component\DependencyInjection\Container->createService(Array, 'jsonapi.entity_resource') (Line: 173)
Drupal\Component\DependencyInjection\Container->get('jsonapi.entity_resource') (Line: 20)
Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition('jsonapi.entity_resource') (Line: 107)
Drupal\Core\Entity\EntityResolverManager->getControllerClass(Array) (Line: 219)
Drupal\Core\Entity\EntityResolverManager->setRouteOptions(Object) (Line: 48)
Drupal\Core\EventSubscriber\EntityRouteAlterSubscriber->onRoutingRouteAlterSetType(Object, 'routing.route_alter', Object)
call_user_func(Array, Object, 'routing.route_alter', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('routing.route_alter', Object) (Line: 189)
Drupal\Core\Routing\RouteBuilder->rebuild() (Line: 83)
Drupal\Core\ProxyClass\Routing\RouteBuilder->rebuild() (Line: 344)
Drupal\Core\Extension\ModuleInstaller->install(Array, 1) (Line: 83)
Drupal\Core\ProxyClass\Extension\ModuleInstaller->install(Array) (Line: 173)
Drupal\system\Form\ModulesListConfirmForm->submitForm(Array, Object)
call_user_func_array(Array, Array) (Line: 114)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers(Array, Object) (Line: 52)
Drupal\Core\Form\FormSubmitter->doSubmitForm(Array, Object) (Line: 593)
Drupal\Core\Form\FormBuilder->processForm('system_modules_confirm_form', Array, Object) (Line: 144)
Drupal\autosave_form\Form\AutosaveFormBuilder->processForm('system_modules_confirm_form', Array, Object) (Line: 321)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 97)
Drupal\autosave_form\Form\AutosaveFormBuilder->buildForm(Object, Object) (Line: 91)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 573)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 151)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 67)
Drupal\simple_oauth\HttpMiddleware\BasicAuthSwap->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 708)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

It seems this has already been reported and the core patch does solve it, but wondering if there will be an issue with Drupal 9. I'm on 8.9.2 currently.

We are currently using this patch on Group RC1 and its works. Though Groups 1.2 available now, is there any patch available for the new release or is it moved already?

We installed the latest version of 'Group' and got issues with PATCH #61. What versions does it work with?

We want to create Group permissions for transitions. The user can be in different groups with a different role, so having transition permissions per role at the user level does not work. We want to silo each group where a user can have different roles in different groups.

this patch adds group permissions for using workflow transitions per node type.

content_moderation.state_transition_validation is renamed as gcontent_moderation.state_transition_validation

@oktboy What issues exactly do you face? What you describe is exactly what this patch is supposed to do. (I'm still using RC5 and #43 and haven't tested newer group/patch versions yet)

this patch adds group permissions for using workflow transitions per node type.

@amitch Isn't this what the patch already did (at least #43 did)? What exactly did you change and why?

@kristiaanvandeneynde Is this something you would want to add to group module or do you think this should live in a seperate module? As per #2984315-56: Media enabler

@arosboro & @kristiaanvandeneynde I think this should be part of the core Group module as Content Moderation is part of Core Drupal (and goes with gnode being part of core Group).

I would be tempted to use different workflows if they have they have different access requirements - this aligns close with core permissions I think (unless like @amitch's patch it just does it for nodes, I'm using it on my own custom entities)

Has any of these patches been tested with 1.3?

So I have a question, I've written my own module which models how content moderation works, all using core workflows and the workflows_field module. Would this addition require that I have content moderation installed and in use or does it integrate with core workflow transitions and states?

Right now, I have this all working by giving group roles a matching system role, which gives them access to the transition permissions. Obviously a fragile system, but it fit the bill for a limit use-case.

@shelane I'm using it with 1.3 with no apparent ill-effects.

@arosboro & @kristiaanvandeneynde I think this should be part of the core Group module as Content Moderation is part of Core Drupal (and goes with gnode being part of core Group).

Well, menus are also part of core, yet Group Menu is a separate module :) So is Subgroup even though "Group" is part of Group :D
That said, I'm probably going to reviewing this Soon™ to see whether it needs to go into Group or a a separate module.

Drupal 8.9.10, Group 1.3.0

#80 threw an error when I tried to enable the gcontent_moderation submodule.

#79 seemed to work at first but after enabling some permissions I got this error when trying to clear caches:

PHP Fatal error: Uncaught TypeError: Argument 1 passed to Drupal\jsonapi\Access\EntityAccessChecker::setLatestRevisionCheck() must be an instance of Drupal\content_moderation\Access\LatestRevisionCheck, instance of Drupal\gcontent_moderation\Access\LatestRevisionCheck given in /Users/kasey/Sites/devdesktop/uua-org-d8/web/core/modules/jsonapi/src/Access/EntityAccessChecker.php:150

I currently have #74 with group 1.3. We previously had #55 with 1.0-rc5. In both cases we discovered that users who should not have permission to a transition (i.e., publish) were still able to see that option in the content moderation selection and apply it. I tested by both the content moderation that displayed on the detail page and on the edit page. So in this screen shot, the role (name is not shown in the image) "Review" should not have access to "Use Publish transition" on either the editorial workflow or the simple workflow. However, they can anyway.

BTW, I also tried with #79 and it was the same.

@shelane: which global permissions do you have set? Do they make a difference?

Yes, globally there is one role for the users. That is why I use groups to manage roles and permissions because it is their role within the group that should determine their ability to create or publish content. So with that, that role globally has permissions for the transition. If that is off, they have no permissions for the transitions.

@azinck, I got that you're using 1.3 of groups, but which patch # are you using without issue? Also, which version of Drupal?

@anyone: which version of the patch works with Groups 1.3 & Drupal 9?

I have tested #80 with Group 8.x-1.3 and the permissions were correctly enforced. Although, the "Latest version" tab was missing and then I realised that the LatestRevisionCheck class was checking for the wrong permission name.

Uploading new patch and interdiff for review.

Setting back to "Needs Work" because of failed test with latest patch.

@veperr For now i'm having a nice experience with group 1.3 drupal 9.1.x and patch #95
I've checked my basic use cases and no trouble for now
I was using before with success drupal 8.9.x, group-rc-1.5 and patch #43
Hope this help for anyone

By the way, this module is awesome and a great inspiration

Thanks

Great work on this feature! Looks like it's working quite well.

I've been thinking a bit about the question raised in #84 by anruether:

@kristiaanvandeneynde Is this something you would want to add to group module or do you think this should live in a seperate module? As per #2984315-56: Media enabler

I think this feature should be a standalone module.

Even though this functionality is in Drupal Core, it doesn’t have to be in the Group module for that reason. For example Group Menu is also not in the Group module.
One of the things kristiaanvandeneynde mentioned in the comment is that he likes to keep the Group module lean. And considering this is already proposed as a submodule, wouldn't it just be better to move it into a standalone module?

I assume kristiaanvandeneynde also has only limited time to contribute to this or move it forward. Having this in a separate module might also speed up a release.

I also have some additional patches to deliver for this module, including support for a "Moderated content" tab on a Group with all the correct filters and the "View own unpublished entity" permission. I think it might be best to create a contrib project for this module. It might also help with moving additional features forward and keep better track of patches and issues.

Works for me. Thanks!

* #95 Works for me. Thanks!

I am using Drupal 9.1.4 and Group module 8.x-1.3. When using the group-content_moderation-2906085-95.patch I get the following error after enabling the "Group Content Moderation" module:

The website encountered an unexpected error. Please try again later.
TypeError: Argument 1 passed to Drupal\jsonapi\Access\EntityAccessChecker::setLatestRevisionCheck() must be an instance of Drupal\content_moderation\Access\LatestRevisionCheck, instance of Drupal\gcontent_moderation\Access\LatestRevisionCheck given in Drupal\jsonapi\Access\EntityAccessChecker->setLatestRevisionCheck() (line 150 of core/modules/jsonapi/src/Access/EntityAccessChecker.php).
Drupal\jsonapi\Access\EntityAccessChecker->setLatestRevisionCheck()
call_user_func_array() (Line: 274)
Drupal\Component\DependencyInjection\Container->createService() (Line: 447)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters() (Line: 235)
Drupal\Component\DependencyInjection\Container->createService() (Line: 171)
Drupal\Component\DependencyInjection\Container->get() (Line: 432)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters() (Line: 235)
Drupal\Component\DependencyInjection\Container->createService() (Line: 171)
Drupal\Component\DependencyInjection\Container->get() (Line: 20)
Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition() (Line: 99)
Drupal\Core\Entity\EntityResolverManager->getControllerClass() (Line: 212)
Drupal\Core\Entity\EntityResolverManager->setRouteOptions() (Line: 48)
Drupal\Core\EventSubscriber\EntityRouteAlterSubscriber->onRoutingRouteAlterSetType()
call_user_func() (Line: 142)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch() (Line: 189)
Drupal\Core\Routing\RouteBuilder->rebuild() (Line: 83)
Drupal\Core\ProxyClass\Routing\RouteBuilder->rebuild() (Line: 348)
Drupal\Core\Extension\ModuleInstaller->install() (Line: 83)
Drupal\Core\ProxyClass\Extension\ModuleInstaller->install() (Line: 473)
Drupal\system\Form\ModulesListForm->submitForm()
call_user_func_array() (Line: 113)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers() (Line: 51)
Drupal\Core\Form\FormSubmitter->doSubmitForm() (Line: 593)
Drupal\Core\Form\FormBuilder->processForm() (Line: 321)
Drupal\Core\Form\FormBuilder->buildForm() (Line: 73)
Drupal\Core\Controller\FormController->getContentResult()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 573)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 158)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 80)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 57)
Drupal\Core\StackMiddleware\Session->handle() (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 23)
Stack\StackedHttpKernel->handle() (Line: 706)
Drupal\Core\DrupalKernel->handle() (Line: 19)

After discussing this on Slack with @kristiaanvandeneynde it seems this is a good candidate to move to a separate contrib module.

As pointed out by Kristiaan: The idea in this case for the plugin handlers is to declare them as a service, and instead of pointing it to a class in the annotation, we can point to a service.
This should allow the plugin handlers of GroupContentEnablers to be decorated as we please and ensure we don't run in to issues where only one instance can decorate them.

Check, thanks for the clarification @ronaldtebrake.

@jaapjan (#100) made the patch available at https://www.drupal.org/project/gcontent_moderation with some additional fixes.

I think it's important to add credits at the module page for everyone participating in the patch. And anyone that wants to be added as maintainer is obviously more than welcome, I created the issue for that here.

So, removing the patch from composer and requiring the new module results in this error (when drush cr runs):

Fatal error: Uncaught AssertionError: The file specified by the given app root, relative path and file name (/var/www/docroot/modules/contrib/group/modules/gcontent_moderation/gcontent_moderation.info.yml) do not exist. in /var/www/docroot/core/lib/Drupal/Core/Extension/Extension.php:67
Stack trace:
#0 /var/www/docroot/core/lib/Drupal/Core/Extension/Extension.php(67): assert(false, 'The file specif...')
#1 /var/www/docroot/core/lib/Drupal/Core/Extension/ModuleHandler.php(114): Drupal\Core\Extension\Extension->__construct('/var/www/docroo...', 'module', 'modules/contrib...', NULL)
#2 /var/www/docroot/core/lib/Drupal/Component/DependencyInjection/Container.php(257): Drupal\Core\Extension\ModuleHandler->__construct('/var/www/docroo...', Array, Object(Drupal\Core\Cache\ChainedFastBackend))
#3 /var/www/docroot/core/lib/Drupal/Component/DependencyInjection/Container.php(171): Drupal\Component\DependencyInjection\Container->createService(Array, 'module_handler')
#4 /var/www/docroot/core/lib/Drupal/Core/DrupalKernel.php(584): Dru in /var/www/docroot/core/lib/Drupal/Core/Extension/Extension.php on line 67

What is a good "update" path? To make this work, I will have to a release that relies on configuration sync and updates.

You should definitely uninstall the module from the patch before removing the patch from composer and only then install the new module. The thing is the code is now in another place and drupal has problems with moving modules around.

Apart from that: Any issues with the new module should be reported in the modules issue queue. I propose to close this issue as won't fix.

You should definitely uninstall the module from the patch before removing the patch

I should be a bit more careful proposing this, since I'm not 100% sure if there could be data loss. But from what I understand how the module works, that should not happen.

Apart from that: Any issues with the new module should be reported in the modules issue queue. I propose to close this issue as won't fix.

Agree, I don't think it makes sense to keep this open. Better to continue the work at the module issue queue.

What is a good "update" path? To make this work, I will have to a release that relies on configuration sync and updates.

Additional to what anruether mentioned, you may also want to regenerate the Composer autoloader files. You can do that via composer dump-autoload afterwards running drush cr. Hope that helps.