REST UI does not support "resource" granularity

It seems the problem occurs due to a wrong settings configuration

This doesn't work

langcode: es
status: true
dependencies:
  module:
    - basic_auth
    - hal
    - node
id: entity.node
plugin_id: 'entity:node'
granularity: resource
configuration:
  methods:
    - GET
    - POST
    - PATCH
    - DELETE
  formats:
    - hal_json
  authentication:
    - basic_auth
  GET:
    supported_formats:
      - hal_json
      - json
    supported_auth:
      - basic_auth
      - cookie

But changing this make it work

configuration:
  methods:
    - GET
    - POST
    - PATCH
    - DELETE
  formats:
    - hal_json
    - json
  authentication:
    - basic_auth
    - cookie
  GET:
    supported_formats:
      ..

I had a similar issue. Rest UI would set the "supported_auth" but not the "authentication" portion of the config. This caused all POST requests to fail because it would try to check with basic_auth instead of cookie, except I wasn't using basic_auth.

Documented here:
https://www.drupal.org/node/2831251#comment-11813795

For drupal 8.2 install the module REST UI 8.x-1.x-dev i have problems with the REST UI module 8.x-1.13, resolved by the dev version module

As today, it seems there's no a difference between both branches. After downloading the module using git
git diff 8.x-1.13 8.x-1.x doesn't output anything

In fact, 8.x-1.13 was released after the last commit to the dev version

I've just inspected the code and it seems the issue is produced due to the settings in rest.resource.entity.node.yml which are not changed when editing the configuration with REST UI

The problem with nodes occurs because when installing the rest module, the entity.node REST resource configuration is created with 'resource' granularity. However, REST UI uses 'method' granularity. This patch fixes the issue.

I cannot reproduce using

- Drupal 8.3.x
- Rest UI 8.x-1.x

using Basic Auth. I've tested with

curl --user admin:admin --header 'Accept: application/json' --request GET 'http://drupal.d8/node/1?_format=json'

[edit]
My config is

uuid: bf40fccb-bca2-49cf-8e16-625fda5eecb2
langcode: en
status: true
dependencies:
  module:
    - basic_auth
    - node
    - serialization
    - user
id: entity.node
plugin_id: 'entity:node'
granularity: method
configuration:
  GET:
    supported_formats:
      - json
    supported_auth:
      - basic_auth
      - cookie
  POST:
    supported_formats:
      - json
    supported_auth:
      - basic_auth
      - cookie
  DELETE:
    supported_formats:
      - json
    supported_auth:
      - basic_auth
      - cookie
  PATCH:
    supported_formats:
      - json
    supported_auth:
      - basic_auth
      - cookie

[/edit]

Oh, it seems I generated the patch wrongly. Sorry for that.

@clemens.tolboom, I don't know if the default settings were changed in 8.3.x since I haven't checked this issue again. The problem was that granularity was set to "resource" by default in core. However, Rest UI relies on adding settings to every method. I'll check if the default configuration was changed in core and let you know.

Core still uses 'resource' as the default granularity when installing rest module. However, this configuration will only be added if the dependencies are enabled. In this case if you enable hal, basic_auth and node, this configuration, which uses 'resource' as granularity, will be added.

If you only install rest_ui, rest, and serialization, you shouldn't get this issue. Yet I guess many site builders might tend to enable http authentication and hal along with rest. Then, they will have this issue

How is it possible that #2758563: Handle core 8.2.x changing REST configuration to use configuration entities did not fix this?

… installed this myself. And indeed, WTF, it does not work correctly at all.

uuid: 453c411a-b207-49e6-9f56-134b8ff86430
langcode: en
status: true
dependencies:
  module:
    - basic_auth
    - hal
    - node
_core:
  default_config_hash: t_jfECmZhJqBOJuSOFn87EOi_TWi-_fRYTuJgd19vgg
id: entity.node
plugin_id: 'entity:node'
granularity: resource
configuration:
  methods:
    - GET
    - POST
    - PATCH
    - DELETE
  formats:
    - hal_json
    - xml
  authentication:
    - basic_auth
    - cookie

is displayed like this:

It's this module's responsibility to nudge people to use the simpler, more maintainable, more easily configurable approach: resource-level granularity. The method-level granularity should be an advanced option, and preferably would not even be exposed in the UI.

This patch fixed the issue in a project I worked on. I don't know if it's the best approach as it expects that the settings form for nodes will be saved before affecting the default Drupal configuration.

It does not.

The UI itself needs to be changed.

Actually, a lot of work still needed to be done to support resource granularity. The attached patch adds the following:

- A select box on the form to choose between resource an method granularity, resource being the default
- Correctly rebuilding the form based on the granularity selection
- Correctly loading defaults for existing configs
- Form validation for resource granularity
- Submit handling for saving resource granularity
- Correct displaying of resource granularity on the resource list page

Here is some review feedback. I did not test the patch extensively. Someone more familiar with the expected functionality might do this.
- Comments are running beyond 85 chars, mainly due to a reformat that aligns function parameter annotations.
- Normal code is restricted to 85 char lines, reducing legibility.
- style attributes in code: RestUIForm.php:218: better to add a class, and make style changes in your theme.
- array() and [] used inconsistently. Use only [] as per D8 coding standards.

+++ b/src/Form/RestUIForm.php
@@ -147,32 +155,139 @@ class RestUIForm extends ConfigFormBase {
+          '#attributes' => array('style' => 'padding-left:20px'),

#17 was almost working. First, let's revert automatic IDE changes. Those changes are very distracting.

Also rebased, there was a conflict due to recent changes.

This is all that was missing. With this, everything works!

1. +++ b/src/Form/RestUIForm.php
   @@ -214,31 +346,60 @@ class RestUIForm extends ConfigFormBase {
   -      $config = $this->resourceConfigStorage->create([
   -        'id' => $resource_id,
   -        'granularity' => RestResourceConfigInterface::METHOD_GRANULARITY,
   -        'configuration' => [],
   -      ]);
   +      $config = $this->resourceConfigStorage->create(
   +        [
   +          'id' => $resource_id,
   +          'granularity' => [],
   +          'configuration' => [],
   +        ]
   +      );
        }
   

   This is another automatic IDE-made change.

2. +++ b/src/Form/RestUIForm.php
   @@ -214,31 +346,60 @@ class RestUIForm extends ConfigFormBase {
   +    if ($granularity == RestResourceConfigInterface::METHOD_GRANULARITY) {
   +      // Granularity is "Method".
   +      $methods = $form_state->getValue(array('wrapper', 'methods'));
   ...
   +    else {
   +      // Granularity is "Resource".
   

   ===

3. +++ b/src/Form/RestUIForm.php
   @@ -214,31 +346,60 @@ class RestUIForm extends ConfigFormBase {
   +      // Granularity is "Method".
   +      $methods = $form_state->getValue(array('wrapper', 'methods'));
   ...
   +    else {
   +      // Granularity is "Resource".
   

   This can be made much clearer by having separate helper methods, and then calling those from here.

4. +++ b/src/Form/RestUIForm.php
   @@ -214,31 +346,60 @@ class RestUIForm extends ConfigFormBase {
   +      $configuration['methods'] = array_keys(
   +        array_filter(
   +          $form_state->getValue(array('wrapper', 'settings', 'methods'))
   +        )
   +      );
   +      $configuration['formats'] = array_keys(
   +        array_filter(
   +          $form_state->getValue(array('wrapper', 'settings', 'formats'))
   +        )
   +      );
   +      $configuration['authentication'] = array_keys(
   +        array_filter(
   +          $form_state->getValue(array('wrapper', 'settings', 'authentication'))
   +        )
   +      );
   

   Strange indentation here.

5. +++ b/src/Form/RestUIForm.php
   @@ -247,4 +408,17 @@ class RestUIForm extends ConfigFormBase {
   +  }
   +
   +
    }
   

   Two newlines, should be one.

Fixed all that.

Yeah, somehow phpStorm messed things up a lot, and I missed that when submitting the patch. Thanx for your help!

I did a clean install yesterday using D 8.3.x + Rest UI which worked out of the box. So how can I reproduce this?

Tests needed updating too. The updated test exposed a small issue with the form validation, I fixed this in the supplied patch.

Also, by updating the existing configuration an issue occurred when switching granularity, because the structure of the old granularity was kept. I suggest we only use the existing configuration to pre-fill the form values, and re-generated the configuration after submit based on the form values. Supplied patch also covers this.

@clemens.tolboom: The module works, but after saving you end up with a resource config with "Method" granularity. There is no way to create a resource config with "Resource" granularity, even though this is the default in Core.

So the issue summary is out of date I guess right?

Patch looks ok to me. I'd like to change the text on ie http://drupal.d8/admin/config/services/rest/resource/entity%3Anode/edit

Here you can restrict which HTTP methods should this resource support. And within each method, the available serialization formats and authentication providers.

That text should now contain something about the Granularity I guess. As this is now marked as critical it is not per se needed now. Is it worth a new issue?

Yes, I think it would be better to create a separate issue for updating the description. Would be nice to finally have this in.

#30: awesome work, thank you!

I'm going to continue my patch reviews now.

+++ b/restui.module
@@ -38,12 +41,14 @@ function restui_theme() {
 function restui_preprocess_restui_resource_info(&$variables) {
   $formats = \Drupal::getContainer()->getParameter('serializer.formats');
   $authentication_providers = array_keys(\Drupal::service('authentication_collector')->getSortedProviders());
-  foreach ($variables['resource'] as $method => $properties) {
-    if (empty($properties['supported_formats'])) {
-      $variables['resource'][$method]['supported_formats'] = $formats;
-    }
-    if (empty($properties['supported_auth'])) {
-      $variables['resource'][$method]['supported_auth'] = $authentication_providers;
+  if ($variables['granularity'] === 'method') {
+    foreach ($variables['configuration'] as $method => $properties) {
+      if (empty($properties['supported_formats'])) {
+        $variables['configuration'][$method]['supported_formats'] = $formats;
+      }
+      if (empty($properties['supported_auth'])) {
+        $variables['configuration'][$method]['supported_auth'] = $authentication_providers;
+      }
     }
   }
 }

Actually, rather than updating this code, we can simply delete it.

Because it's completely wrong, and leads to wrong information being displayed in the UI.

The existing code is saying "if no supported formats are specified, all formats are supported", and the same for authentication mechanisms. But this is not true, and has been wrong since #2065193: supported_formats and supported_auth should work in the same way. That was committed on December 1, 2013. And therefore REST UI has been extremely misleading since then.

Fixing that. (Note that we could move this change to a separate issue, if a maintainer asks that, I'd be happy to oblige. Because this is kind of scope creep. But this should be fixed, otherwise we're just making the problem bigger.)

(Continuing my review+refactor spree.)

1. +++ b/src/Form/RestUIForm.php
   @@ -138,29 +138,129 @@ class RestUIForm extends ConfigFormBase {
   -    $authentication_providers = array_keys($this->authenticationCollector->getSortedProviders());
   -    $authentication_providers = array_combine($authentication_providers, $authentication_providers);
   ...
   +    $authentication_providers = array_keys(
   +      $this->authenticationCollector->getSortedProviders()
   +    );
   +    $authentication_providers = array_combine(
   +      $authentication_providers, $authentication_providers
   +    );
   

   Unnecessary change here.

   Probably also made by IDE.

2. +++ b/src/Form/RestUIForm.php
   @@ -138,29 +138,129 @@ class RestUIForm extends ConfigFormBase {
   +    // Granularity selection.
   

   This comment doesn't belong to this code block, it belongs to the next code block.

   Let's move this first code block to a helper method, that will make the code more legible too.

3. +++ b/src/Form/RestUIForm.php
   @@ -138,29 +138,129 @@ class RestUIForm extends ConfigFormBase {
   +    if ($form_state->getValue('granularity')) {
   

   hasValue()

4. +++ b/src/Form/RestUIForm.php
   @@ -138,29 +138,129 @@ class RestUIForm extends ConfigFormBase {
   +    if ($granularity == RestResourceConfigInterface::METHOD_GRANULARITY) {
   

   ===

5. +++ b/src/Form/RestUIForm.php
   @@ -138,29 +138,129 @@ class RestUIForm extends ConfigFormBase {
   +              ':input[name="wrapper[methods][' . $method . '][' . $method
   +              . ']"]' => array('checked' => TRUE),
   ...
   +              ':input[name="wrapper[methods][' . $method . '][' . $method
   +              . ']"]' => array('checked' => FALSE),
   

   This wrapping is insane.

6. +++ b/src/Form/RestUIForm.php
   @@ -138,29 +138,129 @@ class RestUIForm extends ConfigFormBase {
   +              ':input[name="wrapper[methods][' . $method . '][' . $method
   +              . ']"]' => array('checked' => TRUE),
   ...
   +              ':input[name="wrapper[methods][' . $method . '][' . $method
   +              . ']"]' => array('checked' => FALSE),
   

   Also, we repeat the same expression 4 times!

I'll continue in the morning!

However, if the module maintainers would like to land this already, I'm fine with that! We can continue refactoring later on.

+++ b/src/Form/RestUIForm.php
@@ -189,24 +283,55 @@ class RestUIForm extends ConfigFormBase {
   public function validateForm(array &$form, FormStateInterface $form_state) {

Time to clean this one up.

Again, reverting unwanted wrapping introduced by somebody's IDE.

And again introducing separate helper methods for both types of granularity, to make the code more legible & maintainable.

The result is that the original validateForm() method's body remains 99% identical, it's just been moved to a separate helper method :)

Makes the patch much easier to understand!

1. +++ b/src/Form/RestUIForm.php
   @@ -109,6 +109,20 @@ class RestUIForm extends ConfigFormBase {
      /**
   +   * @param \Drupal\Core\Form\FormStateInterface $form_state
   +   * @param $id
   +   * @return array|mixed|null
   +   */
   +  protected function getGranularity(FormStateInterface $form_state, $id) {
   

   I added this, but did not fix the docblock.

   Made me realize the parameters make a bit more sense in the other order, so fixed that too.

2. +++ b/src/Form/RestUIForm.php
   @@ -138,29 +152,108 @@ class RestUIForm extends ConfigFormBase {
   +        'callback' => array($this, 'processAjaxForm'),
   

   Can be simplified to just ::processAjaxForm.

3. +++ b/src/Form/RestUIForm.php
   @@ -211,40 +323,104 @@ class RestUIForm extends ConfigFormBase {
   -        'granularity' => RestResourceConfigInterface::METHOD_GRANULARITY,
   +        'granularity' => [],
   

   This… does not make sense. We should set it to a valid value right away, or otherwise not set it at all. It will be overwritten at a next step anyway.

4. +++ b/src/Form/RestUIForm.php
   @@ -211,40 +323,104 @@ class RestUIForm extends ConfigFormBase {
   +  /**
   +   * Get the configuration for a REST resource with Method granularity.
   +   *
   +   * @param \Drupal\Core\Form\FormStateInterface $form_state
   +   *
   +   * @return array
   +   */
   ...
   +  /**
   +   * Get the configuration for a REST resource with Resource granularity.
   +   *
   +   * @param \Drupal\Core\Form\FormStateInterface $form_state
   +   * @return array
   +   */
   

   These docs aren't entirely accurate.

5. +++ b/src/Form/RestUIForm.php
   @@ -211,40 +323,104 @@ class RestUIForm extends ConfigFormBase {
   +    $configuration = [];
   ...
   +    $configuration['methods'] = array_keys(array_filter($settings['methods']));
   +    $configuration['formats'] = array_keys(array_filter($settings['formats']));
   +    $configuration['authentication'] = array_keys(array_filter($settings['authentication']));
   

   The 'method' granularity function explicitly creates a $configuration array to update, the 'resource' one does not, it appends to a non-existing array created out of thin air. Let's make this consistent.

6. +++ b/templates/restui-resource-info.html.twig
   @@ -4,13 +4,22 @@
   + * - granularity: string REST resource granularity.
   

   Should link to the interface constants.

+++ b/src/Form/RestUIForm.php
@@ -138,29 +158,108 @@ class RestUIForm extends ConfigFormBase {
     $format_options = array_combine($this->formats, $this->formats);
-    foreach ($methods as $method) {
-      $group = array();
-      $group[$method] = array(
-        '#title' => $method,
-        '#type' => 'checkbox',
-        '#default_value' => isset($config[$method]),
-      );
-      $group['settings'] = array(
-        '#type' => 'container',
-        '#attributes' => array('style' => 'padding-left:20px'),
+
+    $granularity = $this->getGranularity($id, $form_state);
+
+    // Granularity selection.
+    $form['granularity'] = array(
+      '#title' => t('Granularity'),
+      '#type' => 'select',
+      '#options' => array(
+        RestResourceConfigInterface::RESOURCE_GRANULARITY => $this->t('Resource'),
+        RestResourceConfigInterface::METHOD_GRANULARITY => $this->t('Method'),
+      ),

The changes in the ::buildForm() method are unwieldy: they're massive. Let's again apply the same strategy: two helper methods.

That means the vast majority original code remains in the same place, just in a different method (a helper method) instead of in the "main" buildForm() method.

Then we again call either one of the helpers.

In this case it's definitely easier to review this by comparing how RestUIForm is modified in #39's patch versus how it's modified in this patch. Reading the interdiff is not going to make it very clear what we gain by doing this.

And some last nits:

1. +++ b/src/Form/RestUIForm.php
   @@ -133,16 +155,70 @@ class RestUIForm extends ConfigFormBase {
   +    if ($granularity === RestResourceConfigInterface::METHOD_GRANULARITY) {
   
   @@ -189,9 +337,26 @@ class RestUIForm extends ConfigFormBase {
   +    if ($form_state->getValue('granularity') === RestResourceConfigInterface::RESOURCE_GRANULARITY) {
   
   @@ -211,40 +376,107 @@ class RestUIForm extends ConfigFormBase {
   +    $configuration = ($granularity === RestResourceConfigInterface::METHOD_GRANULARITY)
   

   If method, if resource, if method.

   Let's put "resource" first always, because that's the default since 8.3.

2. +++ b/src/Form/RestUIForm.php
   @@ -211,40 +376,107 @@ class RestUIForm extends ConfigFormBase {
   +    foreach ($methods as $method => $values) {
   ...
   -          'supported_formats' => array_keys(array_filter($settings['settings']['formats'])),
   ...
   +          'supported_formats' => array_keys(array_filter($values['settings']['formats'])),
   

   We can make the patch a bit smaller by renaming $values to $settings.

I think this is ready now. This patch makes the REST UI module do the right thing. And with my refactoring, its scope of changes is now minimized.

Lots of improvements are still possible to RestUIForm, and we should do those, but we should do them in separate issues.

This patch impacts the REST Experience because virtually all D8 sites that use REST, configure it through this module. And this module, to this day, is encouraging the wrong type of configuration: the overly specific one, which is unnecessarily complex, and unnecessarily brittle. Which is why we introduced "resource" granularity in the first place. The sad reality is that likely most D8 sites created since 8.2.0 did not take advantage of that simply because REST UI did not support it.
With this patch, REST UI finally encourages the simpler, more reliable default type of configuration.

An important next step after this is converting every 'method' granularity REST resource config entity that can be converted to 'resource' granularity. For that, I created #2869443: REST UI has been creating 'method' granularity configuration: provide update path to automatically convert to 'resource' granularity where possible.

Found one last issue. The form that is build defaulted to 'method' granularity, while the granularity select box defaults to 'resource'. This was giving weird behaviour when submitting new resources without switching the granularity.

Also updated the test to validate that the 'resource' granularity form is build by default.

Great find!

+++ b/src/Form/RestUIForm.php
@@ -188,9 +188,9 @@
-    $form['wrapper'] += ($granularity === RestResourceConfigInterface::RESOURCE_GRANULARITY)
-      ? $this->buildConfigurationFormForResourceGranularity($plugin, $authentication_providers, $format_options, $config)
-      : $this->buildConfigurationFormForMethodGranularity($plugin, $authentication_providers, $format_options, $config);
+    $form['wrapper'] += ($granularity === RestResourceConfigInterface::METHOD_GRANULARITY)
+      ? $this->buildConfigurationFormForMethodGranularity($plugin, $authentication_providers, $format_options, $config)
+      : $this->buildConfigurationFormForResourceGranularity($plugin, $authentication_providers, $format_options, $config);

If this is the solution for #44, then it points to a deeper problem: that the default granularity is not computed correctly. This code should not be the fix. Let's fix the root cause.

Thanks for expanding the test coverage!

That's actually how I fixed it initially :) I agree that getGranularity() should return 'resource' by default if nothing has been set yet. But shouldn't the form logic work the same way? Return the 'resource' granularity form unless 'method' granularity has explicitly been requested?

No, the form logic should be as simple as possible. Which means that $granularity should be either 'method' or 'resource'. Not 'method' or 'resource' or NULL. That's what #45 fixes.

I didn't mean we should handle NULL in the form logic, just suggesting to make the 'resource' form returned by default. But if the granularity is always computed correctly that's not really an issue. As far as I'm concerned, this is RTBC!

Exactly!

And YAY :)

Looking at the patch (AWESOME WORK BOTH OF YOU!), while I do some reading, can you help me to make this UI for informative so an administrator can understand what does imply to choose between Resource and Method?

@juampynr I agree with your findings. Let's create a new issue for that. I'll commit this one.

Patch does not apply due to #2851126: The UI says "disable", but it's really "delete"

@juampynr added the explanation issue here #2872299: Move granularity away into an 'Advanced' tab

Thanks for the reroll! Commented on the follow-up :)

Thanks!

Thanks for moving this forward!

Yeah! :D

YAY!

Now let's do #2869443: REST UI has been creating 'method' granularity configuration: provide update path to automatically convert to 'resource' granularity where possible and then tag a release!? :)