JSON:API POST/PATCH support for fully validatable config entities

Config entities have no validation and access API beside their UI's/Forms, so exposing over REST is tricky.

If we'd want that, it would be clearer if that would be a completely separate plugin and serialization, that would just directly use toArray().

Yeah, I agree that access checking should be sufficient for config entities. We have been moving away from route-based access (i.e. using _permission directly) to entity-level access for config entities so that should work fine. And in terms of validation we can use the new SchemaCheckTrait. It's not as powerful as content entity validation, but I think it too should be sufficient to validate that nothing completely bogus enters the system.

Maybe, but that still means that it would be a separate plugin for config entities and IMHO a feature request, not a bug. The only bug seems to be that you can configure rest.module to expose config entities right now an it's then choking on it...

What services supported and not is not really relevant, a feature in a contrib module doesn't mean that not having it in core is a regression :) Especially because 7.x did not differentiate between config and content entities, services.module did not integrate entities in a generic way and system is not an entity :)

If one config entity is supported then all are.

Configuration being what it is in Drupal, exposing it even behind permissions is a little like exposing custom module code if you've got the right permission. The delicate balance there seems pretty good for Contrib to explore, and maybe something gets into 8.1.

In practice, I expect it's not the full config entity you need, but some externally useful information (e.g., the name of a vocabulary), associated with the content it configures. Full CRUD support for Config would make this an API for building your Drupal site, and that seems like a glaring potential for massive security problems.

In an effort to constrain scope. the REST team decided to focus on content entities and not config entities. I'd be happy if someone took up this important feature. Retitling and recategorizing accordingly.

I'm no REST expert, but from what I can tell, the main problem is validation when modifying config entities.

What if core would only support reading config entities? That'd satisfy the majority and still defer the most complex parts to contrib.

During the Spring of DrupalCon Amsterdam 2014 I did a debug of how get via Rest Config Entity

I did my test with Entity entity:entity_form_display and the URL I use test was http://example.com/entity/entity_form_display/node.article.default after pass the Basic Auth I got a Access Denied 403.

The access denied was trigger in Resource Drupal\Core\Entity\Entity\EntityFormDisplay. because the class Drupal\rest\Plugin\rest\resource\EntityResource tried to execute the method access and doesn't exist.

I will improve the function get of Class EntityResource

public function get(EntityInterface $entity) {
    if (!$entity->access('view')) {
      print_r(get_class($entity));
      throw new AccessDeniedHttpException();
    }
    foreach ($entity as $field_name => $field) {
      if (!$field->access('view')) {
        unset($entity->{$field_name});
      }
    }
    return new ResourceResponse($entity);
  }

ASAP I got a patch I will upload.

Uhhh my fault, I will review and provide a feedback if works for Entity Form Display

Hi folks

I did a patch to support Entity Display entities via Rest, fixing the error trying to call access method with view parameters, also add current validation to confirm the user has access to get Entity Display definition.

To test this patch you can use the Chrome plugin Simple REST Client and the URI http://example.com/entity/entity_form_display/node.article.default remember enable the resource using the Rest UI module as you can see in the following image

As you can see in configuration you must to use Basic Auth in Rest Client and be sure the header Accept with application/json

As result you will get something similar to this output

{
    "uuid": "a3283201-6a27-41a4-a400-bfbd7550fd54",
    "langcode": "en",
    "status": true,
    "dependencies": {
        "entity": [
            "field.field.node.article.body",
            "field.field.node.article.comment",
            "field.field.node.article.field_image",
            "field.field.node.article.field_tags",
            "node.type.article"
        ],
        "module": [
            "comment",
            "entity_reference",
            "image",
            "path",
            "taxonomy",
            "text"
        ]
    },
    "id": "node.article.default",
    "targetEntityType": "node",
    "bundle": "article",
    "mode": "default",
    "content": {
        "title": {
            "type": "string_textfield",
            "weight": 0,
            "settings": {
                "size": 60,
                "placeholder": ""
            },
            "third_party_settings": []
        },
        "body": {
            "type": "text_textarea_with_summary",
            "weight": 1,
            "settings": {
                "rows": 9,
                "summary_rows": 3,
                "placeholder": ""
            },
            "third_party_settings": []
        },
        "field_tags": {
            "type": "taxonomy_autocomplete",
            "weight": 3,
            "settings": [],
            "third_party_settings": []
        },
        "field_image": {
            "type": "image_image",
            "weight": 4,
            "settings": {
                "progress_indicator": "throbber",
                "preview_image_style": "thumbnail"
            },
            "third_party_settings": []
        },
        "uid": {
            "type": "entity_reference_autocomplete",
            "weight": 5,
            "settings": {
                "match_operator": "CONTAINS",
                "size": 60,
                "autocomplete_type": "tags",
                "placeholder": ""
            },
            "third_party_settings": []
        },
        "created": {
            "type": "datetime_timestamp",
            "weight": 10,
            "settings": [],
            "third_party_settings": []
        },
        "promote": {
            "type": "boolean_checkbox",
            "settings": {
                "display_label": "1"
            },
            "weight": 15,
            "third_party_settings": []
        },
        "sticky": {
            "type": "boolean_checkbox",
            "settings": {
                "display_label": "1"
            },
            "weight": 16,
            "third_party_settings": []
        },
        "comment": {
            "type": "comment_default",
            "weight": 20,
            "settings": [],
            "third_party_settings": []
        },
        "path": {
            "type": "path",
            "weight": 30,
            "settings": [],
            "third_party_settings": []
        }
    },
    "hidden": [],
    "third_party_settings": []
}

@-Enzo- thanks, works for me! =)

HI @clemens.tolboom I did a new patch following your recommendation, please check and let me know if works for you.

Hi @clemens.tolboom

I remove unrelated code and fix indentation

@clemens.tolboom how I can do this " this needs tests too to confirm the GET versus other ops work as expected."?

I've tried the patch to get the contents of the main menu. (The path in REST UI is incorrect. The /entity part is missing)

curl --user admin:admin --header "Accept: application/json" --request GET http://drupal.d8/entity/menu/main

The response is correct but useless:

{
    "uuid": "2ee5aa01-b45d-46f0-9d06-6be711cdcedc",
    "langcode": "en",
    "status": true,
    "dependencies": [],
    "id": "main",
    "label": "Main navigation",
    "description": "Use this for linking to the main site sections.",
    "locked": true
}

The thing I needed was the items in the menu, not the metadata of the main menu.

Hi @MartijnBraam

About the missing entity in end point, maybe you need to update the Rest UI module I did a patch to resolve that issue #2290605: REST URI paths changed to canonical paths.

About the entity menu did you request, after enable the Entity Menu Resorce as you can see in the following image.

When I tried to consume this end point using the URL http://example.com/entity/menu/main I got the following error

[Wed Oct 15 06:35:14 2014] [error] [client 127.0.0.1] PHP Fatal error: Call to a member function access() on a non-object in /Users/enzo/www/drupal8beta/core/modules/rest/src/Plugin/rest/resource/EntityResource.php on line 52

I will debug and propose a new version of patch.

Attached you can find a new patch testes Menu, Nodes and Entity Display Form

Hi @clemens.tolboom

Using you patch #35 I did a new patch, because the way you use to get the Id of plugin return empty because is not an object is an array, but if you test with admin user the validation never is executed.

About the test I can't test because I'm getting a awful error trying to list the test to execute.

[Thu Oct 16 05:18:09 2014] [error] [client 127.0.0.1] Uncaught PHP Exception ReflectionException: "Class Drupal\\Tests\\devel_generate\\DevelGenerateManagerTest does not exist in expected /Users/enzo/www/drupal8/modules/devel/devel_generate/tests/src/DevelGenerateManagerTest.php" at /Users/enzo/www/drupal8/core/modules/simpletest/src/TestDiscovery.php line 153, referer: http://drupal8/admin/config/development/testing/settings

ASAP I resolve this issue I will work in testing, but meanwhile do you have any idea what is wrong in test? Also the test must be cover in this issue or maybe we can create a new issue?

@clemens.tolboom I follow your instructions and remove the devel issue, but now I got a new error if I try to run any Test you can see the error at #2341997: Uncaught exception - Circular reference detected for service "database".

Maybe you can consider to separate the Test in other issue and if work the patch apply to Drupal Core.

Any thoughts?

Hey folks

I did a re-roll of path #38 to meet the latest changes in Drupal 8 but still works in Unit Test are required.

Hi all. Tried the patch in 42 (support_configentity-2300677-42.patch), and still getting the metadata for menu:

curl --user admin:admin -H "Accept: application/hal+json" --request GET http://d8.local/entity/menu/footer
{"uuid":"3f36ed88-a631-4614-8601-1d7c9ae394c1","langcode":"en","status":true,"dependencies":[],"id":"footer","label":"Footer","description":"Use this for linking to site information.","locked":true}

Is this patch supposed to pull the actual menu items?

No, the patch is not. Menu links are something else entirely. The menu config entity *is* the menu metadata.

I still have the opinion that providing generic support for config entities is problematic and unneeded. In many cases, they will not give you the data you actually care about. See above.

The only real use case I see for the current content entity implementation is to synchronize data 1:1 between equal systems, e.g. a staging/production site. It is an internal representation of the raw data.

For config entities, we have the configuration sync/import API, and nothing we do could do here would come close to what that provides, with diffs, and sync flags and so on.

IMHO, if you want an API that you can use from an external source, e.g. an app, or a public API for your clients, especially if that involves config entities but likely even for content, then write it yourself, then you can define an API and structure that makes sense for your use case. Take the example in #43, where the expectation is to get all the menu links, which are actually plugins, with different sources. There is no way we can make a generic API that supports that, but it should be fairly easy to write a custom resource plugin that returns menu links in a simple structure.

@Berdir I use this patch with a Headless Drupal Generator github.com/enzolutions/generator-marionette-drupal , in my case is good idea because in that way I can generate HTML5 forms based in Drupal 8 site current status.

I know you point an API is only for data, but with proper permissions you can expose this information to create great tools and services integrated with Drupal 8

Yeah, I really don't understand how you can write a custom front-end for a Drupal 8 site without supporting at least some config entities. The inability to get vocabulary names when displaying a node is a huge limitation.

@clemens.tolboom Correct you can solve that with view because is content.

But in my case I want to create a REST Frontend #headless Drupal with Backbone with the capability to generate Forms for content types based in current content types definition in a Drupal 8 site, how you can solve that with out expose a config entity?

If you check the patch the rest resource require a specific permission to access config entities, so IMHO is not security issue. Also a Rest Resource has a security level with Authentication Provider selected when the Rest Resource is enabled.

If the argument to disable this option is force users to create content in Drupal instead of provide the option the user to decide how access his content even the admin content is the wrong angle.

Hey folks

I did a re-roll of path #42 to meet the latest changes in Drupal 8 but still works in Unit Test are required.

Kicking of the testbot to see if we need a re-roll....

Not sure if this should still be 8.0 or 8.1 but, I will see if I can reroll this patch.

I worked a bit on it, here are some bits I ran into it on the meantime.

Looks great!

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -210,6 +216,9 @@ public function delete(EntityInterface $entity) {
   protected function validate(EntityInterface $entity) {
+    if (!$entity instanceof FieldableEntityInterface) {
+      return;
+    }

Shouldn't we perform config schema validation for config entities? See \Drupal\Core\Config\Schema\SchemaCheckTrait — hattip @tstoeckler in #5.

Closed #2650792: What permissions control REST's field_storage_config? as a duplicate, another person wondering why this doesn't work, and the lack of helpful errors also getting in the way.

I think it's safe to say this is at least major.

Made some progress:

• The create test now actually works, yeah
• Started to expand the read test

Note: This is blocked on #1928868: Typed config incorrectly implements Typed Data interfaces technically, doesn't mean though one cannot work here.

Let's be more honest about it.

Let's work first of the GET aspect of things, see #2724823: EntityResource: read-only (GET) support for configuration entities

what does pp-2 mean?

#2724823: EntityResource: read-only (GET) support for configuration entities landed. So this is now blocked on "just" #1928868: Typed config incorrectly implements Typed Data interfaces.

@Wim Leers
Do you mind updating your comment and replace just with "just"?

Done :P

This is not blocked on reviews. If somebody wants to take it on despite the blocker, feel free to mark it as active. For now, marking as postponed.

Note: #1928868: Typed config incorrectly implements Typed Data interfaces has a needs review patch now. Feedback there would be great.

Just some intermedia reroll, nothing to see :)

We decided that #1928868: Typed config incorrectly implements Typed Data interfaces is the only blocker, yeah!

I rerolled / started this patch based upon on #1928868: Typed config incorrectly implements Typed Data interfaces

Further progress:

• testPost
• testPatch
• testDelete

All of them are now working with the config_test entity type.
There is still a hack inside ConfigEntityBase for example we have to remove of course.

Setting this to needs review to get a testbot run.

Thank you so much for getting this going again! EXCITING! :D Here's a review of the *-review.patch patch.

1. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -277,6 +277,10 @@ config_entity:
   +        Length:
   

   Should this have a capital first letter? Interesting!

2. +++ b/core/modules/rest/src/Plugin/Deriver/EntityDeriver.php
   @@ -88,6 +90,10 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +        if ($entity_type instanceof ConfigEntityTypeInterface) {
   +          $this->derivatives[$entity_type_id]['class'] = ConfigEntityResource::class;
   +        }
   
   +++ b/core/modules/rest/src/Plugin/rest/resource/ConfigEntityResource.php
   @@ -0,0 +1,39 @@
   +class ConfigEntityResource extends EntityResource {
   

   Clever :)

3. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -264,36 +264,38 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   -    foreach ($entity->_restSubmittedFields as $field_name) {
   ...
   +    if ($entity instanceof FieldableEntityInterface) {
   +      foreach ($entity->_restSubmittedFields as $field_name) {
   

   This is just wrapping all this code in an if-statement. Makes sense. (And is also happening in a few other patches.)

4. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -388,20 +390,6 @@ protected function getBaseRoute($canonical_path, $method) {
   -  public function availableMethods() {
   

   Yay, being able to delete this is great :)

5. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -388,20 +390,6 @@ protected function getBaseRoute($canonical_path, $method) {
   -    if ($this->isConfigEntityResource()) {
   

   This was the only call to that helper method, we can delete that helper method also now.

6. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResourceAccessTrait.php
   @@ -25,9 +26,11 @@ protected function checkEditFieldAccess(EntityInterface $entity) {
   -    foreach ($entity->_restSubmittedFields as $key => $field_name) {
   ...
   +    if ($entity instanceof FieldableEntityInterface) {
   +      foreach ($entity->_restSubmittedFields as $key => $field_name) {
   

   Same wrapping here.

7. +++ b/core/modules/rest/tests/src/Functional/EntityResource/ConfigTest/ConfigTestResourceTestBase.php
   @@ -22,11 +22,26 @@
   +      case 'POST':
   +        $this->grantPermissionsToTestedRole(['administer config_test']);
   +        break;
   +      case 'PATCH':
   +        $this->grantPermissionsToTestedRole(['administer config_test']);
   +        break;
   +      case 'DELETE':
   +        $this->grantPermissionsToTestedRole(['administer config_test']);
   

   You can make the first two fall-through cases.

8. +++ b/core/modules/rest/tests/src/Functional/EntityResource/ConfigTest/ConfigTestResourceTestBase.php
   @@ -67,7 +82,33 @@ protected function getExpectedNormalizedEntity() {
   +    switch ($method) {
   +      default:
   +        return "The 'administer config_test' permission is required.";
   +    }
   

   Hm, this is a bit strange.

   First: if there's only the default case, then we can just get rid of the switch statement altogether?

   Second: I'd have thought that in one case, the required permissions is view config_test instead?

9. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -684,12 +684,13 @@ public function testPost() {
   +    if ($this->entity instanceof FieldableEntityInterface) {
   

   I can see that this only makes sense for content entities. But shouldn't you also get a validation error for config entities if you try to do this?

10. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -698,16 +699,23 @@ public function testPost() {
    -      $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nuuid.0.value: UUID: may not be longer than 128 characters.\n", $response);
    +      if ($this->entity instanceof FieldableEntityInterface) {
    +        $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nuuid.0.value: UUID: may not be longer than 128 characters.\n", $response);
    +      }
    +      else {
    +        $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nuuid: UUID: may not be longer than 128 characters.\n", $response);
    +      }
    

    Nice!

11. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -698,16 +699,23 @@ public function testPost() {
    +    if ($this->entity instanceof FieldableEntityInterface) {
    

    Field access only exists for content entities (well, fieldable entities), so that makes sense.

Should this have a capital first letter? Interesting!

Yeah I'm probably the person which with biggest confusion about the typed data validation system, but yeah this is the plugin id to be used here: \Drupal\Core\Validation\Plugin\Validation\Constraint\LengthConstraint

Hm, this is a bit strange.

First: if there's only the default case, then we can just get rid of the switch statement altogether?

Second: I'd have thought that in one case, the required permissions is view config_test instead?

oh yeah I think you are right.

In general I think we should have a new config entity just to test REST validation for now ... given that other tests fail which are using this config entity in other places.
Do you think we should do that, or should we jump directly to fix the other tests as well?

Do you think we should do that, or should we jump directly to fix the other tests as well?

I'm fine with either approach.

We need to fix it inline at some point anyway, given we also want tests for the test entity types. Here is some ongoing work.

Further progress:

testPost
testPatch
testDelete
All of them are now working with the config_test entity type.

This is kind of a lie :)

18 fails is still too much ...

Just a reroll after the other issue landed, yeah!

Tantalizingly close now!

Added a related issue which ideally we fix first: #2869809: Make it easy to get typed config representations of entities as we no longer would have to change a protected to a public method.

In general I'm not 100% convinced to get the patch in as it without having config schema, at least for the non test config entities. Of course this is up for discussion, but its something which should be take into account, if possible.

In general I'm not 100% convinced to get the patch in as it without having config schema

I'm assuming as it was intended to read as is.

I agree, I'm not comfortable with that either. That's too risky. In #98 I was just expressing my excitement of a green patch that is showing significant progress, and a good starting point for further work :)

Awesome job, Daniel!

I agree, I'm not comfortable with that either. That's too risky. In #98 I was just expressing my excitement of a green patch that is showing significant progress, and a good starting point for further work :)

So yeah I'm wondering whether we should opt in entity types over time we support.

That's exactly what I was thinking, some kind of annotation key that opts in to this. won't ensure contrib/custom properly validated third party settings, but it's the same with custom/contrib field types and fields for content entities.

Sounds good :)

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -388,20 +390,6 @@ protected function getBaseRoute($canonical_path, $method) {
-  public function availableMethods() {
-    $methods = parent::availableMethods();
-    if ($this->isConfigEntityResource()) {
-      // Currently only GET is supported for Config Entities.
-      // @todo Remove when supported https://www.drupal.org/node/2300677
-      $unsupported_methods = ['POST', 'PUT', 'DELETE', 'PATCH'];
-      $methods = array_diff($methods, $unsupported_methods);
-    }
-    return $methods;
-  }

We can bring this back and make this work on a per-entity type basis.

What about using an additional like: _rest_resouce, so with an underscore to explain its kinda internal/will not exist forever?

So something like this?

Boolean logic is hard.

#106 suggests an underscore-prefixed annotation key. I'm fine with that.

But #107+#109 omit that leading underscore. Furthermore, I think this is confusing:

+++ b/core/modules/config/tests/config_test/src/Entity/ConfigTest.php
@@ -22,6 +22,7 @@
+ *   rest_resource = TRUE,

I think it'd be better to rename this to __internal__config_entity_type_supports_validation: TRUE, and make that a default annotation key on entities, and make it default to FALSE.

That's super explicit because super ugly, and then also sets it to the correct value for all config entity types by default :)

I think it'd be better to rename this to __internal__config_entity_type_supports_validation: TRUE, and make that a default annotation key on entities, and make it default to FALSE.

Well I have problems with setting them ever to TRUE, which is why I removed the underscore. As of now no config entity out there provides config constrains, so I fear swapping the default would ever to possible in D8. Given that it is actually not internal during the D8 cycle. In D9 I would totally suggest to remove the capability.

Fair. But then let's go with

supports_validation = TRUE

Because this is not only about REST, there may be other functionality that can only work if a config entity supports validation.

Thanks for your impressive work here @dawehner :)

1. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -277,6 +277,10 @@ config_entity:
   +          maxMessage: 'UUID: may not be longer than 128 characters.'
   
   +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php
   @@ -172,6 +172,12 @@ public function testSchemaMapping() {
   +    $expected['mapping']['uuid']['constraints'] = [
   +      'Length' => [
   +        'max' => 128,
   +        'maxMessage' => 'UUID: may not be longer than 128 characters.',
   +      ],
   +    ];
   

   We need this for the "129-character UUID should trigger a validation error" test, so this is definitely distinct from #2870878: Add config validation for UUIDs.

2. +++ b/core/lib/Drupal/Core/Config/StorableConfigBase.php
   @@ -129,7 +129,7 @@ public function getStorage() {
   -  protected function getSchemaWrapper() {
   +  public function getSchemaWrapper() {
   
   +++ b/core/modules/rest/src/Plugin/rest/resource/ConfigEntityResource.php
   @@ -0,0 +1,39 @@
   +    $typed_config = $config->getSchemaWrapper();
   

   We're making this method public because of this.

3. --- a/core/modules/config/src/Tests/ConfigEntityListTest.php
   +++ b/core/modules/config/src/Tests/ConfigEntityListTest.php
   --- a/core/modules/config/src/Tests/ConfigEntityTest.php
   +++ b/core/modules/config/src/Tests/ConfigEntityTest.php
   --- /dev/null
   +++ b/core/modules/config/tests/config_test/config_test.permissions.yml
   --- a/core/modules/config/tests/config_test/src/ConfigTestAccessControlHandler.php
   +++ b/core/modules/config/tests/config_test/src/ConfigTestAccessControlHandler.php
   

   We're making all these changes because to prove that this approach works, this issue is updating config_test and its test coverage to prove that POSTing, PATCHing and DELETEing config entities is in fact possible.

4. +++ b/core/modules/rest/src/Plugin/rest/resource/ConfigEntityResource.php
   @@ -0,0 +1,39 @@
   +    // Add a custom config entity resource class in order to provide
   

   Nit: let's remove this comment.

5. +++ b/core/modules/rest/src/Plugin/rest/resource/ConfigEntityResource.php
   @@ -0,0 +1,39 @@
   +    if ($violations->count() > 0) {
   +      $message = "Unprocessable Entity: validation failed.\n";
   +      foreach ($violations as $violation) {
   +        // We strip every HTML from the error message to have a nicer to read
   +        // message on REST responses.
   +        $message .= $violation->getPropertyPath() . ': ' . PlainTextOutput::renderFromHtml($violation->getMessage()) . "\n";
   +      }
   +      throw new UnprocessableEntityHttpException($message);
   +    }
   

   All of this is duplicating \Drupal\rest\Plugin\rest\resource\EntityResourceValidationTrait::validate(). Which is fine. But if we'd update EntityResourceValidationTrait to have a protected function processViolations($violations) method, then we wouldn't need to duplicate this brittle (yet existing code).

6. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -264,36 +264,38 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   -    foreach ($entity->_restSubmittedFields as $field_name) {
   -      $field = $entity->get($field_name);
   ...
   +    if ($entity instanceof FieldableEntityInterface) {
   +      foreach ($entity->_restSubmittedFields as $field_name) {
   +        $field = $entity->get($field_name);
   
   +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResourceAccessTrait.php
   @@ -25,9 +26,11 @@ protected function checkEditFieldAccess(EntityInterface $entity) {
   -    foreach ($entity->_restSubmittedFields as $key => $field_name) {
   ...
   +    if ($entity instanceof FieldableEntityInterface) {
   +      foreach ($entity->_restSubmittedFields as $key => $field_name) {
   

   This change is trivial: it's not changing the existing code; it's just wrapping it, in an if instanceof check.

7. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -392,7 +394,7 @@ protected function getBaseRoute($canonical_path, $method) {
   +    if ($this->isConfigEntityResource() && empty($this->entityType->get('rest_resource'))) {
   

   This would become much clearer if we did what I suggested in #113.

8. +++ b/core/modules/rest/tests/src/Functional/EntityResource/ConfigTest/ConfigTestResourceTestBase.php
   @@ -22,11 +22,31 @@
   +  protected $counter = 0;
   

   Nit: missing docblock.

9. +++ b/core/modules/rest/tests/src/Functional/EntityResource/ConfigTest/ConfigTestResourceTestBase.php
   @@ -22,11 +22,31 @@
   +      case 'POST':
   +        $this->grantPermissionsToTestedRole(['administer config_test']);
   +        break;
   +      case 'PATCH':
   +        $this->grantPermissionsToTestedRole(['administer config_test']);
   +        break;
   +      case 'DELETE':
   +        $this->grantPermissionsToTestedRole(['administer config_test']);
   

   These can be merged.

10. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -131,6 +131,13 @@
       /**
    +   * Flag to indicate a test which can modify config entities.
    +   *
    +   * @var bool
    +   */
    +  protected $configEntityWithModifyingTest = FALSE;
    
    @@ -585,7 +592,7 @@ protected static function castToString(array $normalization) {
       public function testPost() {
         // @todo Remove this in https://www.drupal.org/node/2300677.
    -    if ($this->entity instanceof ConfigEntityInterface) {
    +    if ($this->entity instanceof ConfigEntityInterface && !$this->configEntityWithModifyingTest) {
    
    @@ -774,7 +794,7 @@ public function testPost() {
    -    if ($this->entity instanceof ConfigEntityInterface) {
    +    if ($this->entity instanceof ConfigEntityInterface && !$this->configEntityWithModifyingTest) {
    
    @@ -973,7 +999,7 @@ public function testPatch() {
    -    if ($this->entity instanceof ConfigEntityInterface) {
    +    if ($this->entity instanceof ConfigEntityInterface && !$this->configEntityWithModifyingTest) {
    

    I think we can remove this; we can just inspect the config entity type's annotation.

11. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -585,7 +592,7 @@ protected static function castToString(array $normalization) {
           $this->assertTrue(TRUE, 'POSTing config entities is not yet supported.');
    
    @@ -774,7 +794,7 @@ public function testPost() {
           $this->assertTrue(TRUE, 'PATCHing config entities is not yet supported.');
    
    @@ -973,7 +999,7 @@ public function testPatch() {
           $this->assertTrue(TRUE, 'DELETEing config entities is not yet supported.');
    

    And we can then also update this message, to:

    POSTing config entities of this type is not yet supported.
    

    Same for PATCHing and DELETEing.

12. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -598,7 +605,12 @@ public function testPost() {
    -    $parseable_invalid_request_body_2 = $this->serializer->encode($this->getNormalizedPostEntity() + ['uuid' => [$this->randomMachineName(129)]], static::$format);
    +    if ($this->entity instanceof FieldableEntityInterface) {
    +      $parseable_invalid_request_body_2 = $this->serializer->encode($this->getNormalizedPostEntity() + ['uuid' => [$this->randomMachineName(129)]], static::$format);
    +    }
    +    else {
    +      $parseable_invalid_request_body_2 = $this->serializer->encode($this->getNormalizedPostEntity() + ['uuid' => $this->randomMachineName(129)], static::$format);
    +    }
    

    Is this change actually necessary? If it is, we should document this.

13. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -684,12 +696,13 @@ public function testPost() {
    -    // DX: 422 when invalid entity: multiple values sent for single-value field.
    -    $response = $this->request('POST', $url, $request_options);
    -    $label_field = $this->entity->getEntityType()->hasKey('label') ? $this->entity->getEntityType()->getKey('label') : static::$labelFieldName;
    -    $label_field_capitalized = $this->entity->getFieldDefinition($label_field)->getLabel();
    -    $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\n$label_field: $label_field_capitalized: this field cannot hold more than 1 values.\n", $response);
    -
    +    if ($this->entity instanceof FieldableEntityInterface) {
    +      // DX: 422 when invalid entity: multiple values sent for single-value field.
    +      $response = $this->request('POST', $url, $request_options);
    +      $label_field = $this->entity->getEntityType() ->hasKey('label') ? $this->entity->getEntityType() ->getKey('label') : static::$labelFieldName;
    +      $label_field_capitalized = $this->entity->getFieldDefinition($label_field) ->getLabel();
    +      $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\n$label_field: $label_field_capitalized: this field cannot hold more than 1 values.\n", $response);
    +    }
    

    Hm… don't we want a validation error for config entities if you send multiple values for a single-value property?

    (In other words: if you're sending a list or map for a boolean or string value?)

14. Removing Needs tests because this is expanding the existing REST test coverage! :) :)
15. We need to document the relation of this issue to #2869792: [meta] Add constraints to all config entity types. I think the intent is that both of these can happen independently?

#114.2

We're making this method public because of this.

Note: Once #2869809: Make it easy to get typed config representations of entities is in, we no longer need that.

#114.4

Nit: let's remove this comment.

Totally

#114.5

All of this is duplicating \Drupal\rest\Plugin\rest\resource\EntityResourceValidationTrait::validate(). Which is fine. But if we'd update EntityResourceValidationTrait to have a protected function processViolations($violations) method, then we wouldn't need to duplicate this brittle (yet existing code).

Totally

#114.7

Because this is not only about REST, there may be other functionality that can only work if a config entity supports validation.

I initially thought this sounds a bit like a weird idea, but the more the think about it, this makes clear what the root cause of that is. One could have suggested something like: supports_update_rest, but you know, it does supports that. It just doesn't support the validation aspect of things.

#114.10

I think we can remove this; we can just inspect the config entity type's annotation.

Done that

#114.12

Is this change actually necessary? If it is, we should document this.

    // The normalized structure looks different for fieldable and non fieldable
    // entities.

#114.13

Hm… don't we want a validation error for config entities if you send multiple values for a single-value property?

(In other words: if you're sending a list or map for a boolean or string value?)

We should be totally able to to that. Let's give it a try. I would assume that this might trigger some alternative error message, as config validation/schema validation is triggered.

#115.15

We need to document the relation of this issue to #2869792: Add constraints to all config entities. I think the intent is that both of these can happen independently?

You are absolutely right. Both can and should happen totally in parallel.

I know this will fail the test for now, but I have to stop working for a quick.

Note: Once #2869809: Make it easy to get typed config representations of entities is in, we no longer need that.

Oh, nice! Updating issue title to reflect that this issue is blocked on that.

I initially thought this sounds a bit like a weird idea, but the more the think about it, this makes clear what the root cause of that is. One could have suggested something like: supports_update_rest, but you know, it does supports that. It just doesn't support the validation aspect of things.

Great! :)

Interdiff review

+++ b/core/lib/Drupal/Core/Entity/ContentEntityType.php
@@ -17,6 +17,11 @@ class ContentEntityType extends EntityType implements ContentEntityTypeInterface
+  protected $supports_validation = TRUE;

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -278,6 +278,13 @@ class EntityType extends PluginDefinition implements EntityTypeInterface {
+  protected $supports_validation = FALSE;

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -394,7 +394,7 @@ protected function getBaseRoute($canonical_path, $method) {
-    if ($this->isConfigEntityResource() && empty($this->entityType->get('rest_resource'))) {
+    if ($this->entityType->get('supports_validation')) {

Ohhhh, even better! This means all entity types have this in their annotation, and content entity types default to TRUE, all others default to FALSE — this makes total sense!

It's great to see things come together so nicely. Useful metadata across all entity types.

Better yet, this means that the ContentModerationState content entity can (and should!) set supports_validation = FALSE, i.e. it should opt out from the default for content entity types. At least if #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 continues the direction it's gone in, which is: that entity type is considered internal, does not allow access for any operation, and does not support validation.

This API addition in the form of a new annotation key therefore allows us to express things clearly, and not just for config entities.

I love it when things come together so nicely :)

Review

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -887,18 +899,22 @@ public function testPatch() {
-    $response = $this->request('PATCH', $url, $request_options);
-    $label_field = $this->entity->getEntityType()->hasKey('label') ? $this->entity->getEntityType()->getKey('label') : static::$labelFieldName;
-    $label_field_capitalized = $this->entity->getFieldDefinition($label_field)->getLabel();
-    $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\n$label_field: $label_field_capitalized: this field cannot hold more than 1 values.\n", $response);
+    if ($this->entity instanceof FieldableEntityInterface) {
+      $response = $this->request('PATCH', $url, $request_options);
+      $label_field = $this->entity->getEntityType()->hasKey('label') ? $this->entity->getEntityType()->getKey('label') : static::$labelFieldName;
+      $label_field_capitalized = $this->entity->getFieldDefinition($label_field)->getLabel();
+      $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\n$label_field: $label_field_capitalized: this field cannot hold more than 1 values.\n", $response);
+    }

Same remark as #114.13, which covered POSTing, this is testing PATCHing.

Rerolled to fix my nits rather than mentioning them in this comment.

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResourceAccessTrait.php
@@ -23,14 +23,16 @@
   protected function checkEditFieldAccess(EntityInterface $entity) {
+    if (!$entity instanceof FieldableEntityInterface) {
+      return;
+    }

Nice!!G

Boolean logic is hard ...

Some progress here ...

Let's fix the last failure ...

I think this is ready. This is now just hard-blocked on #2869809: Make it easy to get typed config representations of entities.

Great work :)

+++ b/core/config/schema/core.data_types.schema.yml
@@ -277,6 +277,10 @@ config_entity:
+          maxMessage: 'UUID: may not be longer than 128 characters.'

Nit: why this semicolon?

You mean colon? This was introduced to match the error message ...

Eh yes, colon.

But this patch also introduces the error message; why even have the colon in the error message?

But this patch also introduces the error message; why even have the colon in the error message?

I tried to have a message which is similar to:
$this->assertSame($this->serializer->encode(['message' => "Unprocessable Entity: validation failed.\nuuid.0.value: <em class=\"placeholder\">UUID</em>: may not be longer than 128 characters.\n"], static::$format), (string) $response->getBody());

Related: #2870878: Add config validation for UUIDs landed!

I think we can unpostpone it now again, right?

Here is a reroll for now, given we can now drop the custom validation for UUIDs.

Wasn't this actually blocked on #2869809: Make it easy to get typed config representations of entities, not #2870878: Add config validation for UUIDs? Or I guess it was blocked on both? It seems that's the case. It's only blocked on #2869809: Make it easy to get typed config representations of entities for the public function getSchemaWrapper() bit?

It's only blocked on #2869809: Make it easy to get typed config representations of entities for the public function getSchemaWrapper() bit?

well yeah. To be honest having uglyness for a short time is not necessarily wrong from a project managment point of view. I actually wanted it to be on needs review so the testbot picks it up again.

#2300677: JSON:API POST/PATCH support for fully validatable config entities is in, this is now unblocked! We just need a reroll now.

Here is a quick reroll. I'm really not sure what we should do with the error messages ...

The message now is:
Unprocessable Entity: validation failed.\nuuid: This is not a valid UUID.\n
which is drastically different to Unprocessable Entity: validation failed.\nuuid.0.value: UUID: may not be longer than 128 characters.\n

Why is this the case? One reason is that we don't actually use the UUID constrain when we validate UUID field items, but rather all we check is the length, in the case of content entities.

Should the validator actually check also for the length and as such provide a more helpful message? I'm personally not sure whether the length message actually provides a better error message.

I'm personally not sure whether the length message actually provides a better error message.

Me neither. I think the same UUID validation logic should be applied to content entities' UUIDs, to be honest. In fact, I'm shocked that that is apparently not already the case.

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -743,16 +755,23 @@ public function testPost() {
-      $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nuuid.0.value: UUID: may not be longer than 128 characters.\n", $response);
+      if ($this->entity instanceof FieldableEntityInterface) {
+        $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nuuid.0.value: UUID: may not be longer than 128 characters.\n", $response);
+      }
+      else {
+        $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nuuid: This is not a valid UUID.\n", $response);
+      }

So how about we keep this, but open a follow-up issue to make content entities also use the UUID constraint validator, just like #2870878: Add config validation for UUIDs?

In that follow-up, this if/else could be simplified to a single assertion.

@berdir gave some historic background to the discussion.
It was argued that we control config entities 100% so we know exactly how it should look like. On the other hand content entities might come from other sources and as such their UUIDs look different. Given that we can guarantee less. I think for now we should be able to ignore this problem then.

Works for me.

@Wim Leers
In other words we keep the behaviour as it is, right?

Yep.

I think the only thing that remains for this to be RTBC'd is for an Entity API maintainer to review this (catch, Berdir, plach or tstoeckler), as well as a Configuration Entity API maintainer (Alex Pott or Tim Plunkett).

d.o--

Updated IS.

Fixing nits. Removing dead code. Removing solved @todos.

Per #117, make ContentModerationState opt out from validation, because it's considered internal per #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.

The changes from @Wim Leers in #146 and #147 seems fine for me

So who can be motivate to review the patch, maybe berdir?

That would be wonderful.

We'll see about wonderful :)

1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -264,36 +264,38 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +    if ($entity instanceof FieldableEntityInterface) {
   +      foreach ($entity->_restSubmittedFields as $field_name) {
   +        $field = $entity->get($field_name);
   +
   +        // Entity key fields need special treatment: together they uniquely
   

   The if makes sense of course because only fieldable entities have those methods.

   But there is no alternative, so this means that you can patch all you want but $original_entity isn't actually changed for config entities?

   This gets back to my rant about REST tests in the translation support issue that we're not actually testing any successfull cases right now other than that there is no error?

   We even have a test for not changing something we shoudn't change, but we still have zero test coverage (as far as I see) to make sure that we actually *can* patch something? :)

   I guess we also commented out enough coverage for config entity PATCH to also not test any validation failures? because at least that should have failed because the original entity is valid?

2. +++ b/core/modules/rest/tests/modules/config_test_rest/config_test_rest.module
   @@ -26,5 +26,8 @@ function config_test_rest_config_test_access(EntityInterface $entity, $operation
      // Add permission, so that EntityResourceTestBase's scenarios can test access
      // being denied. By default, all access is always allowed for the config_test
      // config entity.
   -  return AccessResult::forbiddenIf(!$account->hasPermission('view config_test'))->cachePerPermissions();
   +  if ($operation === 'view') {
   +    return AccessResult::forbiddenIf(!$account->hasPermission('view config_test'))->cachePerPermissions();
   +  }
   +  return AccessResult::neutral();
    }
   

   this whole hook is now pointless because that is now already the default implementation.

1. We even have a test for not changing something we shoudn't change, but we still have zero test coverage (as far as I see) to make sure that we actually *can* patch something? :)

   I promise you that \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::testPatch() is actually verifying you can patch something.

   By default, it reuses getNormalizedPostEntity(). See \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::getNormalizedPatchEntity(). Specific entity types can override it.

   In this case, it's changing the label from Llama to Llamam.

   So, can you be a bit more specific?

2. Unless I'm mistaken, you're mistaken :P. \Drupal\config_test\ConfigTestAccessControlHandler::checkAccess() simply returns AccessResult::allowed(). This hook is changing that, so that a permission is needed for $op === 'view'. And the default is changed from ::allowed() to ::neutral().

1. It can execute a PATCH request without error. But unless I'm blind (which is possible), we have no assertion that the title change was actually *persisted*. The test is happy as long as there is a 200 response, which makes sense because the patch() method isn't really doing anything.

To rule out any blindness issues on my side, lets extend the test a bit to proof this :)

I think we should have a method to assert that each entity type PATCH actually correctly did what it had to do by reloading the entity and letting them assert the changes.

2. It used to do that. But this patch changes it and now both ConfigTestAccessControlHandler::checkAccess() and this hook now have a very similar implementation.

Again, to prove that, I just removed the hook now, and it's behaving as expected.

so the access tests did fail, but if I see that correctly, they only failed due to the different error message?

1. But unless I'm blind (which is possible), we have no assertion that the title change was actually *persisted*. The test is happy as long as there is a 200 response, which makes sense because the patch() method isn't really doing anything.

   This is absolutely true. Let's fix that. Opened #2882717: EntityResourceTestBase: assert that PATCHed and POSTed entities contain the intended values. Let's postpone this on that.

2. Ohhh, I see what you mean now! You're right.

#2882717: EntityResourceTestBase: assert that PATCHed and POSTed entities contain the intended values is blocked on review now. Once that's in, we can continue here.

Back to normal mode

Indeed, yay! :)

And also: this now needs a reroll, because #153's interdiff can safely be reverted (since #2882717: EntityResourceTestBase: assert that PATCHed and POSTed entities contain the intended values expanded the generic test coverage, which means #153's added test coverage is now obsolete). And also #153's non-test change can be reverted, because it now should fail tests without that.

Sure

Here is a fix :)

I think this looks great!

1. The key thing that I think we still need is … actual validation of config entities? This patch is currently only testing a subset of the keys that ConfigTest allows. The expected normalization for a GET request is:
   

     protected function getExpectedNormalizedEntity() {
       $normalization = [
         'uuid' => $this->entity->uuid(),
         'id' => 'llama',
         'weight' => 0,
         'langcode' => 'en',
         'status' => TRUE,
         'dependencies' => [],
         'label' => 'Llama',
         'style' => NULL,
         'size' => NULL,
         'size_value' => NULL,
         'protected_property' => NULL,
       ];
   
       return $normalization;
     }
   

   So I think we should have ConfigTestResourceTestBase implement the optional assertNormalizationEdgeCases(), where we could then assert that sending values for lots of these is not allowed. E.g. protected_property.

   For examples, see:

   • \Drupal\Tests\hal\Functional\EntityResource\HalEntityNormalizationTrait::assertNormalizationEdgeCases()
   • \Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDatetimeTest::assertNormalizationEdgeCases() \Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDateonlyTest::assertNormalizationEdgeCases()

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/ConfigTest/ConfigTestResourceTestBase.php
   @@ -23,10 +23,35 @@
   +  public function setUp() {
   

   Nit: missing "inheritdoc".

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -705,7 +704,14 @@ public function testPost() {
   +    if ($this->entity instanceof FieldableEntityInterface) {
   
   @@ -793,9 +799,14 @@ public function testPost() {
   +    if ($this->entity instanceof ConfigEntityInterface) {
   
   @@ -805,16 +816,23 @@ public function testPost() {
   +      if ($this->entity instanceof FieldableEntityInterface) {
   ...
   +    if ($this->entity instanceof FieldableEntityInterface) {
   

   This is not consistent.

4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -805,16 +816,23 @@ public function testPost() {
   +        $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nuuid.0.value: UUID: may not be longer than 128 characters.\n", $response);
   +      }
   +      else {
   +        $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nuuid: This is not a valid UUID.\n", $response);
   

   Perhaps an @see for each branch, pointing to the validation logic? Then it'll be clear to future readers of the patch why the validation errors are different.

5. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -861,7 +879,7 @@ public function testPost() {
   +      if ($created_entity instanceof  FieldableEntityInterface && $created_entity->hasField($field_name)) {
   
   @@ -1025,18 +1042,22 @@ public function testPatch() {
   +    if ($this->entity instanceof FieldableEntityInterface) {
   ...
   +    if ($this->entity instanceof FieldableEntityInterface) {
   
   @@ -1083,7 +1104,8 @@ public function testPatch() {
   +    if ($this->entity instanceof FieldableEntityInterface) {
   

   Nit: two spaces after "instanceof", should be one.

6. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -1083,7 +1104,8 @@ public function testPatch() {
   -    // Assert that the entity was indeed updated, and that the response body
   ...
   +      // Assert that the entity was indeed updated, and that the response body
        // contains the serialized updated entity.
        $updated_entity = $this->entityStorage->loadUnchanged($this->entity->id());
        $updated_entity_normalization = $this->serializer->normalize($updated_entity, static::$format, ['account' => $this->account]);
   

   The indentation is no longer correct here.

7. +++ b/core/modules/rest/tests/src/Functional/EntityResource/ConfigTest/ConfigTestResourceTestBase.php
   @@ -67,7 +92,33 @@ protected function getExpectedNormalizedEntity() {
   +  protected function makeNormalizationInvalid(array $normalization) {
   +    $normalization['label'] = ['foo', 'bar'];
   +    return $normalization;
   +  }
   
   +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -1303,7 +1325,15 @@ protected function getEntityResourcePostUrl() {
      protected function makeNormalizationInvalid(array $normalization) {
        // Add a second label to this entity to make it invalid.
        $label_field = $this->entity->getEntityType()->hasKey('label') ? $this->entity->getEntityType()->getKey('label') : static::$labelFieldName;
   -    $normalization[$label_field][1]['value'] = 'Second Title';
   +    if ($this->entity instanceof FieldableEntityInterface) {
   +      $normalization[$label_field][1]['value'] = 'Second Title';
   +    }
   +    else {
   +      $normalization[$label_field] = [
   +        $normalization[$label_field],
   +        'Second title',
   +      ];
   +    }
   

   I don't think we need this override in ConfigTestResourceTestBase because we've fixed the generic one in EntityResourceTestBase?

Keeping at NR because this patch would benefit from Entity API review too of course.

1. This makes sense. I turns out, even if its called protected, its just protected in some way. If you construct the config entity manually, which is what happens on denormalization, you can still change any value, as this test proves.

2-7. Addressed

Lookin' good. I read through the patch a couple of times and only have some minor things. It could well just me being silly.

Maybe 'supports_validation' is the wrong terminology. That seems less descriptive and more about the implementation. What I mean is this could be a kind of misleading correlation between the naming and the functionality it unlocks when used with rest?

EDIT: Deleted the rest of the review. I mis-read that the default to TRUE was on content entities.

I think the current fails are because the protected_property is not exported when toArray() is called (in ConfigEntityResource::validate) so it's not saved or anything either, but never validated against. Just ignored and discarded.

that might explain the POST fail, didn't check that, but I'm still not seeing an implementation for config entities in patch()? Which means we're still not actually doing anything there and also means we still don't actually have validation for that part for config entities?

patch() receives two entities, one is the original, loaded entity and the other ($entity) has the submitted values. We need to copy those to $original_entity. Right now, that only happens inside the if ($entity instanceof FieldableEntityInterface) { and there no else.

Yes, that seems like a very good point!

Rerolling

I can do let, let's see how this goes.

Just reroll #164

meh

Here is some test coverage + an actual implementation of patch for config entities.

1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -225,29 +226,37 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +    if ($entity instanceof ConfigEntityInterface && $original_entity instanceof ConfigEntityInterface) {
   

   I assume an entity could never be an instance of FieldableEntityInterface and ConfigEntityInterface, correct?

   If so why not but this is an elseif block off the FieldableEntityInterface if statement.

   Otherwise it looks like all the logic FieldableEntityInterface where access is checked would be overwritten by the FieldableEntityInterface which doesn't check access. I know this is not the case but why not make it explicit?

2. What are we going to do in the case of POSTing with a duplicate ID that already exists? I tried this and it is not caught by the validation and gets a 500 error which makes sense because typed data doesn't have a concept of this?

   Would we have to create a validator that extended \Drupal\Core\TypedData\Validation\RecursiveValidator to check for preexisting ids?

I assume an entity could never be an instance of FieldableEntityInterface and ConfigEntityInterface, correct?

If so why not but this is an elseif block off the FieldableEntityInterface if statement.

Yeah this is not supported at the moment. I remember @tim.plunkett trying to figure that out, but gave up.
I like to use elseif here :)

What are we going to do in the case of POSTing with a duplicate ID that already exists? I tried this and it is not caught by the validation and gets a 500 error which makes sense because typed data doesn't have a concept of this?
Would we have to create a validator that extended \Drupal\Core\TypedData\Validation\RecursiveValidator to check for preexisting ids?

Nice catch! This sounds like a generic problem outside of this issue. Maybe we should open its own issue for that?

Oops, here are the actual patches ...

Maybe we should open its own issue for that?

+1

What are we going to do in the case of POSTing with a duplicate ID that already exists? I tried this and it is not caught by the validation and gets a 500 error which makes sense because typed data doesn't have a concept of this?

Here it is: #2905594: Missing entity validation constraint: don't allow new entities when there is an existing one with the same ID

We no longer need a followup, and I've just written a basic change record.

#176 was 40 KB, #179 is 107 KB, despite a <1 KB interdiff. Some other cruft ended up in there. Rerolling.

I worked on a final review round for this patch. I can't find a single thing to complain about! Exciting! 😀

So, on to the change record and issue then:

• The CR's title was REST got full config entity support (POST, PATCH, DELETE), which is exaggerating quite a lot: there's no actual "full support" for config entities until #2869792: [meta] Add constraints to all config entity types is done. This is also the label for this issue in #2905563: REST: top priorities for Drupal 8.5.x so it's kind of my fault. Sorry! 😔 Fixed that at #2905563-5: REST: top priorities for Drupal 8.5.x. CR title updated.
• Issue title updated for similar reasons.
• The CR was incomplete: fixed. (It was especially missing information on how to add validation constraints.)
• Points in the IS that have been completed: marked them as done with <del>.

WOOOOT!

Major 👏 👏👏👏👏 to Daniel! 🎉

@Wim Leers
OH wow I would have not expected to not have change records for adding constraints when we added the functionality earlier ...

Hah, I was equally surprised :) But in the end, I think this makes sense — now we have one change record for this one unit of semantically related changes :)

+++ b/core/lib/Drupal/Core/Entity/ContentEntityType.php
@@ -17,6 +17,11 @@ class ContentEntityType extends EntityType implements ContentEntityTypeInterface
+  protected $supports_validation = TRUE;

+++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
@@ -31,6 +31,7 @@
+ *   supports_validation = FALSE,

This change is not a good one, IMHO. All entity types that implement FieldableEntityInterface have to support validation because of the three methods from that interface: validate(), isValidationRequired() and setValidationRequired().

Also, even if ContentModerationState is an "internal" entity type which should not be interacted with directly, why wouldn't it support validation?

Basically, I completely disagree with this proposed resolution point from the current issue summary:

Opt out content_moderation_state content entity type, to show that there's a use case for opting out particular content entity types

Instead of this new supports_validation flag on all entity types, I would suggest adding a new ValidatableEntityInterface (or ValidationEntityInterface) which would contain only those three methods mentioned above (validate(), isValidationRequired() and setValidationRequired()) and make config_test implement it.

Also, I was looking at EntityResourceValidationTrait, which has a @todo pointing here that it could be changed to not be @internal anymore in this issue. That is a trait with a single method which this patch does not use at all, so we need to either remove that @todo or maybe remove the trait and move its code into \Drupal\rest\Plugin\rest\resource\EntityResource.

#190: Thanks for reviewing!

Reading your comment, it seems most of your disagreement comes from opting out ContentModerationState from "supporting validation", right? I can understand that.

On top of that, you think that only @ConfigEntityType should have this supports_validation annotation key. And really, we shouldn't be adding any new annotation keys, but just use the existing validate() + isValidationRequired() + setValidationRequired() infrastructure. Which is currently tied to fieldable entities, but you're saying it could be extracted to a separate interface so that config entity types could opt in to it, but nothing would change for fieldable entity types.

Did I understand that correctly?

(Intentionally not replying to the last paragraph of #190 right now, because it's a minor thing compared to everything else in #190.)

In any case, 🙏 thank you for your review! 👏

#191: Yes, you understood it correctly :)

The patch from #184 does not apply to HEAD anymore because of #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior, but here's an interdiff for it to show how I think this should look like.

If you agree with it, we should open a separate issue for the new interface because it's kind of important and it shouldn't be tucked away at the bottom of an issue that's already at 200 comments.

+++ b/core/lib/Drupal/Core/Entity/FieldableEntityInterface.php
@@ -20,7 +20,7 @@
-interface FieldableEntityInterface extends EntityInterface {
+interface FieldableEntityInterface extends EntityInterface, ValidatableInterface {
 

So does that mean you will not be able to opt out of validation once you have any kind of fields? Note: This approach would actually not work in this example, because \Drupal\content_moderation\Entity\ContentModerationState extends ContentEntityBase, which is fieldable :(

@dawehner, see #190 for why we can't make any content entity type un-validatable. Since FieldableEntityInterface already has those three methods related to validation, it would be an API break at this point.

#192 looks awesome. +1 for a new issue to extract ValidatableEntityInterface. Would you like me to create that issue, or would you rather do that yourself, @amateescu?

If I then look at how it would affect the rest of the existing patch, then:

+++ b/core/modules/rest/src/Plugin/Deriver/EntityDeriver.php
@@ -88,6 +90,10 @@ public function getDerivativeDefinitions($base_plugin_definition) {
+        if ($entity_type instanceof ConfigEntityTypeInterface) {
+          $this->derivatives[$entity_type_id]['class'] = ConfigEntityResource::class;
+        }

+++ b/core/modules/rest/src/Plugin/rest/resource/ConfigEntityResource.php
@@ -0,0 +1,25 @@
+class ConfigEntityResource extends EntityResource {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function validate(EntityInterface $entity) {
+    // Use typed config to validate the validate the config entity.
+    /** @var \Drupal\Core\Config\TypedConfigManagerInterface $type_config_manager */
+    $type_config_manager = \Drupal::service('config.typed');
+    $typed_config = $type_config_manager->createFromNameAndData($entity->getConfigDependencyName(), $entity->toArray());
+    $violations = $typed_config->validate();
+
+    $this->processViolations($violations);
+  }
+
+}

We could then also drop this.

We could move the bulk of that logic to a ValidatableConfigEntityTrait (I don't think we'd want it in ConfigEntityBase), which would implement validate() on behalf of the config entity using that trait, and it'd just call typed config validation.

I think moving that to ConfigEntityBase would be sort of a bad move potentially. On purpose config entities are not tied to the concept of typed data, which IMHO is actually a really good thing. Typed data is sort of something you can do, but normally you should not have to care about it.

I think moving that to ConfigEntityBase would be sort of a bad move potentially.

I didn't propose that, and @amateescu didn't either:

We could move the bulk of that logic to a ValidatableConfigEntityTrait (I don't think we'd want it in ConfigEntityBase)

Do you also have concerns about that?

Here's the issue for the new interface: #2907163: Split validation-related methods from FieldableEntityInterface into a separate ValidatableInterface