As of #2308745: Remove rest.settings.yml, use rest_resource config entities, it's actually possible to really disable REST resources: you can call RestResourceConfig->disable() on it now. We still need to fix a bug in core to actually make that work as expected (#2844046: REST Resource config entities do not respect the status (enabled/disabled)), but then it will work.

  1. Disabling = keep the configuration, but it's not active; it accepts no requests.
  2. Deleting = delete the configuration forever.
CommentFileSizeAuthor
#4 2851126-4.patch1.72 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: The UI says "disable", but it's really "delete" » [PP-2] The UI says "disable", but it's really "delete"

This is blocked on #2844046: REST Resource config entities do not respect the status (enabled/disabled), which is blocked on #2815845: Importing (deploying) REST resource config entities should automatically do the necessary route rebuilding.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: [PP-2] The UI says "disable", but it's really "delete" » [PP-1] The UI says "disable", but it's really "delete"

#2815845: Importing (deploying) REST resource config entities should automatically do the necessary route rebuilding landed, so this is blocked less now :)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: [PP-1] The UI says "disable", but it's really "delete" » The UI says "disable", but it's really "delete"
Status: Active » Needs review
FileSize
2851126-4.patch1.72 KB

#2844046: REST Resource config entities do not respect the status (enabled/disabled) also landed, so this is unblocked too.

I wanted to change Disable to Delete. But then that is much scarier. So… what if instead, we make Disable actually disable, and make Enable create it if it doesn't exist, and enable if it does already exist? Actually, that's already the logic that \Drupal\restui\Form\RestUIForm::submitForm() contains, so there's nothing new here. We're just making the UI actually do what it was claiming to do.

This patch tries to minimize the number of changes.

rogierbom’s picture

rogierbom CreditAttribution: rogierbom at Synetic commented

I agree. Deleting the configuration has more impact than just disabling it, and should IMHO be confirmed by the user first. It would be better to create a separate issue the add the delete action, with everything that comes with it.

Tiny nitpick with the patch: why not use fluent interface when enabling the resource, like it is done when disabling?

diff --git a/src/Form/RestUIForm.php b/src/Form/RestUIForm.php
index 34faeac..a83b974 100644
--- a/src/Form/RestUIForm.php
+++ b/src/Form/RestUIForm.php
@@ -240,6 +240,7 @@ class RestUIForm extends ConfigFormBase {
       }
     }
     $config->set('configuration', $configuration);
+    $config->enable();
     $config->save();
 
     drupal_set_message($this->t('The resource has been updated.'));
rogierbom’s picture

rogierbom CreditAttribution: rogierbom at Synetic commented
Status: Needs review » Needs work
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review

Tiny nitpick with the patch: why not use fluent interface when enabling the resource, like it is done when disabling?

Because the existing code doesn't do that either. The style in this patch is consistent with the existing code.

I totally agree we should fix that, but that entire file needs a serious refactoring wrt code style.

rogierbom’s picture

rogierbom CreditAttribution: rogierbom at Synetic commented
Status: Needs review » Reviewed & tested by the community

Agreed, let's keep the refactoring separated. In that case the patch is OK!

  • Wim Leers authored 733c58e on 8.x-1.x 
    Issue #2851126 by Wim Leers: The UI says "disable", but it's really "...
clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom as a volunteer commented
Status: Reviewed & tested by the community » Fixed
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Yay :)

Status: Fixed » Closed (fixed)

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