Add per-bundle unpublished content permissions

There's also talk about moving the view any unpublished content to core rather than content_moderation. #273595: Move permission "view any unpublished content" from Content Moderation to Node

Review as follows. I know there has been some friction in other issues that introduce a lot of permissions. I think it makes sense though because node allows you to customise everything per-bundle.

1. +++ b/core/modules/content_moderation/src/Permissions.php
   @@ -2,17 +2,69 @@
   +      $container->get('entity_type.manager'),
   

   We could just inject the result of getDefinitions() here if we aren't using all of entityTypeManager.

2. +++ b/core/modules/content_moderation/src/Permissions.php
   @@ -35,4 +87,28 @@ public function transitionPermissions() {
   +        foreach ($this->bundleInfo->getBundleInfo($entity_type->id()) as $bundle_id => $bundle_information) {
   

   It would be good to have a test for an entity without bundles and ensure we don't get two permissions that do that same thing (wouldn't want a permission for "entity_test" and "entity_test:entity_test").

3. +++ b/core/modules/content_moderation/src/Permissions.php
   @@ -35,4 +87,28 @@ public function transitionPermissions() {
   +          if ($this->moderationInfo->shouldModerateEntitiesOfBundle($entity_type, $bundle_id))
   +          $permissions["view any unpublished {$entity_type->id()}:{$bundle_id} content"] = [
   +            'title' => $this->t('%entity_type: View any unpublished %bundle content', [
   +              '%entity_type' => $entity_type->getLabel(),
   +              '%bundle' => $bundle_information['label'],
   +            ]),
   +          ];
   

   Missing braces and indent.

I think this looks pretty good, very tempted to RTBC, but two very minor comments first, so leaving as Needs Review:

1. +++ b/core/modules/content_moderation/src/Access/LatestRevisionCheck.php
   @@ -57,6 +57,12 @@ public function access(Route $route, RouteMatchInterface $route_match, AccountIn
   +      if (!$access_result->isAllowed()) {
   ...
          if (!$access_result->isAllowed()) {
   

   Something does feel right about doing this same check twice. I guess it makes sense, if access is not allowed check this, if access is still not allowed check this, etc. Although I wonder if there is a better solution.

2. +++ b/core/modules/content_moderation/src/Permissions.php
   @@ -35,4 +87,29 @@ public function transitionPermissions() {
   +    foreach ($this->entityDefinitions as $entity_type) {
   +      if ($this->moderationInfo->canModerateEntitiesOfEntityType($entity_type)) {
   +        foreach ($this->bundleInfo->getBundleInfo($entity_type->id()) as $bundle_id => $bundle_information) {
   +          if ($this->moderationInfo->shouldModerateEntitiesOfBundle($entity_type, $bundle_id)) {
   

   I'm surprised we don't have one or more get methods in ModerationInformation that would do this. Like getBundlesThatShouldModerate() or something.

Re-rolled.

Just a general comment. I would expect \Drupal\node\Plugin\views\filter\Status::query to be extended in some way or another here.

• I agree that #19 could be handled in a follow up.
• It might be worth adding a test with \Drupal\entity_test\Entity\EntityTestNoBundle just to make sure when the $bundle is added to a permission name / title it still works ok when there is no bundle entity key on the entity.
• Also, as I noted in #2, there is an effort to move permissions to core (#273595: Move permission "view any unpublished content" from Content Moderation to Node), I guess we'd do the same with the per-bundle permissions too.

Hi,

I applied patch in #18 to clean Drupal 8.5.x

Made 3 roles, 2 content types and 2 users. As far as I can see the patch does the job.

Few observations:
Users can comment on unpublished nodes if they have "view any unpublished x content". This probably is as it should?
You can create permission set where user can create nodes (unpublished) but can't see them after creating them. This is also fine, I think.

There was one small glitch while patching:

patching file core/modules/content_moderation/tests/src/Functional/NodeAccessTest.php
Hunk #1 succeeded at 184 with fuzz 1 (offset 33 lines).

I still think we need the extra test, as stated in #21.

I have just applied the patch in #26. Worked for me in my system with 4 content type. Tried different combinations. Viewed node/%nid/latest having different permissions selected. Changed permissions and tried again. All works as expected.

Thank you @jhedstrom !!!

Looks good, think it's ready for committer review.

Should we check whether permission_granularity is set to bundle for a given entity type before adding this to all entity types?

Should we focus instead on #2809177: Introduce entity permission providers ?

Hmm, I'm not sure #2809177: Introduce entity permission providers will cover this functionality...? It doesn't look like it -- the closest I'm seeing so far is "view own unpublished ENTITY_TYPE" -- not "view own unpublished (BUNDLE) ENTITY_TYPE," or "view unpublished ENTITY_TYPE" or "view unpublished (BUNDLE) ENTITY_TYPE".

I'll post a comment over there, too, just to see.

LGTM, just nits 😅

1. +++ b/core/modules/content_moderation/src/Permissions.php
   @@ -2,18 +2,69 @@
   +  protected $entityDefinitions;
   ...
   +   * @param \Drupal\Core\Entity\EntityTypeInterface[] $entity_definitions
   

   🤓 Nit: $entity_type_definitions would be more accurate.

2. +++ b/core/modules/content_moderation/src/Permissions.php
   @@ -37,4 +88,29 @@ public function transitionPermissions() {
   +   * Returns an array of per-entity and bundle unpublished permissions.
   ...
   +   *   The per-entity-bundle permissions for viewing unpublished content.
   

   🤓 Nit: "per entity type", not "per entity"

3. +++ b/core/modules/content_moderation/tests/src/Functional/NodeAccessTest.php
   @@ -184,4 +184,77 @@ public function testPageAccess() {
   +    // Access that the status field is no longer visible.
   

   🤓 s/Access/Assess/ or perhaps "assert".

Have made changes suggested in #39.

Hi Folks, I have updated the patch to fix the coding standard and using the deprecated class usage. Please review it. Thanks

Great! Just one detail was forgotten 😅

+++ b/core/modules/content_moderation/src/Permissions.php
@@ -2,18 +2,69 @@
+  protected $entityDefinitions;

This should be named $entityTypeDefinitions.

Hi Wim,

I have updated patch with your suggestions mentioned in #45. Please review the patch. Thanks

LGTM!

To reach RTBC, we need some meta work to be done too.

Hi All, I have added the change record for this issue. Please review. Change record URL - https://www.drupal.org/node/3104146

Reviewing this with the benefit of some foresight, two things stand out to me as potential issues or caveats:

• The node permission "view own unpublished content" doesn't have the same per-bundle support.
• The existing permission is supported via \Drupal\node\Plugin\views\filter\Status, I don't know how these new permissions will interact with this existing feature.

+++ b/core/modules/content_moderation/tests/src/Unit/LatestRevisionCheckTest.php
@@ -106,27 +111,31 @@ public function testLatestAccessPermissions($entity_class, $entity_type, $has_pe
   public function accessSituationProvider() {
     return [
       // Node with global permissions and latest version.

I also think it's cleaner if all these test cases are additive instead of modified.

Adding related view_unpublished issue.

Simple re-roll of #46 for 9.2.x

Thanks @kevin.dutra! Perhaps an interdiff can be added and Status changed to "Needs review", to get it tested?

@ressa, that was just a re-roll of the patch in #46, so it doesn't address the comments in #51 and as a result it's not ready for review. There's no interdiff because there are no code changes from the #46 patch, just adjustments to the code context so that Git can find the correct places to apply the changes.

I see, thanks for clarifying. I originally just did a quick comparison between the two patches, and saw a bigger change, which I thought was the removal of a service. Looking closer, I now see that that change is in the current 9.3.x-dev version.

@kevin.dutra For what it's worth, #56 applies cleanly to core 9.2.5