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.

Remaining tasks

  • Review
  • Address feedback
  • RTBC
  • Commit

User interface changes

None.

API changes

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

Files: 
CommentFileSizeAuthor
#153 2300677-153-interdiff.txt2.17 KBBerdir
#153 2300677-153.patch29.32 KBBerdir
#147 2300677-147.patch28.72 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
#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

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

#2300677: [PP-1] Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST is in, this is now unblocked! We just need a reroll now.

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