Prevent ContentModerationState from being exposed by REST's EntityResource

I assume this will be quite different to other tests.

A moderation state is not a standalone thing, it is closely tied to an actual entity that is being moderated. So the tests here will likely be very specific to it, to make sure that changing a moderation state does the thing it needs to to its moderated entity.

Another thing is validation and access logic. Of the moderation state itself, I'm not sure how much support we have for that already (e.g. that only some users can move to published and only some changes are allowed and so on) but also that you can directly change the status of a node anymore. #2068063: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button will likely add hook_entity_field_access() to enforce that.

Thanks, HeyLodyM!

To accomplish this task, I wanted to test a validated issue but I was stuck on this error:

./vendor/bin/phpunit -c core core/modules/rest/tests/src/Functional/EntityResour
ce/MenuLinkContent/MenuLinkContentJsonBasicAuthTest.php
PHPUnit 4.8.28 by Sebastian Bergmann and contributors.

F...

Time: 2.91 minutes, Memory: 6.00Mb

There was 1 failure:

1) Drupal\Tests\rest\Functional\EntityResource\MenuLinkContent\MenuLinkContentJsonBasicAuthTest::testGet
Failed asserting that Array &0 (
0 => 'canonical'
) is identical to Array &0 (
0 => 'canonical'
1 => 'edit-form'
2 => 'https://drupal.org/link-relations/delete-form'
).

/var/www/DrupalProject/drupal-8.3.x/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:440

FAILURES!
Tests: 4, Assertions: 163, Failures: 1.

If someone has an idea?

#2843774-6: EntityResource: Provide comprehensive test coverage for BlockContentType entity reported the same problem. I have no idea how that is possible — I can't reproduce this (see #2843774-8: EntityResource: Provide comprehensive test coverage for BlockContentType entity).

Closing as agreed in #2779931: Add storage exception that enforces unique content_entity_type_id and content_entity_id on the content moderation state content entity, and add access control handler to forbid all access. There is an access control handler that denies access to the entity.

I was just coming here to do exactly that! Thanks, @Sam152 :)

Updated #2824572 accordingly: #2824572-31: Write EntityResourceTestBase subclasses for every other entity type..

Actually, based on what I've just been doing in #2835767: Media + REST: comprehensive test coverage for Media + MediaType entity types, I think a similar approach is warranted here.

1. In #2779931: Add storage exception that enforces unique content_entity_type_id and content_entity_id on the content moderation state content entity, and add access control handler to forbid all access, we added an access control handler that forbids every operation on content_moderation_state entities, including viewing.
2. Yet the entity:content_moderation_state derivative REST resource plugin still exists!
3. This means that when the REST UI module is installed, it's possible to configure a REST resource that exposes Content Moderation State entities:
   
   … yet it won't work because of point 1.

The solution is simple: we remove the entity:content_moderation_state derivative REST resource plugin — which is exactly what #2835767: Media + REST: comprehensive test coverage for Media + MediaType entity types does for the Media module's entities.

Small naming problem.

+++ b/core/modules/content_moderation/tests/src/Functional/ModerationStateResourceTest.php
@@ -0,0 +1,38 @@
+class ModerationStateResourceTest extends BrowserTestBase {

Why is this a Browser test? As far as I see it doesn't interact with any page, so it can simply be a kernel test?

Edit: and there's still a small naming problem: ContentModerationStateResourceTest

Edit: and there's still a small naming problem: ContentModerationStateResourceTest

Well spotted! I only noticed it because testbot complained. Already fixed in #18 :)

so it can simply be a kernel test?

Great point, done!

Oh, perhaps you meant that it should be renamed from ModerationStateResourceTest to ContentModerationStateResourceTest? Happy to do that. I named it like that because almost everything else is called ModerationState*Test.php. Perhaps the reasoning is just not clear to me.

Let me know, happy to reroll.

@Wim Leers, I think the name of the test class should be ContentModerationStateResourceTest because it tests the ContentModerationState entity type :)

Fair!

Looks good to me now :)

+++ b/core/modules/content_moderation/content_moderation.module
@@ -248,3 +248,13 @@ function content_moderation_workflow_insert(WorkflowInterface $entity) {
+  // ContentModerationState is an internal entity type. Therefore it should also
+  // not be exposed via REST.

nit:not sure the word 'also' is needed here (can be fixed on commit)

@Wim Leers and @xjm suggested to add this as a MUST-HAVE for the Workflow Initiative.

[removed]

Updating issue credit to add @amateescu who helped shape patch

Can we get an issue summary update as the proposed resolution isn't where the patch ended up.

Thanks @Sam152 for updating issue summary

Fixed on commit

diff --git a/core/modules/content_moderation/content_moderation.module b/core/modules/content_moderation/content_moderation.module
index 72df8c8..0dc03bf 100644
--- a/core/modules/content_moderation/content_moderation.module
+++ b/core/modules/content_moderation/content_moderation.module
@@ -267,8 +267,8 @@ function content_moderation_workflow_update(WorkflowInterface $entity) {
  * Implements hook_rest_resource_alter().
  */
 function content_moderation_rest_resource_alter(&$definitions) {
-  // ContentModerationState is an internal entity type. Therefore it should also
-  // not be exposed via REST.
+  // ContentModerationState is an internal entity type. Therefore it should not
+  // be exposed via REST.
   // @see \Drupal\content_moderation\ContentModerationStateAccessControlHandler
   unset($definitions['entity:content_moderation_state']);
 }

Committed as 930a35b and pushed to 8.4.x. On with the next WI critical!

We're keeping this test, but the work-around introduced here is now being removed in #2941622: Make REST's EntityResource deriver exclude internal entity types and mark ContentModerationState entity type as internal instead of the current hack because we now have a proper API for this!