Problem/Motivation

As an overall issue based on #2292707: GET on entity/taxonomy_vocabulary/{id} is not working it is weird we cannot POST/PATCH/DELETE for config entities.

Before #1928868: Typed config incorrectly implements Typed Data interfaces it was impossible for config entities to provide validation on the config level, so it was not possible to guarantee that the given input is valid.

Proposed resolution

  • Allow config entities to opt into REST support by adding a supports_validation annotation to their @ConfigEntityType annotation (i.e. opt in — the default is supports_validation = FALSE)
  • Make content entities use the same approach, but have it be opt-out — the default is supports_validation = TRUE
  • Leverage config validation to ensure the POSTed/PATCHed entities are valid
  • Opt in config_test config entity type, and implement config validation, to prove that the approach works.
  • Opt out content_moderation_state content entity type, to show that there's a use case for opting out particular content entity types
  • Open a Plan issue with dozens of child issues to add validation to all other config entity types: #2869792: Add constraints to all config entity types.

Remaining tasks

  • Remove delete support. Cleaning up deleted dependencies would open up a wormhole for now
  • Expand the test coverage to test schema and constraint validation

Next steps: #2869792: Add constraints to all config entity types.

User interface changes

None.

API changes

New @EntityType annotation: supports_validation. Defaults to FALSE. For @ContentEntityType, it defaults to TRUE.

CommentFileSizeAuthor
#213 2300677-213.patch38.24 KBtedbow
#208 interdiff-2300677.txt1.05 KBdawehner
#208 2300677-208.patch42.19 KBdawehner
#206 2300677-206-.patch41.9 KBJo Fitzgerald
#206 interdiff-200-206.txt2.66 KBJo Fitzgerald
#200 2300677-200.patch41.87 KBtedbow
#200 interdiff-199-200.txt8.52 KBtedbow
#199 interdiff-ValidatableInterface.txt8.54 KBtedbow
#199 2300677-199-ValidatableInterface.patch39.11 KBtedbow
#199 2300677-199.patch37.35 KBtedbow
#192 interdiff-192.txt8.58 KBamateescu
#184 2300677-184.patch39.6 KBWim Leers
#179 interdiff-2300677.txt855 bytesdawehner
#179 2300677-178.patch106.73 KBdawehner
#176 interdiff-2300677.txt10.59 KBdawehner
#176 2300677-176.patch39.72 KBdawehner
#174 2300677-173.patch31.58 KBdawehner
#164 interdiff-2300677.txt8.5 KBdawehner
#164 2300677-164.patch33.43 KBdawehner
#162 interdiff-2300677.txt2.39 KBdawehner
#162 2300677-162.patch30.52 KBdawehner
#160 2300677-160.patch29.13 KBdawehner
#153 2300677-153-interdiff.txt2.17 KBBerdir
#153 2300677-153.patch29.32 KBBerdir
#147 2300677-147.patch28.72 KBWim Leers
#147 interdiff.txt735 bytesWim Leers
#146 2300677-146.patch28.11 KBWim Leers
#146 interdiff.txt4.16 KBWim Leers
#138 interdiff-2300677.txt4.07 KBdawehner
#138 2300677-138.patch27.58 KBdawehner
#133 interdiff-2300677.txt525 bytesdawehner
#133 2300677-133.patch29.05 KBdawehner
#124 interdiff-2300677.txt691 bytesdawehner
#124 2300677-124.patch29.57 KBdawehner
#122 interdiff-2300677.txt3 KBdawehner
#122 2300677-122.patch29.56 KBdawehner
#120 interdiff-2300677.txt2.05 KBdawehner
#120 2300677-120.patch28.9 KBdawehner
#117 2300677-117.patch29.18 KBWim Leers
#117 interdiff.txt6.49 KBWim Leers
#115 interdiff-2300677.txt11.37 KBdawehner
#115 2300677-115.patch29.79 KBdawehner
#109 interdiff-2300677.txt874 bytesdawehner
#109 2300677-109.patch27.94 KBdawehner
#107 interdiff-2300677.txt1.5 KBdawehner
#107 2300677-107.patch27.94 KBdawehner
#101 interdiff-2300677.txt903 bytesdawehner
#101 2300677-101.patch27.54 KBdawehner
#96 2300677-96.patch27.53 KBdawehner
#94 2300677-94.patch55.23 KBdawehner
#94 interdiff-2300677.txt4.79 KBdawehner
#92 interdiff-2300677.txt7.94 KBdawehner
#92 2300677-92.patch62.97 KBdawehner
#86 2300677-86.patch46.83 KBdawehner
#86 2300677-review.patch18.61 KBdawehner
#85 2300677-review.patch11.77 KBdawehner
#85 2300677-85.patch39.99 KBdawehner
#80 2300677-80.patch7.85 KBdawehner
#64 interdiff.txt13.38 KBdawehner
#64 2300677-64.patch19.84 KBdawehner
#60 2300677-60.patch7.33 KBdawehner
#50 support_configentity-2300677-50.patch3.74 KB-enzo-
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch support_configentity-2300677-50.patch. Unable to apply patch. See the log in the details link for more information. View
#42 support_configentity-2300677-42.patch5.4 KB-enzo-
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch support_configentity-2300677-42.patch. Unable to apply patch. See the log in the details link for more information. View
#38 support_configentity-2300677-38.patch5.42 KB-enzo-
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch support_configentity-2300677-38.patch. Unable to apply patch. See the log in the details link for more information. View
#35 support_configentity-2300677-35.patch5.48 KBclemens.tolboom
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,472 pass(es), 1 fail(s), and 0 exception(s). View
#31 entity_menu_rest_resource.png46.71 KB-enzo-
#27 support_configentity-2300677-27.patch3.87 KB-enzo-
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,479 pass(es), 1 fail(s), and 0 exception(s). View
#25 support_configentity-2300677-25.patch5.76 KB-enzo-
#22 drupal8_rest_ui_entity_display_form.png103.37 KB-enzo-
#22 support_configentity-2300677-22.patch3.93 KB-enzo-
rest-config-entity.patch1.06 KBclemens.tolboom
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,371 pass(es), 1 fail(s), and 6 exception(s). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

clemens.tolboom’s picture

Berdir’s picture

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().

clemens.tolboom’s picture

@Berdir thanks for the quick response. But I don't understand. For me its 'just' an entity where rest manage permission. Why distinguish on Content versus Config? Patch seems valid to me :p

Patch skips field access for ConfigEntities checks but not entity view.

class EntityResource extends ResourceBase {

...
  public function get(EntityInterface $entity) {
    if (!$entity->access('view')) {
      throw new AccessDeniedHttpException();
    }
...

What do I miss?

Status: Needs review » Needs work

The last submitted patch, rest-config-entity.patch, failed testing.

tstoeckler’s picture

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.

Berdir’s picture

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...

clemens.tolboom’s picture

I just checked service 7.x-3.x module which exposed 2 config `entities`

  • taxonomy_vocabulary
  • system

I myself ran into what menu item belong to ie 'main' which both are config entities :(

Use case: A headless Drupal with AngularJS menu controller.

From the list below I'm not sure we need all ConfigEntities available but at least menu, menu_link, tour are very useful.

$ drush @drupal.d8 entity-type-read  `drush @drupal.d8 entity-type-read` | grep ConfigEntity | sort
...
    [action] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [block] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [block_content_type] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [breakpoint] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [breakpoint_group] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [comment_type] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [contact_category] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [date_format] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [editor] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [entity_form_display] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [entity_view_display] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [field_config] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [field_instance_config] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [filter_format] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [form_mode] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [image_style] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [menu] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [node_type] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [rdf_mapping] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [search_page] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [shortcut_set] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [taxonomy_vocabulary] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [tour] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [user_role] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [view] => Drupal\Core\Config\Entity\ConfigEntityType Object
    [view_mode] => Drupal\Core\Config\Entity\ConfigEntityType Object

@tstoeckler what code should be changed for CRUD through REST for ConfigEntities?

Berdir’s picture

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.

clemens.tolboom’s picture

I was only trying to list and select 'valid' reasons to expose a ConfigEntity. In the end permissions guard what is exposed.

I hope for some pointers to fix this bug.

Grayside’s picture

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.

moshe weitzman’s picture

Title: Rest should support ConfigEntity » Support ConfigEntity via REST
Category: Bug report » Feature request

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.

clemens.tolboom’s picture

@moshe weitzman it's good to focus on content.

I hope for some more pointers on how to solve this issue.

Wim Leers’s picture

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.

clemens.tolboom’s picture

When adding GET on ConfigEntityType we should block the other methods for sure otherwise we get core bug reports when people enable permissions on those methods.

clemens.tolboom’s picture

XREF #1897612-16: Add serializer support for YAML

The REST and CMI teams already concluded that since CMI files are best deployed via Git, not via REST, so we're not supporting ConfigEntities. That means this wouldn't be useful for doing configuration deployments.

clemens.tolboom’s picture

Issue summary: View changes

Trying building a web app we are blocked for not GETting Vocabulary and Menu and even just the site name.

What options do we have to expose these resources and what more resources are valid candidates to expose for GET?

-enzo-’s picture

Issue tags: +#amsterdam2014 #headlessdrupal

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.

clemens.tolboom’s picture

Issue summary: View changes

@-enzo- this issue has a patch already. What is wrong with that patch?

Note there is #2339815: Limit EntityResource to content entities, rather than them causing fatal errors which is doing the opposite.

It would be great to have more use cases into the summary so please add yours.

-enzo-’s picture

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

alansaviolobo’s picture

Issue tags: -#amsterdam2014 #headlessdrupal +Amsterdam2014, +#headlessdrupal
clemens.tolboom’s picture

Parent issue: » #2335229: [meta] REST et al
-enzo-’s picture

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": []
}
dmouse’s picture

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

clemens.tolboom’s picture

Issue tags: +needs test

Patch look great. Hope to review this week. This needs some tests.

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -32,6 +37,55 @@
    +
    

    Remove white line

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -32,6 +37,55 @@
    +  /**
    

    prepend white line

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -32,6 +37,55 @@
    +    public function __construct(
    +        array $configuration,
    +        $plugin_id,
    +        $plugin_definition,
    +        array $serializer_formats,
    +        LoggerInterface $logger,
    +        AccountProxyInterface $current_user) {
    +
    +        parent::__construct($configuration, $plugin_id, $plugin_definition, $serializer_formats, $logger);
    +
    +        $this->currentUser = $current_user;
    +      }
    +
    

    Why add line breaks in the arguments list? Grepping on core source tree show lots of single line __construct.

    Fix indentation of function body.

  4. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -43,12 +97,26 @@ class EntityResource extends ResourceBase {
    +    else if (is_a($entity, ContentEntityInterface)) {
    

    Use instanceOf instead

-enzo-’s picture

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

clemens.tolboom’s picture

General: this needs tests too to confirm the GET versus other ops work as expected.

  1. +++ b/core/lib/Drupal/Core/Entity/Plugin/rest/resource/BundleViewModesResource.php
    @@ -86,10 +86,10 @@ public function get($entity = NULL, $bundle = NULL) {
           }
     
    -      throw new NotFoundHttpException(t('Log entry with ID @id was not found', array('@id' => $id)));
    +      throw new NotFoundHttpException(t('Views modes for @entity and @bundle were not found', array('@entity' => $entity, '@bundle' => $bundle)));
         }
     
    -    throw new HttpException(t('No log entry ID was provided'));
    +    throw new HttpException(t('Entity or Bundle weren\'t provided'));
       }
     
    

    This seems unrelated.

  2. +++ b/core/lib/Drupal/Core/Entity/Plugin/rest/resource/EntityBundlesResource.php
    @@ -73,7 +73,7 @@ public function get($entity = NULL) {
    -      throw new NotFoundHttpException(t('Log entry with ID @id was not found', array('@id' => $id)));
    +      throw new NotFoundHttpException(t('Bundles for entity @entity were not found', array('@entity' => $entity)));
         }
    

    This seems unrelated.

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -34,6 +39,47 @@
    +        parent::__construct($configuration, $plugin_id, $plugin_definition, $serializer_formats, $logger);
    +        $this->currentUser = $current_user;
    +      }
    +
    

    Fix indentation.

-enzo-’s picture

Status: Needs work » Needs review
FileSize
3.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,479 pass(es), 1 fail(s), and 0 exception(s). View

Hi @clemens.tolboom

I remove unrelated code and fix indentation

-enzo-’s picture

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

Status: Needs review » Needs work

The last submitted patch, 27: support_configentity-2300677-27.patch, failed testing.

MartijnBraam’s picture

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.

-enzo-’s picture

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.

-enzo-’s picture

Status: Needs work » Needs review
FileSize
3.77 KB

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

clemens.tolboom’s picture

Issue summary: View changes
Status: Needs review » Needs work
clemens.tolboom’s picture

Ehm ... I did not delete file from #32 ... how could I delete that file!?!?

clemens.tolboom’s picture

Status: Needs work » Needs review
FileSize
5.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,472 pass(es), 1 fail(s), and 0 exception(s). View

I've tested using Rest UI to configure + permissions for anonymous with

curl --user admin:admin --header "Accept: application/json" --request GET http://drupal.d8/entity/user_role/anonymous
$ curl --user anonymous: --header "Accept: application/json" --request GET http://drupal.d8/entity/user_role/anonymous

both gives

{"uuid":"f5c18ef3-25f2-4442-9204-4cf8d72a4380","langcode":"en","status":true,"dependencies":[],"id":"anonymous","label":"Anonymous user","weight":0,"permissions":["use text format plain_text","access content","use text format restricted_html","access comments","access site-wide contact form","create article content","edit any article content","delete any article content","restful get entity:node","restful post entity:node","restful delete entity:node","restful patch entity:node","restful get entity:comment","restful post entity:comment","restful delete entity:comment","restful patch entity:comment","restful get entity:user","restful post entity:user","restful delete entity:user","restful patch entity:user","restful get entity:file","restful get entity:menu","restful get entity:user_role","restful get entity:taxonomy_vocabulary"]}

Attached patch adds a test as an example. It needs some fixes.

Status: Needs review » Needs work

The last submitted patch, 35: support_configentity-2300677-35.patch, failed testing.

clemens.tolboom’s picture

@-enzo- I used your patch from #32

Below the changes I did. Interdiff was better :-/ I added a test which 'nicely' failed.

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -45,12 +91,26 @@ class EntityResource extends ResourceBase {
    +    else if ($entity instanceof EntityDisplayBase) {
    

    Added space after if

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -45,12 +91,26 @@ class EntityResource extends ResourceBase {
    +      $permission = "restful get $plugin_definition->pluginId";
    

    Don't use label.

-enzo-’s picture

FileSize
5.42 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch support_configentity-2300677-38.patch. Unable to apply patch. See the log in the details link for more information. View

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’s picture

@-enzo- thanks for the feedback and sorry for the bad code :-/

Just remove devel module temporary.

I wrote #35 in order to show you were test could be written.

Test must be in this issue :-)

-enzo-’s picture

@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?

tim.plunkett’s picture

Issue tags: -needs test +Needs tests
-enzo-’s picture

FileSize
5.4 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch support_configentity-2300677-42.patch. Unable to apply patch. See the log in the details link for more information. View

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.

djbobbydrake’s picture

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?

Berdir’s picture

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.

-enzo-’s picture

@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

webchick’s picture

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’s picture

Today I revisited our tiny effort to mimic garland using Angular https://github.com/build2be/drupal-8-rest-angularjs

It could not list the term names as we have a list of nodes on taxonomy/term/:tid which renders the term on HTML but there is no equivalent in REST context. Adding a view we can add a list of terms which partly solves term names.

But IMHO it should not be necessary to build that view on every site. It should be out of the box.

To really build the mentioned Garland App menu items are really needed. As menu items are not config entities (what are they?) I guess a contrib module should solve that :-/

andypost’s picture

Related issues: +#2413769: Prevent the deletion of a bundle entity if there are existing content entities of that bundle
-enzo-’s picture

@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.

-enzo-’s picture

FileSize
3.74 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch support_configentity-2300677-50.patch. Unable to apply patch. See the log in the details link for more information. View

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.

davidwbarratt’s picture

Status: Needs work » Needs review

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

The last submitted patch, 38: support_configentity-2300677-38.patch, failed testing.

The last submitted patch, 42: support_configentity-2300677-42.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 50: support_configentity-2300677-50.patch, failed testing.

frob’s picture

Assigned: Unassigned » frob

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

Status: Needs work » Needs review
Wim Leers’s picture

Version: 8.0.x-dev » 8.1.x-dev
dawehner’s picture

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

Status: Needs review » Needs work

The last submitted patch, 60: 2300677-60.patch, failed testing.

Wim Leers’s picture

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.

Wim Leers’s picture

Priority: Normal » Major
Related issues: +#2650792: What permissions control REST's field_storage_config?

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
19.84 KB
13.38 KB

Made some progress:

  • The create test now actually works, yeah
  • Started to expand the read test
frob’s picture

Assigned: frob » Unassigned

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

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

dawehner’s picture

Title: Support ConfigEntity via REST » [pp-2] Support ConfigEntity via REST

Let's be more honest about it.

clemens.tolboom’s picture

Issue summary: View changes

Fixed links using _format now.

dawehner’s picture

frob’s picture

what does pp-2 mean?

clemens.tolboom’s picture

Wim Leers’s picture

Title: [pp-2] Support ConfigEntity via REST » [PP-1] Support ConfigEntity via REST
dawehner’s picture

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

Wim Leers’s picture

Done :P

Wim Leers’s picture

Title: [PP-1] Support ConfigEntity via REST » [PP-1] Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST
Wim Leers’s picture

Status: Needs review » Postponed

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.

dawehner’s picture

Issue tags: +DevDaysMilan

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

dawehner’s picture

FileSize
7.85 KB

Just some intermedia reroll, nothing to see :)

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Title: [PP-1] Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST » [PP-2] Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST
dawehner’s picture

Title: [PP-2] Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST » [PP-1] Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST
dawehner’s picture

dawehner’s picture

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.

The last submitted patch, 86: 2300677-review.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 86: 2300677-86.patch, failed testing.

Wim Leers’s picture

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.

dawehner’s picture

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?

Wim Leers’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
62.97 KB
7.94 KB

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 :)

Status: Needs review » Needs work

The last submitted patch, 92: 2300677-92.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.79 KB
55.23 KB

18 fails is still too much ...

Status: Needs review » Needs work

The last submitted patch, 94: 2300677-94.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
27.53 KB

Just a reroll after the other issue landed, yeah!

jibran’s picture

Title: [PP-1] Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST » Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST
Wim Leers’s picture

Tantalizingly close now!

Status: Needs review » Needs work

The last submitted patch, 96: 2300677-96.patch, failed testing.

dawehner’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
27.54 KB
903 bytes

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.

Wim Leers’s picture

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!

dawehner’s picture

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.

Berdir’s picture

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.

Wim Leers’s picture

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.

dawehner’s picture

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

dawehner’s picture

So something like this?

Status: Needs review » Needs work

The last submitted patch, 107: 2300677-107.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
27.94 KB
874 bytes

Boolean logic is hard.

dawehner’s picture

Issue summary: View changes
Wim Leers’s picture

#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 :)

dawehner’s picture

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.

Wim Leers’s picture

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.

Wim Leers’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs tests

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: Add constraints to all config entity types. I think the intent is that both of these can happen independently?
dawehner’s picture

Status: Needs work » Needs review
Issue tags: +Baltimore2017
FileSize
29.79 KB
11.37 KB

#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.

Status: Needs review » Needs work

The last submitted patch, 115: 2300677-115.patch, failed testing.

Wim Leers’s picture

Title: Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST » [PP-1] Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST
Status: Needs work » Needs review
Issue tags: +API addition, +Needs change record
FileSize
6.49 KB
29.18 KB

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.

dawehner’s picture

+++ 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

Status: Needs review » Needs work

The last submitted patch, 117: 2300677-117.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
28.9 KB
2.05 KB

Boolean logic is hard ...

Status: Needs review » Needs work

The last submitted patch, 120: 2300677-120.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
29.56 KB
3 KB

Some progress here ...

Status: Needs review » Needs work

The last submitted patch, 122: 2300677-122.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
29.57 KB
691 bytes

Let's fix the last failure ...

Wim Leers’s picture

Status: Needs review » Postponed

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?

dawehner’s picture

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

Wim Leers’s picture

Eh yes, colon.

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

dawehner’s picture

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());

Wim Leers’s picture

dawehner’s picture

Status: Postponed » Needs review

I think we can unpostpone it now again, right?

The last submitted patch, 85: 2300677-85.patch, failed testing.

The last submitted patch, 85: 2300677-review.patch, failed testing.

dawehner’s picture

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

Status: Needs review » Needs work

The last submitted patch, 133: 2300677-133.patch, failed testing.

Wim Leers’s picture

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?

dawehner’s picture

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.

Wim Leers’s picture

Title: [PP-1] Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST » Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST
dawehner’s picture

Status: Needs work » Needs review
FileSize
27.58 KB
4.07 KB

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.

Wim Leers’s picture

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.

dawehner’s picture

@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.

Wim Leers’s picture

Works for me.

dawehner’s picture

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

Wim Leers’s picture

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).

Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes

Updated IS.

Wim Leers’s picture

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

Wim Leers’s picture

dawehner’s picture

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

dawehner’s picture

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

Wim Leers’s picture

That would be wonderful.

Berdir’s picture

Status: Needs review » Needs work

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.

Wim Leers’s picture

Status: Needs work » Needs review
  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().
Berdir’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 153: 2300677-153.patch, failed testing.

Berdir’s picture

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

Wim Leers’s picture

Title: Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST » [PP-1] Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST
Status: Needs work » Postponed
Related issues: +#2882717: EntityResourceTestBase: assert that PATCHed and POSTed entities contain the intended values
  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.
Wim Leers’s picture

dawehner’s picture

Title: [PP-1] Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST » Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST
Status: Postponed » Needs review

Back to normal mode

Wim Leers’s picture

Status: Needs review » Needs work

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
29.13 KB

Sure

Status: Needs review » Needs work

The last submitted patch, 160: 2300677-160.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dawehner’s picture

Status: Needs work » Needs review
FileSize
30.52 KB
2.39 KB

Here is a fix :)

Wim Leers’s picture

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.

dawehner’s picture

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

Status: Needs review » Needs work

The last submitted patch, 164: 2300677-164.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

damiankloip’s picture

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.

damiankloip’s picture

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.

Berdir’s picture

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.

damiankloip’s picture

Yes, that seems like a very good point!

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tedbow’s picture

Issue tags: +Needs reroll

Rerolling

dawehner’s picture

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

tedbow’s picture

Status: Needs work » Needs review
FileSize
32.57 KB

Just reroll #164

dawehner’s picture

Issue tags: -Needs reroll
FileSize
31.58 KB

meh

Status: Needs review » Needs work

The last submitted patch, 174: 2300677-173.patch, failed testing. View results

dawehner’s picture

Status: Needs work » Needs review
FileSize
39.72 KB
10.59 KB

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

tedbow’s picture

  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?

dawehner’s picture

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?

dawehner’s picture

Oops, here are the actual patches ...

Wim Leers’s picture

Issue tags: +Needs followup

Maybe we should open its own issue for that?

+1

dawehner’s picture

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

dawehner’s picture

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

vaplas’s picture

  1. 💎💎💎Delightful work💎💎💎
  2. nit: #179 contains a side code (+Drupal 8.4.x, xxxx-xx-xx ...
Wim Leers’s picture

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

Wim Leers’s picture

Title: Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST » POST/PATCH/DELETE config entities via REST for config entity types that support validation
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Related issues: +#2869792: Add constraints to all config entity types

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:

Wim Leers’s picture

WOOOOT!

Major 👏 👏👏👏👏 to Daniel! 🎉

Wim Leers’s picture

Issue tags: +Entity validation
dawehner’s picture

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

Wim Leers’s picture

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 :)

amateescu’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

Wim Leers’s picture

#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! 👏

amateescu’s picture

FileSize
8.58 KB

#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.

dawehner’s picture

+++ 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 :(

amateescu’s picture

@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.

Wim Leers’s picture

#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.

dawehner’s picture

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.

Wim Leers’s picture

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?

amateescu’s picture

tedbow’s picture

Rerolled #184

Also a patch that adds suggest by @amateescu in #192 and interdiff between the 2.

I like this idea with the new trait.

tedbow’s picture

2300677-199-ValidatableInterface.patch will fail.

Here is a try to get the tests to pass. Added ConfigValidatableTrait for config_test to use.

The last submitted patch, 199: 2300677-199-ValidatableInterface.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 199: 2300677-199.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

+++ b/core/modules/config/tests/config_test/src/ConfigTestInterface.php
@@ -3,10 +3,11 @@
-interface ConfigTestInterface extends ConfigEntityInterface {
+interface ConfigTestInterface extends ConfigEntityInterface, ValidatableInterface {

This is technically a BC break. We should add it to the concrete implementation, not the interface.

dawehner’s picture

This is technically a BC break. We should add it to the concrete implementation, not the interface.

This is just for tests. I hope we don't treat tests as part of our API.

Wim Leers’s picture

True, but since this patch is meant to serve as an example, I think it makes sense to make the example unambiguous.

Jo Fitzgerald’s picture

Coding standards corrections.

Wim Leers’s picture

Title: POST/PATCH/DELETE config entities via REST for config entity types that support validation » [PP-1] POST/PATCH/DELETE config entities via REST for config entity types that support validation
Status: Needs review » Postponed
dawehner’s picture

What about this change? This does improve the readability a bit.

Wim Leers’s picture

Makes it easier to understand 👍

xjm’s picture

So there are a number of concerns raised about this by subsystem maintainers in #2907163: [PP-1] Split validation-related methods from FieldableEntityInterface into a separate ValidatableInterface:

  1. That config schema is already the way we validate config, and so implementing other validation could be superfluous or even dangerous.
  2. That config is not currently coupled to typed data and we want to keep it that way.
  3. That implementing such other validation inconsistently for some config entity types on a case-by-case basis or as a gradual API addition might actually be worse.

I recognize that the goal of this issue is to unblock REST functionality for config entities and it's not just a proposed architecture change in isolation. However, this could have pretty significant long-term consequences. I'd like to see this issue's summary updated with the counter-arguments to the proposed resolution here, and to get subsystem and framework manager input on this patch.

I'm also pretty sure there are issues out there about typed data validation vs. schema validation from before release, so we should track those down if possible.

dawehner’s picture

@tedbow and myself also talked a bit how this issue relates to #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts
That issue introduces the concept of exposable, so something which should be exposed to the outer world.

The problem we have in this issue is not the same to be honest, as we just want to hide the config entities when they don't yet implement validation properly. In an ideal future we should be able to assume that all config entities have the proper constrains, given that, no matter how we filter out the types now, it ideally won't have to stick around in 9.x.

tedbow’s picture

@dawehner I thought from our discussions the problem was not just that we want to hide certain config entities that don't support validation is that some config entities should be internal i.e. not exposed via a REST API(or JSON API) regardless of whether they support validation or not, ContentModerationState being the first example of that in core.

Would it make sense to got back to approach in #184 and before except instead of

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

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

Don't add this but
protected $internal_only = FALSE

Then ContentModerationState would set this to TRUE and REST(and JSON API/Graphql etc) would never expose this.

tedbow’s picture

Status: Postponed » Needs review
FileSize
38.24 KB

UPDATE: sorry this patch/approach is not really relevant to this issue. Feel free to skip, see #216

Here is re-roll of #184 but with using internal_only instead of supports_validation. So it only requires marking \Drupal\Core\Entity\EntityType as internal_only = FALSE then \Drupal\content_moderation\Entity\ContentModerationState as internal_only = TRUE.

If we look at phpdoc for ContentModerationState

 * @internal
 *   This entity is marked internal because it should not be used directly to
 *   alter the moderation state of an entity. Instead, the computed
 *   moderation_state field should be set on the entity directly.

Marking it internal for any REST API(or JSON API) purposes actually makes sense. If it should not be used programmatically directly but rather a field should be set on the entity then should be the case for REST also.

Currently I have just changed \Drupal\rest\Plugin\rest\resource\EntityResource::availableMethods to use internal_only but this of course creates a Read-only REST resource. Should this and other internal_only not be part of the API at all? If so we would want to update \Drupal\rest\Plugin\Deriver\EntityDeriver::getDerivativeDefinitions and not make derivatives at all for interanal_only entities.

If we want Drupal to be API First we have to have way to mark entities as not part of the API and not make the assumption that all entities are part of an external API.

Re @xjm's comment in #210

I recognize that the goal of this issue is to unblock REST functionality for config entities and it's not just a proposed architecture change in isolation. However, this could have pretty significant long-term consequences.

If we take this patches approach then we won't necessarily have "pretty significant long-term consequences". Validation for now won't be encapsulated at all in config entities and won't be tied to typed data but if #2907163: [PP-1] Split validation-related methods from FieldableEntityInterface into a separate ValidatableInterface seems to be the way to go later and we want to use the new ValidatableInterface on config entities then we are free to do so later. We could then just get rid of \Drupal\rest\Plugin\rest\resource\ConfigEntityResource(mark as internal now) because it is only needed for the difference in validation between config and content entities.

I'm also pretty sure there are issues out there about typed data validation vs. schema validation from before release, so we should track those down if possible.

Right now I left \Drupal\rest\Plugin\rest\resource\ConfigEntityResource::validate using \Drupal::service('config.typed') but that could easily be changed.

Wim Leers’s picture

#212: I think we can leave ContentModerationState out of the discussion here. We can discuss it in a follow-up. We already have content_moderation_rest_resource_alter() to ensure rest.module doesn't expose it (nor the contrib https://www.drupal.org/project/restui module).


The central question in this issue is: how can we let config entity types A) have support for validation, B) signal their support for validation on per-config entity type basis.

The 3 concerns explained in #210 show that the concern here is that we introduce i) coupling, ii) inconsistencies.

I'd like to see this issue's summary updated with the counter-arguments to the proposed resolution here

And I would like to see counterproposals from those who have counter arguments to the proposed resolution of the current patch.

Status: Needs review » Needs work

The last submitted patch, 213: 2300677-213.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tedbow’s picture

re

The central question in this issue is: how can we let config entity types A) have support for validation, B) signal their support for validation on per-config entity type basis.

Ok #212 & #213 probably misses the point of the issue. Sorry will update #213

tedbow’s picture

Status: Needs work » Postponed

Setting back to postponed as it was before #213

dawehner’s picture

I was wondering whether we can have a meeting to discuss this issue? It feels like we are kinda stuck right now.

amateescu’s picture

That's a very nice idea! Good thing that most of us will be in Vienna next week :)

Wim Leers’s picture

Status: Postponed » Needs review

Agreed that a meeting would help. Both dawehner and amateescu were invited to the "API-First" Hard Problems meeting next week in Vienna, it probably makes sense to discuss this in that meeting.

Wim Leers’s picture

Status: Needs review » Postponed

IDK how that status change happened.

tedbow’s picture

Adding related issue #1818574: Support config entities in typed data EntityAdapter
This would allow us to validate config entities in the same way as \Drupal\Core\Entity\ContentEntityBase::validate

dawehner’s picture

Issue summary: View changes
vaplas’s picture

The more additional issues, the better :)

dawehner’s picture

1️⃣ It feels like it would be worth creating an issue simply to discuss, how we want to allow config entities to opt out of rest support, maybe for validation or some other reasons. Making a decision on that point we help moving this issue forward.
2️⃣ As part of the discussion in vienna we wanted to ensure that we have a decent test coverage. It feels like laying what we have already in the issue summary would be great to get an overview and help the committers.
3️⃣ Anything else?

amateescu’s picture

It feels like it would be worth creating an issue simply to discuss, how we want to allow config entities to opt out of rest support, maybe for validation or some other reasons. Making a decision on that point we help moving this issue forward.

If some kind of generic validation is added to all config entity types, I'm pretty sure that they can use the existing mechanism for opting out of being exposed by REST, which is hook_rest_resource_alter.

dawehner’s picture

If some kind of generic validation is added to all config entity types, I'm pretty sure that they can use the existing mechanism for opting out of being exposed by REST, which is hook_rest_resource_alter.

When we don't have any kind of official API/flag other implementations like jsonapi, relaxed and graphql can't opt out either.

Sam152’s picture

We have no way of telling if an entity type has correctly provided constraints to validate things which may otherwise have been validated in forms or methods on the entity class. For that reason I think this has to be opt in? From a DX perspective, I think a property on the annotation makes the most sense. Just a shame we'll have a duality between how config and content entities are supported.

I'm interested to know how access will work with REST too. For the workflow entity the "update" operation doesn't grant you access to modify the entire tree of configuration. We have additional operations like delete-state:foo. Some extra background in #2896726: Expand the entity access model for workflow states and transitions..

I can't see many other implementations of this nature, but it does raise an interesting question: can we assume that authors of config entities intended the "update" access opperation to grant unfettered access to the entire config object. What if, I as an author only exposed a subset of the tree to the UI and it was my never my intention for the whole thing to be updatable by a user. Is opting into rest support off the table for me now?

Totally random example but in the views config object, it makes no sense for anyone to ever directly modify the cache_metadata key. Does "update" on view grant me this privilege. How do I support rest safely?

dawehner’s picture

You are rising indeed good questions. Maybe this actually means we need field level access or just a list of values you can actually manipulate ...

webchick’s picture

This is probably a terrible idea, but just spitballing. Do we need a new set of "opt-in" hooks? hook_rest_insert(), hook_rest_update(), etc? Might be a bit of code duplication, but that way we can surmise that the module author has considered some of these implications.

Sam152’s picture

Possibly some entity handler to smooth over any differences that can't be inferred generically?

Another question I had around this was in relation to plugins and schema provided by plugins. As a config entity with a plugin collection, how do I ensure all plugin implementations have also correctly provided constraints for their schema? Opting into rest support could possibly be seen as a BC break or expose an access bypass, because as an plugin implementor the only way config was updated was via the configuration forms I exposed and presumably validated.

I think one option could be, opt-in REST support for simple, fully validated configuration schema and for complex configuration entities there could be a framework around being able to provide custom REST resources which expose safer domain-specific endpoints. To continue the views example, safely providing support for an opperation like "adding/update a display" could live on an endpoint like /entity/view/{view_id}/add-display/{handler}/{display_id} and the associated handler would only use methods on DisplayPluginInterface, which would presumably provide more safety than replacing a blob of configuration. In the previous example, the cache_metadata can't be set with this interface and would be calculated as a result of this operation. It would be a far less glamorous option than managing to solve every entity type in one go, but the issues raised with the generic solutions so far seem significant.

From a DX perspecitve, something like link templates and "form/route_provider" handers, but for rest?

dawehner’s picture

Let's stop merging everything into one issue: #2920756: Discuss how config entity types can opt out of REST support

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

While #2920756: Discuss how config entity types can opt out of REST support is one thing, we still want to disable certain entity types as long they don't have validation support. Do we plan to make them internal till they have proper validation?

tedbow’s picture

Do we plan to make them internal till they have proper validation?

I don't think that will work. I think internal config entity types should not be exposed via an API at all. @see #2941622: Make REST's EntityResource deriver exclude internal entity types and mark ContentModerationState entity type as internal instead of the current hack
While as most config entity types will be exposed now for GET and we are looking for way to mark them validatable on Entity type basis so they can be exposed for POST and PATCH.

But then we also want to not add DELETE support because dependencies get very complicated on deletion.

So if we were add the 'supports_validation' now to the entity type annotation and later we wanted to DELETE support to some entity types via API what would we add for those entity types, 'supports_delete', 'supports_delete_validation'?

Maybe instead of tying the idea of exposing certain operations on entity types via the API to validation we explicitly add 'api_supported_operations' array to entity type annotation.

'api_supported_operations' then for content entity types this could default to all, and for config entities this could default to ['GET'].

Then core could choose for now to not expose operations all methods on config entity types and to update certain entity types to support more operations as they gain validations and it made sense to expose them.

The thing is even if we went the method of ValidatableInterface would that always mean that we wanted to expose POST/PATCH on those entity types? ValidatableInterface would help for better code even if we had no API exposing modules. So we may want to add entity types that can be validated programmatically and still only want to expose GET for them.

I think an example for would be a config entity type that is really a child of another type that only gets created programmatically when the parent entity gets created.

dawehner’s picture

The thing is even if we went the method of ValidatableInterface would that always mean that we wanted to expose POST/PATCH on those entity types? ValidatableInterface would help for better code even if we had no API exposing modules. So we may want to add entity types that can be validated programmatically and still only want to expose GET for them.

One of the problems of relying purely on a ValidatableInterface I see is that config entities should always be validatable, at least conceptually as they have config schema.

'api_supported_operations' then for content entity types this could default to all, and for config entities this could default to ['GET'].

I really like this explicitness. Exposing this annotation on the entity level seems to make entities more API first.