REST module is currently hard-coded to JSON-LD. We want to be able to configure what format should be available on what resource operation. Example: I want to create nodes with POST requests that carry XML, but I want to forbid all other formats. I want to retrieve nodes with GET requests that ask for JSON-LD or XML, but I want to forbid all other formats.

Depends on #1850704: Available serialization formats.

CommentFileSizeAuthor
#17 format-config-1850734-17.patch20.38 KBklausi
#17 format-config-1850734-17-interdiff.txt1.08 KBklausi
#16 format-config-1850734-16.patch18.07 KBklausi
#15 format-config-1850734-15.patch18.08 KBklausi
#15 format-config-1850734-15-interdiff.txt6.25 KBklausi
#10 format-config-1850734-10.patch21.97 KBklausi
#10 format-config-1850734-10-interdiff.txt4.92 KBklausi
#8 format-config-1850734-8.patch21.04 KBklausi
#8 format-config-1850734-8-interdiff.txt1.98 KBklausi
#6 format-config-1850734-6.patch20.13 KBklausi
#5 format-config-1850734-5.patch24.14 KBklausi
#5 format-config-1850734-5-interdiff.txt1.8 KBklausi
#3 format-config-1850734-3.patch21.93 KBklausi
#3 format-config-1850734-3-interdiff.txt4.84 KBklausi
#1 format-config-1850734-1.patch17.45 KBklausi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Active » Needs review
FileSize
format-config-1850734-1.patch17.45 KB

Here is a first patch that attempts to make resource operations (GET, POST, PUT etc.) as well as serialization formats (drupal_jsonld, jsonld, xml etc.) configurable. It contains mostly changes to the admin UI to add nested options for configuration. Please forgive me for the poor usability, but since that won't be a settings page that is often used I just implemented those nested checkboxes. Better suggestions are welcome!

This is not yet finished, I just worked on GET/read test case. The rest will probably fail horribly. Posting my intermediate work to keep you up to date and to give you a chance to chime in early.

Status: Needs review » Needs work

The last submitted patch, format-config-1850734-1.patch, failed testing.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs work » Needs review
FileSize
format-config-1850734-3-interdiff.txt4.84 KB
format-config-1850734-3.patch21.93 KB

Now at least the existing test cases should pass.

TODO:
* Implement the Content-Type format restriction for POST/PUT/PATCH write operations. The routing does not handle that for us
* Look into extending the test cases and what we have to cover.

Status: Needs review » Needs work

The last submitted patch, format-config-1850734-3.patch, failed testing.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs work » Needs review
FileSize
format-config-1850734-5-interdiff.txt1.8 KB
format-config-1850734-5.patch24.14 KB

Implemented the format restriction for incoming data now and merged with #1850704: Available serialization formats.

TODO: Look into extending the test cases and what we have to cover.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
FileSize
format-config-1850734-6.patch20.13 KB

Rerolled after #1850704: Available serialization formats was committed. No other changes.

Status: Needs review » Needs work

The last submitted patch, format-config-1850734-6.patch, failed testing.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs work » Needs review
FileSize
format-config-1850734-8-interdiff.txt1.98 KB
format-config-1850734-8.patch21.04 KB

Some interesting things changed in the routing system, this should fix the read test case.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Needs work 
+++ b/core/modules/rest/lib/Drupal/rest/EventSubscriber/RouteSubscriber.php
@@ -54,15 +54,27 @@ public function __construct(ResourcePluginManager $manager, ConfigFactory $confi
+    $enabled = $this->config->get('rest.settings')->load()->get('resources');

$enabled sounds like a boolean, not a list of enabled resources. This should probably be $enabled_formats or $enabled_resources or something more descriptive.

+++ b/core/modules/rest/lib/Drupal/rest/EventSubscriber/RouteSubscriber.php
@@ -54,15 +54,27 @@ public function __construct(ResourcePluginManager $manager, ConfigFactory $confi
+          // No format restrictions are configured, so always add the route.
+          if (empty($enabled_methods[$method])) {
+            $collection->add("rest.$name", $route);
+            continue;

The comment doesn't match the code. The comment says "no format restrictions", but the code is checking methods. Vis, it's adding the route no matter what if it isn't filtering on method, but it could still be filtering on format, no?

+++ b/core/modules/rest/lib/Drupal/rest/RequestHandler.php
@@ -59,9 +71,15 @@ public function handle(Request $request, $id = NULL) {
+      $format = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT)->getRequirement('_format');
+      // If there is no format associated just pick Drupal JSON-LD.
+      if (!$format) {
+        $format = 'drupal_jsonld';

This can be collapsed to an abbreviated ternary.

$request->blah ?: 'drupal_jsonld';

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs work » Needs review
FileSize
format-config-1850734-10-interdiff.txt4.92 KB
format-config-1850734-10.patch21.97 KB

Thanks, fixed that. I guess the format restriction setting per method is confusing, it works the usual Drupal way of "if nothing is selected all must be allowed".

Also added the public method to the resource interface.

Status: Needs review » Needs work

The last submitted patch, format-config-1850734-10.patch, failed testing.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs work » Needs review

#10: format-config-1850734-10.patch queued for re-testing.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Reviewed & tested by the community

Thanks, klausi!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Usability

Going to ask for a usability review. I'm not sure we really need a UI for this - we still don't have one for RSS feeds for example.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
FileSize
format-config-1850734-15-interdiff.txt6.25 KB
format-config-1850734-15.patch18.08 KB

So here is a patch that does not make the additional configuration options available in the UI. If a resource is enabled in the UI simply all methods and all formats are enabled for it.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
FileSize
format-config-1850734-16.patch18.07 KB

Patch does not apply anymore, rerolled. No other changes.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Issue tags: +WSCCI
FileSize
format-config-1850734-17-interdiff.txt1.08 KB
format-config-1850734-17.patch20.38 KB

Patch does not apply anymore, rerolled.

Also simplified the dblog resource, see interdiff.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Issue tags: +Stalking Crell

Also tagging for Crell.

Crell’s picture

Crell CreditAttribution: Crell commented

Klausi: Can you generate some screenshots here to help the UX folks give feedback?

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Issue tags: -Usability

Actually I want to move on with the recent non UI changing patch, so that we have the settings available in YAML and can add a proper UI later or in contrib.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Priority: Normal » Major

Raising priority to major since it would be really bad to ship REST module with a hard coded format (currently JSON-LD).

I also opened #1927162: Remove REST module's UI to discuss the fate of REST module's UI as a follow-up. So let's leave the usability question out of this issue to move forward.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Reviewed & tested by the community

I'm OK with punting on the UI. There's things to discuss there yet, and I don't want to hold up API flexibility on it. We can follow up in the other issue.

Back to RTBC again.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

Automatically closed -- issue fixed for 2 weeks with no activity.