bshaffer hit a DX issue when testing REST. He tested it without custom config, and JSON worked. Then he added hal_json: TRUE and the JSON stopped working. This is by design, but it does seem like a DX issue.

He suggested changing the format from:

GET:
  hal_json: 'TRUE'

to

GET:
  supported_formats:
    - hal_json
CommentFileSizeAuthor
#6 interdiff.txt2.55 KBlinclark
#6 2011832-06-rest-config.patch2.58 KBlinclark
#4 2011832-04-rest-config.patch2.57 KBlinclark
#3 2011832-03-rest-config.patch2.57 KBlinclark
#3 interdiff.txt959 byteslinclark
#1 2011832-01-rest-config.patch2.52 KBlinclark
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

linclark’s picture

linclark CreditAttribution: linclark commented
Status: Active » Needs review
FileSize
2011832-01-rest-config.patch2.52 KB

This patch passes tests. I don't know if this means it works or that we don't have full test coverage.

linclark’s picture

linclark CreditAttribution: linclark commented
Issue summary: View changes

Updated issue summary.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Needs work 
+++ b/core/modules/rest/lib/Drupal/rest/EventSubscriber/RouteSubscriber.php
@@ -75,7 +75,7 @@ public function dynamicRoutes(RouteBuildEvent $event) {
           // If there is no format requirement or if it matches the
           // configuration also add the route.
           $format_requirement = $route->getRequirement('_format');
-          if (!$format_requirement || isset($enabled_methods[$method][$format_requirement])) {
+          if (!$format_requirement || in_array($format_requirement, $enabled_methods[$method]['supportedFormats'])) {
             $collection->add("rest.$name", $route);
           }

This will throw PHP notices if the supportedFormats key is not set (all formats allowed).

+++ b/core/modules/rest/lib/Drupal/rest/RequestHandler.php
@@ -51,8 +51,8 @@ public function handle(Request $request, $id = NULL) {
-      if (empty($enabled_formats) || isset($enabled_formats[$format])) {
+      $method_settings = $config[$plugin][$request->getMethod()];
+      if (empty($method_settings['supportedFormats']) || in_array($format, $method_settings['supportedFormats'])) {
         $definition = $resource->getPluginDefinition();
         $class = $definition['serialization_class'];

Same here, right?

Otherwise looks good to me.

linclark’s picture

linclark CreditAttribution: linclark commented
Status: Needs work » Needs review
FileSize
interdiff.txt959 bytes
2011832-03-rest-config.patch2.57 KB

if (empty($method_settings['supportedFormats']) || in_array($format, $method_settings['supportedFormats']))

In an OR, doesn't it evaluate the first, and if it returns TRUE then it doesn't evaluate the second? Thus, if supportedFormats is not set, it will pass the first condition and not run the second?

I've corrected the other. Eventually, we might want to consider whether this check should go in a method. Can the settings be a configurable with its own methods?

linclark’s picture

linclark CreditAttribution: linclark commented
FileSize
2011832-04-rest-config.patch2.57 KB

Typo will make that last one fail.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Reviewed & tested by the community

Right, the second OR is of course fine.

Although checking these settings is a bit tedious I don't think it warrants the creation of a config entity with a method for REST module.

We need to update the docs at https://drupal.org/documentation/modules/rest after this lands.

Looks RTBC to me!

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Issue summary: View changes

camel case, yay

linclark’s picture

linclark CreditAttribution: linclark commented
FileSize
2011832-06-rest-config.patch2.58 KB
interdiff.txt2.55 KB

Klaus said in IRC that he preferred supported_formats to supportedFormats, and I agree. I asked alexpott, and he said that was fine, so switching back.

Dries’s picture

Dries CreditAttribution: Dries commented

It's not clear how the new format fixes the DX issue. Care to explain a bit better?

linclark’s picture

linclark CreditAttribution: linclark commented

The problem was that the user was confused what 'json' => TRUE meant. By explicitly stating that this is a list of supported formats, it makes it clearer that something that if it is not in the list, it is not a supported format. It also makes the API more open to additions later down the road, if we determine we need additional settings for the methods.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work

I think we should use YAML commits in the default rest.settings.yml to explain how the file can look like...

Then changes like this become easier to review...

Setting back to needs works because I think this is an ideal place to do this... as we are changing the format... but I really like the change... config files that can be read and the meaning intuited are a good thing.

linclark’s picture

linclark CreditAttribution: linclark commented
Status: Needs work » Needs review

This is related to another issue... I think that we should enable methods by default on nodes. See #2031647: Provide minimal default rest configuration.

Would it be alright if we address the documentation in that issue?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs review » Reviewed & tested by the community

Okay after discussing this with @linclark #2031647: Provide minimal default rest configuration will add the docs...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 7f2133b and pushed to 8.x. Thanks!

klausi’s picture

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

I fixed the example YAML config on https://drupal.org/documentation/modules/rest

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Updated issue summary.