Problem/Motivation

Views currently has basic functionality to enabled and disable views. This currently works by setting the 'disabled' key on ViewStorage and calling save() on the view. It would be better if this could work more like exportables currently does in D7, with the status to be set/overridden outside of the view.

This feature could benefit any type of configuration entities.

Proposed resolution

Add a new key to the entity info for configuration entities to store the config name for a new file. This file can store an array of key => status for each configuration entity.

Remaining tasks

Initial patch only, needs review and work.

User interface changes

None

API changes

Enable, disable, and isEnabled methods to interface and ConfigEntityBase and ConfigStorageController. As above, definitely needs work and review.

Original report by damiankloip

CommentFileSizeAuthor
#155 1826602-154.patch2.38 KBdamiankloip
#155 interdiff.txt704 bytesdamiankloip
#146 vdc-1826602-146.patch2.38 KBtim.plunkett
#146 interdiff.txt1.77 KBtim.plunkett
#144 1826602-144.patch2.16 KBdamiankloip
#142 1826602-142.patch1.39 KBdamiankloip
#136 1826602-136.patch84.33 KBdamiankloip
#136 interdiff.txt2.55 KBdamiankloip
#130 1826602-130.patch83.19 KBdamiankloip
#128 1826602-128.patch84 KBdamiankloip
#128 interdiff.txt2.79 KBdamiankloip
#124 1826602-124.patch86.12 KBdamiankloip
#124 interdiff-1826602.txt776 bytesdamiankloip
#122 1826602-122.patch85.7 KBdamiankloip
#122 interdiff.txt623 bytesdamiankloip
#120 1826602-120.patch85.39 KBdamiankloip
#120 interdiff.txt6.02 KBdamiankloip
#118 1826602-118.patch84.93 KBdamiankloip
#118 interdiff.txt828 bytesdamiankloip
#116 1826602-116.patch85.12 KBdamiankloip
#114 1826602-114.patch85.67 KBdamiankloip
#114 interdiff.txt2.02 KBdamiankloip
#112 1826602-112.patch85.65 KBdamiankloip
#112 interdiff.txt6.89 KBdamiankloip
#110 1826602-110.patch78.94 KBdamiankloip
#110 interdiff.txt749 bytesdamiankloip
#108 1826602-107.patch79.56 KBdamiankloip
#102 1826602-102.patch74.45 KBdamiankloip
#100 1826602-100.patch74.92 KBdamiankloip
#100 interdiff.txt6.41 KBdamiankloip
#98 views-1826602-98.patch70.66 KBxjm
#98 interdiff.txt1.46 KBxjm
#96 merge-conflicts.txt8.56 KBxjm
#95 1826602-95.patch69.2 KBxjm
#90 1826602-86.patch2.58 KBEclipseGc
#63 1826602-63.patch66.21 KBdamiankloip
#63 interdiff.txt1.98 KBdamiankloip
#61 1826602-61.patch65.37 KBdamiankloip
#59 interdiff.txt17.98 KBsun
#56 1826602-55.patch65.17 KBdamiankloip
#56 interdiff.txt18.06 KBdamiankloip
#55 drupal8.config-entity-status.55.patch64.88 KBsun
#52 1826602-52.patch56.44 KBdamiankloip
#49 1826602-49.patch57.14 KBdamiankloip
#46 1826602-46.patch56.21 KBdamiankloip
#44 drupal-1826602-44.patch56.2 KBdawehner
#44 interdiff.txt4.97 KBdawehner
#42 1826602-42.patch52.84 KBdamiankloip
#40 1826602-40.patch16.1 KBdamiankloip
#27 1826602-27.patch14.48 KBdamiankloip
#22 1826602-22.patch13.01 KBdamiankloip
#18 1826602-18.patch11.59 KBdamiankloip
#15 1826602-15.patch11.68 KBdamiankloip
#9 1826602-9.patch11.32 KBdamiankloip
#6 1826602-6.patch12.46 KBdamiankloip
#5 1826602-5.patch11.76 KBdamiankloip
#1 1826602.patch6.36 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
6.36 KB

Here is a patch to get the ball rolling.

Also tagging with Needs tests as we will need more, or move the views tests too.

damiankloip’s picture

Issue tags: +Configuration system

Status: Needs review » Needs work

The last submitted patch, 1826602.patch, failed testing.

gdd’s picture

If we end up adding manifest files (see #1697256: Create a UI for importing new configuration) then it seems that would be a sensical place to also store enabled/disabled?

damiankloip’s picture

Issue tags: -Needs tests
FileSize
11.76 KB

This patch adds tests to the testCRUDUI method, changes the actual names, and implements the callbacks and methods in config_test module to have a status.

@heyrocker, This definitely sounds like a plan. I am just posting this patch anyway, because this is what I have. When I work out what's going on with this manifest, and/or it gets committed, It should be relatively straight forward to swap out the status storage location I am using at the moment.

damiankloip’s picture

FileSize
12.46 KB

Added some normal CRUD tests to testCRUD.

damiankloip’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1826602-6.patch, failed testing.

damiankloip’s picture

damiankloip’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1826602-9.patch, failed testing.

damiankloip’s picture

Assigned: Unassigned » damiankloip
Status: Needs work » Postponed

postponed on #1697256: Create a UI for importing new configuration, So we can use the manifest file for a config entity type to store the statuses.

xjm’s picture

Priority: Normal » Major
Issue tags: +feature freeze

Not having this is a regression from D7 Views, so bumping to major.

tim.plunkett’s picture

Title: Allow configuration entities to be enabled/disabled » Allow all configuration entities to be enabled/disabled
Priority: Major » Normal
Issue tags: -feature freeze

This is already in Views, we're just working to abstract it for all ConfigEntity.

damiankloip’s picture

FileSize
11.68 KB

Here is the latest version, based on @heyrockers patch in #1697256: Create a UI for importing new configuration.

Reviews welcome.

damiankloip’s picture

Status: Postponed » Needs review

Let's give this a try.

Status: Needs review » Needs work

The last submitted patch, 1826602-15.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
11.59 KB

Re rolled.

yched’s picture

ConfigStorageController::setStatus()/getStatus() look like they could be abstracted to act on any "property" besides 'enabled' ?

aspilicious’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.phpundefined
@@ -26,6 +26,13 @@
+   * Returns whether the configuration entity's status is disabled or not.
+   *
+   * @var bool
+   */
+  public $disabled;

There is alrdy an issue to change this to $enabled. I fully support that.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.phpundefined
@@ -53,6 +62,28 @@ public function setOriginalID($id) {
+  /*
+   * Implements Drupal\Core\Config\Entity\ConfigEntityInterface::enable().

/**

+++ b/core/modules/config/tests/config_test/config_test.moduleundefined
@@ -197,4 +210,26 @@ function config_test_cache_flush() {
   return array();
-}
\ No newline at end of file
+}

Damnit how did this get in in previous patches. It will be gone now ;)

Status: Needs review » Needs work

The last submitted patch, 1826602-18.patch, failed testing.

damiankloip’s picture

FileSize
13.01 KB

Fixed those changes, and the suggestion in #19 seems like a good one too.

I haven't had time to fix the fails yet.

aspilicious’s picture

patch is identical

damiankloip’s picture

No. It adds a key parameter to the getStatus and setStatus methods.

andypost’s picture

It looks strange to have imageStyle disabled or a whole menu... Suppose better to provide a annotation disableable for config entities

damiankloip’s picture

To me this makes sense most of the time, I think it would be a benefit to be able to enable and disable things like image styles.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
14.48 KB

Fixed (I think) the tests, but didn't have a connection at the time so couldn't check the previous fails.

I also changed the set/getStatus methods to set/getManifestProperty, then we can use this generically for things stored in the manifest file.

Status: Needs review » Needs work

The last submitted patch, 1826602-27.patch, failed testing.

damiankloip’s picture

Ha, seems it's just our views tests that are failing :) I'll sort this out.

attiks’s picture

Trying to understand this, the enable/disable is stored inside a manifest yml file, so this would mean that if I want to deploy something I need to copy an extra file now?

damiankloip’s picture

Yes, But I think you would need this extra file now anyway, regardless of enabled/disabled status.

gdd’s picture

Yes, please see the discussion this came from at #1697256-44: Create a UI for importing new configuration

attiks’s picture

#31 for some reason I missed the incarnation of the manifest system.

damiankloip’s picture

@attiks, yes, it was tucked away in that config UI patch :)

sun’s picture

There is absolutely no reason to put this into manifest files, or tie it to them in any way. It would be a clear abuse of manifest files.

Configuration objects are cached. And there are usually not many of a certain type. If you need to iterate over all, just do it. If this turns out to become slow, we add specialized caching of lists and/or helper methods for it.

Furthermore, a disabled config object does exist. The system cannot simply hide them away as if they would not. Doing so would have an immense amount of ugly repercussions down the line.

The proper solution here is:

  1. Add an 'enabled' or 'status' key to config files.

    (Ideally following what content entities are doing; the common publishing status property of content entities actually is not too different from the desired effect here.)

  2. Add a new ConfigEntityToggleableInterface (better name welcome).

    Config entities that implement this interface have a status and can be enabled and disabled.

  3. Enhance ConfigStorageController with the proposed enable/disable/isEnabled methods, but check instanceof ConfigEntityToggleableInterface before operating on one.

As a slight variation of the last two points, we could alternatively consider to add a new 'status' => 'status' key to the entity_keys array in entity info, which is the pattern for enabling additional entity semantics/behavior already (such as UUIDs, revision support, etc).

tim.plunkett’s picture

Views already currently has protected $status, see ViewStorageInterface

However, the act of enabling and disabling is made much faster by this patch, since the whole View yml file doesn't have to be written, just the small manifest file.

sun’s picture

To further explain why we cannot simply hide away disabled configuration:
#949220: Text format names can never be reused (Possible solution: Allow disabled text formats to be re-enabled)
#562694: Text formats should throw an error and refuse to process if a plugin goes missing

In other words:

Disabled configuration/entities still exist. The system and API has to know about them. They are an active part of the system, at all times. The decision to "disable" them (instead of deleting) has UI consequences only. They still exist. They just don't necessarily participate at all times with regard to dependent operations.

E.g., a disabled node type must still exist. It's a bundle, and there can be fields attached to the bundle. The node type has to participate in all API operations, regardless of whether it is enabled or disabled. The only thing that really changes is that it no longer appears on /node/add and potentially elsewhere, but all of these decisions are purely cosmetic.

This means that listing/loading all config entities of a certain type will always return all, regardless of whether they are enabled or disabled.

If you want to remove configuration, delete it.

damiankloip’s picture

@sun, I'm not disagreeing that parts of what you have said are no doubt correct, and could be the right path to take. However, I also think you may have partially mis-interpreted what we are doing here.

We are not trying to hide or stop configuration entities from being loaded with the manifest file, more like an overridden status (think CTools exportables). So they do not control what gets loaded, it is just a place to store the status. So you are right, if we didn't want a particular entity, it should definitely be deleted. This is mostly a UI facing thing, but also has an impact on things like access to views too.

As Tim pointed out, we already have it working as you've suggested in views, by being stored on the actual entity.

Not sure if that changes your opinion at all? :)

I will definitely work on implementing the status key in the entity info and work on this some more, this does make alot of sense.

sun’s picture

We should limit the scope of this issue to a pure enabled/disabled status as an opt-in for config entity types.

Anything related to "overridden" is an entirely different can of worms, for which we have separate issues already:
#1398040: Detect if default configuration of a module has been changed, and allow to restore to the original
#1620140: Allow synchronizing config entities from default config when modules are updated

Also, depending on how fast we move forward here, #1779026: Convert Text Formats to Configuration System might land upfront, and like Views, Filter module's text formats can be disabled in HEAD already — although that behaves slightly differently than Views and also, disabling a text format has a non-trivial range of consequences across the entire system. In general though, there's a lot that can be learned from the status implications of text formats.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
16.1 KB

Ok, how about something like this instead?

  • Added a "status" key to entity keys. This is used in the controller to determine whether to enable/disable and whether something isEnabled or not.
  • Added a statusKey property and getter, mainly to use to easily check. This is used internally on the controller and in the config list controller
  • I have not converted views in this patch (depending on the reception of the approach), so we will see some fails.
  • I also have an alternate patch, that adds a ConfigStatusEntityBase and interface class but I think it might be better just having the logic on the regular controller?

Status: Needs review » Needs work

The last submitted patch, 1826602-40.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
52.84 KB

See how this goes...

Status: Needs review » Needs work

The last submitted patch, 1826602-42.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.97 KB
56.2 KB

This patch fixes hopefully all failing tests.

    // Force the rebuild of the menu.
    menu_router_rebuild();

For whatever reason just using

    state()->set('menu_rebuild_needed', TRUE);

doesn't cause the page to appear, maybe someone has a clue why.

damiankloip’s picture

Yeah, this is the fix I had locally earlier. I couldn't find another way to fix the tests... It's weird because essentially this patch is just moving what is done on ViewStorage/controller to the ConfigStorageController.

damiankloip’s picture

FileSize
56.21 KB

Just changed the enable and disable methods on the controller to use save() directly and not $entity->save().

sun’s picture

This looks pretty good. Some questions:

1) I wonder whether we shouldn't add (enforce) a status property/key for all config entities, just like we have a langcode for all. Long-term, I think that would be good for consistency. Of course, the status handling would have to be implemented first, so short-term, the easiest way would be to force the statusKey to be 'status' and force the value to be 1 (enabled). And in the list controller, we'd have to differentiate between config entities that "actually" support it and those that do not (only temporarily until status handling has been implemented everywhere).

2) Regardless of 1), the status for config entities that support a status should default to 1/enabled. That way, default configuration supplied by modules would only have to specify the status if the entity should not be enabled by default.

3) I don't necessarily see why the status property should be bound to the storage controller, as we'd still expect a status to be there, even if the storage controller is swapped out — but alas, that's how we're doing things in the entity system currently (and I don't understand it), so this aspect of the proposed implementation, while strange, seems to be correct.

4) Regardless of 3), I'm not sure whether it is a good idea to directly save an entity upon calling ->enable() or ->disable(). For the storage controller methods, that might make sense — but for the entity methods, I wouldn't have expected those methods to directly perform a write operation. I'd only expect the entity methods ->save() and ->delete() to actually perform storage operations/manipulations, so this would be more natural from my perspective:

$foo = entity_create('foo', array());
$foo->disable();
$foo->save();

(...which, alas, questions whether those should be methods on the entity in the first place, but I don't see why not.)

sun’s picture

Issue tags: +Needs tests

Oh, and we need dedicated tests for this functionality; i.e., not mixed into completely different tests. I'd even recommend to create a new ConfigEntityStatusTest class, since I predict that we'll have to verify some edge-cases over time. You should be able to use DrupalUnitTestBase for that.

damiankloip’s picture

FileSize
57.14 KB

Ok, this patch moves the tests into their own class as suggested. It's a better idea, and much more organised. I also changed the fact that the enable/disable operations were saving, so now it works as suggested in #47.4.

I haven't yet changed anything based on comments in #47.1/2; In this patch The view and the config test entity implement their own $status property that provides the default of '1'. I didn't implement this on the base class as an extending class might want to use a different status key, so this makes it easier.

So you think move this property onto the base class, and also enforce $statusKey = 'status' too (I'm not opposed)? Then will it be more difficult to distinguish between config entities that want to implement this, and ones that dont?

EDIT: I also have not changed any logic etc.. on the list controller yet, as this is based alot on the above :)

sun’s picture

Yes, long-term, I think we want to have an enable/disable functionality on all config entities. In fact, most Configurables we have support that notion in one way or the other today already, it is merely not standardized yet. @merlinofchaos suggested that direction very early already, and I agreed, and only deferred the topic to "when we're there" — now, we are. ;)

Off-hand, I can't really see what would break by making $status a public property on ConfigEntityBase and defaulting it to 1 in the constructor, unless a config entity type explicitly opted-out by setting $statusKey to FALSE. (i.e., the identical opt-out logic as existing for $uuid for config entities already; we should actually make sure to copy the entire logic of uuid as a template for status, because it should behave exactly the same with regard to the entire entity life-cycle) — right now, we only have config test, image styles, contact categories, and views in core, no? I don't see why any of those would break.

Aside from that:

The enable/disable/isEnabled methods on ConfigEntityBase should really just set/check the property value on the entity object, instead of calling into the storage controller; only save() and delete() should call into the storage. EDIT: Again, just copy what we're doing for uuid().

And minor: The new test class should be renamed to ConfigEntityStatusTest, since the context/subject is ConfigEntity.

This looks great, and I think we're very close.

Status: Needs review » Needs work

The last submitted patch, 1826602-49.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
56.44 KB

ok, here are some of those updates. Check to see if this is how you imagined it being implemented. So we assume the statusKey is always going to be 'status' when enabling/disabling on config entities.

Status: Needs review » Needs work

The last submitted patch, 1826602-52.patch, failed testing.

sun’s picture

Thanks! :)

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -26,6 +26,13 @@
+  public $status = '1';

The default value should be set in the ::create() method of the (config) storage controller, just like uuid is set there by default.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -88,6 +95,28 @@ public function set($property_name, $value, $langcode = NULL) {
+  public function enable() {
+    $this->status = '1';

We are type-casting everything to strings at the storage layer currently, but I think the default value as well as the value being set by enable()/disable() should be TRUE/FALSE Boolean value.

(Also note that we might remove the string-casting via #1653026: [META] Use properly typed values in module configuration)

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -88,6 +95,28 @@ public function set($property_name, $value, $langcode = NULL) {
+  public function isEnabled() {
+    $status_key = entity_get_controller($this->entityType)->statusKey();
+    return isset($status_key) ? $this->status : TRUE;
+  }

The current entity system practice is to all these methods identical to their properties (which is a bad practice), so/but at least for now, we should keep this in line and rename isEnabled() to just status().

Second, as you can see from the other entity methods that return entity key-specific values, the default implementation on the entity class always assumes and uses the default property name; i.e., we do not call into the storage controller here, but instead, simply return $this->status.

(I've been told that entity classes that diverge from the defaults are supposed to override the entity methods as needed; e.g., you also need to override the id() method if your entity key is different; see #1822176: Why do we have $entity_info['entity keys']['id'] *and* MyEntity::id() method overrides?)

Also, let's type-cast the returned value to bool, so functions that call into status() can at least reliably expect a Boolean value there.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
@@ -32,4 +32,21 @@ public function getOriginalID();
+  /*

Missing * :)

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
@@ -57,6 +57,13 @@ class ConfigStorageController implements EntityStorageControllerInterface {
+  protected $statusKey = 'status';

@@ -66,6 +73,13 @@ public function __construct($entityType) {
+    if (isset($this->entityInfo['entity_keys']['status'])) {
+      $this->statusKey = $this->entityInfo['entity_keys']['status'];
+    }
+    else {
+      $this->statusKey = FALSE;
+    }

OK, thinking through this once more, I guess that my suggestion to make the status key an opt-out like uuid wasn't really right/correct, since there is not really a way to opt out of entity keys (you could specify FALSE as value in the plugin annotation, but that would be not very consistent with the other entity keys).

For now, I guess that this code works. We might want to add a @todo, or simply create a follow-up issue to actually enforce some entity keys for config entities (at least the uuid property/key is strictly required, but not properly enforced currently). So yeah, that rather sounds like an independent follow-up issue. This code looks fine to me.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
@@ -237,6 +251,13 @@ public function create(array $values) {
+  public function statusKey() {
+    return $this->statusKey;
+  }

This method should be obsolete with the above ConfigEntity class changes.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.php
@@ -54,19 +54,27 @@ function testList() {
+      'disable' => array(
+        'title' => t('Disable'),
+        'ajax' => TRUE,
+        'token' => TRUE,

1) Note that config_test module does not use translatable strings currently. That's why the other expected operation labels are not wrapped in t() either.

2) I don't really get the purpose of the 'ajax' and 'token' properties — looks like those should be removed here and tackled in a separate follow-up issue?

+++ b/core/modules/config/tests/config_test/config/config_test.dynamic.default.yml
@@ -1,2 +1,3 @@
+status: 1

I wonder whether we should intentionally comment this out, and add a comment above it that the default status is expected to be enabled (and add a test for that)?

+++ b/core/modules/config/tests/config_test/config_test.module
@@ -116,6 +117,18 @@ function config_test_menu() {
+  $items['admin/structure/config_test/manage/%config_test/enable'] = array(
...
+  $items['admin/structure/config_test/manage/%config_test/disable'] = array(

One very interesting aspect is that the enable/disable operations can only be accessed from the list overview page, but not from within the entity edit form. I think Views gets around that by adding a dropbutton on the upper right of the edit form, which contains all of the entity operations once more. In general, this also reminds of #1834002: Configuration delete operations are all over the place, and we will have to design/invent a pattern for that.

However, just mentioning — that should be done in a separate follow-up issue.

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/Plugin/Core/Entity/ConfigTest.php
@@ -62,4 +63,11 @@ class ConfigTest extends ConfigEntityBase {
+  public $status = '1';

Specific entity classes do not need to repeat properties of the base class, so this ConfigTest entity property can be removed.

sun’s picture

Status: Needs work » Needs review
FileSize
64.88 KB

Helping you out a bit.

damiankloip’s picture

Issue tags: -Needs tests
FileSize
18.06 KB
65.17 KB

@sun, Thanks for the feedback!

I *think* I've addressed everything in the feedback. I did check through. They all make sense. I actually did have the statuses as bool values up until just before I posted the patch, then for some reason I thought it was a good idea to change them as they get cast to string anyway. Can't wait for that not to be the case :)

To address the status() method, I am just returning !empty($this->status) as this will return bool true for '1'/TRUE or bool false for '0'/FALSE and if it's not set.

damiankloip’s picture

Oops, cross post! Maybe we can see how similar the two are ? ;)

Status: Needs review » Needs work

The last submitted patch, 1826602-55.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
17.98 KB

Attached is a reverse-interdiff between #56 (old) and #55 (new)

sun’s picture

Heh. Surprisingly similar, even. ;)

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityListController.php
@@ -30,25 +30,24 @@
-    if (!$entity->status()) {
...
+    if ($this->getStorageController()->statusKey()) {
+      if (!$entity->status()) {

ah, removing that condition makes sense.

damiankloip’s picture

FileSize
65.37 KB

Ok, here both patches kind of put together. I also changed the ViewUI isEnabled() function (__call replacement one) with status() now as this should be pretty safe.

Status: Needs review » Needs work

The last submitted patch, 1826602-61.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.98 KB
66.21 KB

This should pass the tests now. Hopefully we are good to go!

sun’s picture

Component: views.module » configuration system
Status: Needs review » Reviewed & tested by the community
Issue tags: +Configurables

Excellent! :)

tim.plunkett’s picture

Extra +1 from me!

catch’s picture

I haven't got time to fully review the patch now, but this is something with Views that always made me uncomfortable so when I saw the title and the RTBC status I wanted to get in quick. Will try to explain the lack of comfort below, if it's not valid please say so:

Disabled makes sense if you have 'default' configuration in PHP that you can't delete, like stuff from hook_views_default_views(), however we no longer have that situation.

If you delete some default configuration, the configuration still exists in the folder of the module that provided it. Currently there's no public API or UI for accessing default configuration vs. live, but that feels like something that could be useful for other things (like override/reset).

If we had that, then could 'disabled' configuration be 'stuff that exists as a default somewhere, but isn't in the live configuration? i.e. have a UI for showing configuration objects that exists as default but aren't represented in the active storage?

And if that's the case, then for modules to provide configuration disabled by default we'd need to provide a way to say "don't actually import this configuration file, just leave it there', and people could find it if they want it.

On top of that, if there's configuration that's not default, why would you want to disable it? I can see something like wanting to work on a custom view without exposing it anywhere else maybe? Is that the idea here and if so how does that tie in with the potential for eventual revisions and drafts on config entities down the line?

sun’s picture

@catch:
I think I understood your concerns. And I think they are irrelevant for this issue. Here's why:

  1. Loading default config from ./config directories within individual extensions is and was only ever acceptable for the installer from my perspective, and for that limited scenario and use-case, it made a tonne of sense. At regular runtime, a filesystem scan across potentially hundreds of extensions would not remotely be acceptable from a performance perspective. The configuration system does not assume that the storage is shielded with a cache bin/backend, and I'd strongly object to any proposal that wants to introduce that assumption. Lastly, and most importantly, configuration is declarative, and loading "additional" configuration from arbitrary locations fundamentally breaks the concept of staging configuration. Your configuration is not declarative anymore. It is extended by "may or may nots" and tons of vague assumptions about what exists and what doesn't, and what has been configured and what was not. It is impossible to stage vague, random assumptions.

  2. As mentioned elsewhere already: Configuration is owned by the user. Configuration, even if it came as some default, can be deleted. Default configuration in ./config directories cannot be deleted.

    Default configuration in ./config directories cannot be disabled either. The maximum you'd end up with is the current, terrible architectural design of menu links in the menu/router system: As soon as you touch the default config in any way, it cannot be default configuration anymore. Whereas "touching" includes to innocently drag other configuration items within a tabledrag-enhanced table on an administration page, since the weight of all items needs to be adjusted. You didn't actually touch the default configuration, but yet, it has to be customized. #550254: Menu links are sometimes not properly re-parented tells an epic story about that. An architectural mistake I don't want to repeat at all costs. The essential architectural flaw is that the system assumes there would be some sort of a hybrid ownership of something, which leads to conflicts that are impossible to resolve. I will not accept this hybrid for configuration. And yes, I'm perfectly aware that we're still lacking bare-essential tools to cope with the situation, but my entire stance is that we will only be able to invent and provide the required tools if the ownership is clearly defined.

  3. As you already alluded to, there are use-cases for disabled configuration. Regardless of whether it was supplied as default or not.

  4. I actually consider this change as an essential milestone to bring config entities more in line with content entities: As I already mentioned earlier in this issue, the "enabled status" of config entities has logically (not necessarily technically) the exact same effect as the "published status" of content entities: The entity still exists, and the entity still participates on all fronts. The only difference is a UI implication: Much like unpublished content entities, disabled entities no longer appear in certain non-administrative listings.

    This is essentially why I made sure that we're using the identical property name as content entities that support a publishing status and that the entire concept we're introducing here is identical to content entities. (And thanks to @damiankloip for coping with me ;))

    This change won't prevent us from introducing revisions for config entities. Quite the contrary in my mind: The more we harmonize the two, the more likely are features like revisions (as well as typed properties/fields).

catch’s picture

@sun I think we're talking past each other a bit (and I still haven't reviewed the patch). Here's some use cases though:

1. I have a view, that provides a block and a menu item. I disable it and the block and menu item disappear. What happens to my Scotch display that has the block configured then?

2. I have an image style and disable it, what happens if you try to visit an image URI, or the image style is configured for a field formatter?

3. I have a node type and disable the node type, what happens to all the nodes?

Essentially this feels like the same problem with have with disabled modules, but multiplied many, many times over, unless we actively stop people from using it all over the place whenever there's any kind of dependency.

attiks’s picture

#68, @cacth: this is the same as it is now for Drupal 7

1. If a view is disabled it no longer will show

2. If an image style is disabled the picture will no longer output, or maybe will default to the original image

3. The node will still be there, but the output isn't going to work, you probably still see it in admin/content, but editing/viewing will not be possible

I'm also waiting for this functionality for breakpoint so we can allow people to disable/enable breakpoints coming from themes, see #1734642-133: Move breakpoint module into core, breakpoint originally had enabled/disabled en override/revert. The problem is that we need everything inside the config dir (sites/default/files/config) because the config is build the moment a module/theme is enabled. We can not fallback to the .yml file shipped with the module/theme for this functionality.

catch’s picture

Status: Reviewed & tested by the community » Needs review

No for #3 we actually don't let you disable a node type, at least via the UI, if you have content attached to it, that was fixed since Drupal 7. Node types disappearing at random was the cause of this bug:

#1017672: D6 to D7 update process permanently deletes comment bodies and other data, and throws fatal SQL errors

If we can't deal with the dependencies that content has on configuration, then we shouldn't allow the configuration to disappear arbitrarily.

Moving back to CNR, 'because you could do it in Drupal 7' isn't a good enough argument.

attiks’s picture

#70 I was just trying to answer your questions in #68, my arguments isn't that we should do it "because we could do it in Drupal 7", but the functionality is needed as nicely explained by @sun in #67 and my attempt for the breakpoint case in #69

catch’s picture

@attiks, there isn't actually a use case for why it's needed in #67, it just mentions that there are use-cases.

With breakpoints coming from themes, why can't users just delete them if they don't want them?

attiks’s picture

#72
Deleting a breakpoint will mean that we need to reparse the theme.breakpoint.yml file to 'enable' it again, it will also mean that if the breakpoint has been added to a custom group, it has to be re-added to that group.

sun’s picture

Typical use-cases from my POV:

  1. Build out [possibly even sets of] site configuration without immediately "publishing" it for user consumption.
  2. Disable some bundle/configuration, because it should (temporarily?) not be used (anymore).

What you could argue is that most use-cases can be translated into restricted access — but, if you think about it, then you'll realize that the status property of content entities works in the exact same way and has the exact same purpose; it is a flag whose value has access implementations, nothing else.

In light of that, the way Views handles the status flag indeed seems to be bogus/wrong; it needs to register all views in hook_menu() regardless of their status, and only check the status upon trying to access a view.

damiankloip’s picture

Assigned: damiankloip » Unassigned

I think this issue is digressing slightly from it's initial purpose of just providing a standardised way for config entities to use a status flag, and bring them in line with other entities.

The patch does not convert any config entities to use this, except views, which this is a conversion from (kind of) anyway. So each implementation of this will be in it's own issue. Where the actual handling/implications of the status can be considered for each case.

Sound good? :)

sun’s picture

Given the discussion and conclusions in #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API, I think that @catch is very right to be concerned about what the exact consequences of disabled configuration are. For me, that essentially boils down to what I mentioned in #74 and earlier:

1) The enabled/disabled status of a config entity must only have an impact on "user view access".

2) Disabled config entities must still exist and be fully available.

3) The status of a config entity must only ever have an impact on 'view' operations in the UI. It needs to be treated like the publishing status of nodes.

This inherently means that Views' treatment and implementation of disabled views violates those roles, since it uses the status flag during API operations already and e.g. skips over disabled views when registering menu router paths. Instead, it always needs to register everything, and essentially, only check for access on view.

Likewise, to address #70, the disabled status flag on a content type would only mean that it does not appear on /node/add for unprivileged users. Every other aspect of the content type would be retained 100% functional.

If there are architectural problems even with this very limited impact of the enabled/disabled status, and we cannot address them, then I think I tend to agree with @catch that we cannot implement the status and we want to remove it altogether instead.

damiankloip’s picture

Again, surely this is about the specific implementations of this, and not the actual patch here.

How the status gets implemented is down to each use case, not the fact that we have a status flag and an isEnabled method available. The points above are definitely valid, but they are things to bear in mind when modules actually check the status of their config entities. Modules are already implementing this in their own way, aren't we just trying to standardise this?

If we need to change how views deals with these, so be it, but I don't think this is the place to do that. That would be a relatively minimal change for views.

sun’s picture

I've the impression we're talking past each other :)

The conclusion that can be drawn from most of our existing "disabled status" throughout Drupal core, in particular with regard to the amount of critical/major bugs that the fundamental idea of having anything disabled has caused over the years, is that the idea itself caused more architectural and technical/functional problems than it actually resolves from a user perspective.

Disabled modules, disabled text formats, and almost everything else that can be disabled caused us a lot of problems, of which some are still not resolved today, and a few of them are almost impossible to resolve. Most of the problems are about stale/invalid references from something that is enabled to something that is disabled, as well as logical and security problems caused by disabled things that are involved in user workflows (e.g., something was created while something else was enabled, but then that other thing got disabled, which leaves the newly created thing in a undefined state - and also vice-versa, when something was created before something else got enabled, whereas the newly enabled thing could have a security impact on the newly created thing, potentially leaving it in a undefined state).

This is why we are extremely wary when introducing a new disabled notion. Thus, to clarify, the concerns are targeting the entire disabled config concept, not a particular implementation.

Therefore, it is critically important that we have a clear and concise understanding of the exact consequences of an enabled/disabled status for config, and also, what kind of effects/implementations are "allowed" (or "supported") and which are not. I think this is what @catch was referring to in his comments.

Technically, I think that the interpretation/guidelines for disabled config in #76 should not impose any of the typical "disabled problems", but I also want to admit that I'm not 100% sure.

damiankloip’s picture

Yep, this does all make sense. I can understand the concerns from catch and yourself :) I'm definitely not saying you guys are wrong.

So where do we go from here, how do we resolve the ultimate question?

damiankloip’s picture

And admittedly, the title is pretty scary too...

Fabianx’s picture

#76: So for context (for example) this would mean:

* Disabled contexts are available for re-enabling
* Disabled contexts are greyed out on the UI.
* Context chooses to only process "published" contexts (status==1)

Do I understand this right (applied to this use case?).

sun’s picture

That said, I'd also like to amend that the disabled status for a text format is actually required and the only way to make a text format disappear from the user interface. That is, because text formats can only be created, but they cannot be deleted - since we are not able to determine where a text format is used and thus whether a format can be deleted, and it is additionally not possible to automatically determine a replacement text format that has the same security effects as the deleted one.

Therefore, we do not allow to delete text formats, they can only be disabled, which essentially hides them from the node/add and node/edit forms — unless the format is assigned to the content already, in which case unprivileged users are not allowed to edit the content, and privileged/administrative users are asked to change it to a format that is enabled. However, the text format itself is fully functional and works as usual even if it is disabled.

And in essence, I guess that's the kind of usage that #76 would foresee - disabled config only has access implications, and those implications need to be handled very carefully. (The access handling in filter_process_format() is anything but trivial...)

For text formats, we will have to introduce this notion anyway. However, that doesn't necessarily mean that we need to support it for everything; it could also be a one-off implementation.

re: #81:
Unfortunately, I'm not familiar with contexts, so I can't comment on that. I guess it would be better to think through config entities that are in core, since everyone of us knows how they work. :)

dawehner’s picture

Probably similar to text formats, also Image styles would make sense to disable,
as you have the same issues to figure out where it is used.
Not sure about the other configuration entities, but we should better not provide a way to disable it, as many of them in custom/contrib world could have the issues mentioned by catch and sun. Maybe we could introduce "disable-ability" similar to the way uuid() works.

tim.plunkett’s picture

Component: configuration system » configuration entity system
damiankloip’s picture

Example case for this: #1868772: Convert filters to plugins is currently just checking $filter->status. I think it should be using this instead...

xjm’s picture

So @EclipseGc thought of a different kind of solution to this problem: prefixing the filenames with disabled. I initially rejected this idea because of all the reasons we've disliked magical names elsewhere--disallowing modules from having that name, #1186034: Length of configuration object name can be too small, #1831774: Config import assumes that 'config_prefix' contains one dot only, more magic generally, etc. etc. However, there are also advantages:

  • It's really simple. We don't have to add hardly any new complexity to the configuration system; it just works with listAll().
  • You don't have to load the objects to see if they're disabled.
  • It's REALLY easy to see what disabled objects you have.

See also: #1881096: Regression: Site broken when disabling module without removing block instances.

sun’s picture

The reasons stated in #86 are the exact consequences that must not happen for disabled config.

Disabled config has to exist. It has to be listed with listAll(). It has to be loaded with entity_load_multiple(). It has to be processed as usual. It has to be maintained.

The only acceptable impact of disabled config is an access condition for UI purposes.

Any additional hiding/munging on top of that will ultimately end up in the same, architecturally broken situation as disabled modules. We definitely do not want to go there, since we already know where that ends.

FWIW, the actual problem of #1881096: Regression: Site broken when disabling module without removing block instances. is much rather that the new config files of the new block system are not namespaced + owned + supplied by each module, and instead, collected by a single service. Therefore, the service has to manually check whether the configuration is actually valid, and it also has to maintain the configuration on behalf of the actual modules. Instead of plugin.core.block.ID config names, the block system should actually use $module.plugin.core.block.ID. I wanted to raise this concern in the block plugin conversion issue already, but didn't want to block that monster patch on it.

xjm’s picture

Instead of plugin.core.block.ID config names, the block system should actually use $module.plugin.core.block.ID. I wanted to raise this concern in the block plugin conversion issue already, but didn't want to block that monster patch on it.

There's already an issue for this: #1839904: Rename plugin.core.block CMI prefix to block.block

Well, not exactly what you've suggested I guess, but core also doesn't do what you suggest elsewhere. We use (e.g.) views.view.frontpage.yml, not node.views.view.frontpage.yml.

Disabled config has to exist. It has to be listed with listAll(). It has to be loaded with entity_load_multiple(). It has to be processed as usual. It has to be maintained.

This seems completely counterintuitive to me. If the module isn't there, it can't be processed as usual.

xjm’s picture

Oh, sorry, I just realized the error in my thinking. Disabled things belonging to enabled modules really aren't the same things as previously-enabled things belonging to disabled modules.

EclipseGc’s picture

FileSize
2.58 KB

OK, wondering about something like this? I had to rearrange module_disable slightly so that entity cache is still around and usable for the hook. Likely a good thing, but not sure how it'll affect tests.

Eclipse

sun’s picture

@EclipseGc: Unfortunately, that's really something very different than the goal and scope of this issue. We have a patch in #63 already, and all we're trying to decipher here is the exact architectural definition of "disabled config entities of an enabled module" and what that MAY constitute, though much more importantly, what it MUST NOT constitute.

I think the patch in #90 is more related to the discussion in #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API and related issues (of which we have too many already).

EclipseGc’s picture

@sun,

I agree. This was a misunderstanding on my part with regard to the desired outcome here.

EclipseGc’s picture

Although, if tests come back green on it, I'd still suggest the module_disable change I made.

Status: Needs review » Needs work

The last submitted patch, 1826602-86.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
69.2 KB

General apologies to @damiankloip, though I do think we've accomplished something here which is to clarify that this issue solves a very different problem than #1881096: Regression: Site broken when disabling module without removing block instances.. :P

Here's an attempt to reroll this to "un-derail" the thread and also stir the pot since this has been sitting awhile. The block changes will still need to be reimplemented.

xjm’s picture

FileSize
8.56 KB

The merge conflicts.

Status: Needs review » Needs work

The last submitted patch, 1826602-95.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
1.46 KB
70.66 KB

And fixing the Views blocks.

Status: Needs review » Needs work

The last submitted patch, views-1826602-98.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
6.41 KB
74.92 KB

Ok, Not sure how the getProperties snuck back onto the controller, but alot has changed since the last patch (in #63). This should fix the rest of the tests now; I also updated the config files that weren't converted to status.

Status: Needs review » Needs work

The last submitted patch, 1826602-100.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
74.45 KB

Oh, the load method was from way back too. Crazy.

xjm’s picture

It's entirely possible I screwed up the reroll or that the merge wasn't smart enough. :)

sun’s picture

Category: feature » task

Meanwhile, we have some real world data of running into the overall problem space that we've extensively discussed in this issue:

#1901636: Importing a disabled format that is assigned to a role causes a fatal

Alas, text formats cannot be deleted, so they have to be disabled.

In turn, in one way or another, we will have to deal with disabled config entities. So I'm actually inclined to simply move forward here, since all this patch is really doing is to make the existing concept more generic. Which in turn means that we need to cater for the possibility within and outside of the ConfigEntity API.

damiankloip’s picture

#102: 1826602-102.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system, +VDC, +Configurables

The last submitted patch, 1826602-102.patch, failed testing.

damiankloip’s picture

Assigned: Unassigned » damiankloip

I'll reroll this on the train.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
79.56 KB

rerolled, I have amended FilterFormat and Block config entities too.

Status: Needs review » Needs work

The last submitted patch, 1826602-107.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
749 bytes
78.94 KB

Another reroll, and don't try to call filter on format arrays.

Status: Needs review » Needs work

The last submitted patch, 1826602-110.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
6.89 KB
85.65 KB

This should fix the views tests, let's see how we're looking after this.

Status: Needs review » Needs work

The last submitted patch, 1826602-112.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.02 KB
85.67 KB

I think this is largely due to me completely messing up filter.module

Status: Needs review » Needs work

The last submitted patch, 1826602-114.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
85.12 KB

Hopefully, this is less fails again...

Status: Needs review » Needs work

The last submitted patch, 1826602-116.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
828 bytes
84.93 KB

Looks random, but just removed a couple of @todos which were put in but I don't think are correct. We added these procedural wrappers for drush mainly. As it needs a procedural function to call.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.phpundefined
@@ -89,6 +96,29 @@ public function set($property_name, $value, $langcode = NULL) {
+   * Implements Drupal\Core\Config\Entity\ConfigEntityInterface::enable().
...
+   * Implements Drupal\Core\Config\Entity\ConfigEntityInterface::disable().
...
+   * Implements Drupal\Core\Config\Entity\ConfigEntityInterface::status().

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityListController.phpundefined
@@ -23,4 +24,31 @@ public function load() {
+   * Overrides Drupal\Core\Entity\EntityListController::getOperations();

\Drupal

+++ b/core/modules/block/block.moduleundefined
@@ -365,20 +365,17 @@ function _block_rehash($theme = NULL) {
-    if (!empty($region) && $region != BLOCK_REGION_NONE && !isset($regions[$region]) && $status == 1) {
+    if (!empty($region) && $region != BLOCK_REGION_NONE && !isset($regions[$region]) && $status) {

Heh, that was ugly

+++ b/core/modules/block/block.moduleundefined
@@ -365,20 +365,17 @@ function _block_rehash($theme = NULL) {
     // Set region to none if not enabled and make sure status is set.
     if (empty($status)) {

This comment needs adjusting, and this could probably be if (!$status) {

+++ b/core/modules/block/block.moduleundefined
@@ -365,20 +365,17 @@ function _block_rehash($theme = NULL) {
       $block->set('region', BLOCK_REGION_NONE);
-      $block->set('status', 0);
-      $block->save();

Good catch on the status bit, but we still need the save() for the region.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityStatusTest.phpundefined
@@ -0,0 +1,54 @@
+ * Contains Drupal\config\Tests\ConfigEntityStatusTest.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityStatusUITest.phpundefined
@@ -0,0 +1,59 @@
+ * Contains Drupal\config\Tests\ConfigEntityStatusUITest.

Contains \Drupal

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityStatusTest.phpundefined
@@ -0,0 +1,54 @@
+use Drupal\Core\Entity\EntityMalformedException;

Not needed?

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityStatusTest.phpundefined
@@ -0,0 +1,54 @@
+   * Test the enabling/disabling of entities.

Tests

+++ b/core/modules/config/tests/config_test/config_test.moduleundefined
@@ -146,3 +159,25 @@ function config_test_config_test_create(ConfigTest $config_test) {
+function config_test_entity_enable(ConfigTest $config_test) {
+  $config_test->enable()->save();
+  return new RedirectResponse(url('admin/structure/config_test', array('absolute' => TRUE)));

Nice!

+++ b/core/modules/filter/filter.moduleundefined
@@ -351,7 +350,7 @@ function filter_formats($account = NULL) {
-        if (!empty($filter_format->status)) {
+        if ($filter_format->status()) {

@@ -481,7 +480,7 @@ function filter_get_filter_types_by_format($format_id) {
-    return $filter->status;
+    return !empty($filter->status);

This looks odd. Opposite changes?

+++ b/core/modules/node/lib/Drupal/node/Tests/Views/StatusExtraTest.phpundefined
@@ -35,7 +35,8 @@ public static function getInfo() {
-    state()->set('menu_rebuild_needed', TRUE);
+    menu_router_rebuild();

+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/AreaTest.phpundefined
@@ -173,7 +173,10 @@ public function testTitleArea() {
+    // Force the rebuild of the menu.
+    menu_router_rebuild();

This seems unfortunate. But it *is* in a test

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewListController.phpundefined
@@ -82,7 +82,9 @@ public function buildHeader() {
+   * @todo Obsolete after extending from ConfigEntityListController.

Why not just remove it?

damiankloip’s picture

FileSize
6.02 KB
85.39 KB

Nice! Thanks for the review, some good points.

  • Sorry, yeah, I totally should have kept in that save() call. Changed the comments and now just using !$status
  • We can't quite remove the whole getOperations() method, as we need the clone link. But everything else, I've ripped out

Status: Needs review » Needs work

The last submitted patch, 1826602-120.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
623 bytes
85.7 KB

To inherit from parent getOperation methods, we need View::uri to have a proper implementation.

Status: Needs review » Needs work

The last submitted patch, 1826602-122.patch, failed testing.

damiankloip’s picture

Assigned: damiankloip » Unassigned
Status: Needs work » Needs review
FileSize
776 bytes
86.12 KB

We also need to ACTUALLY inherit the ConfigEntityListController.

Status: Needs review » Needs work
Issue tags: -Configuration system, -VDC, -Configurables

The last submitted patch, 1826602-124.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +VDC, +Configurables

#124: 1826602-124.patch queued for re-testing.

dawehner’s picture

+++ b/core/modules/action/tests/action_bulk_test/config/views.view.test_bulk_form.ymlundefined
@@ -143,6 +143,6 @@ display:
-disabled: '0'
+status: '1'

That is great, no reverse logic anymore.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityStatusTest.phpundefined
@@ -0,0 +1,53 @@
+    $entity = entity_create('config_test', array(
...
+    $entity = entity_load('config_test', $entity->id());

Shouldn't we use the entity manager to load the entity via the storage controller? Urg, yeah this makes it way harder to read.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityStatusUITest.phpundefined
@@ -0,0 +1,59 @@
+    $disable_path = "admin/structure/config_test/manage/$id/disable";
...
+    $enable_path = "admin/structure/config_test/manage/$id/enable";

Should/Could we use entity uri as well similar to the other part before?

+++ b/core/modules/config/tests/config_test/config_test.moduleundefined
@@ -146,3 +159,25 @@ function config_test_config_test_create(ConfigTest $config_test) {
+function config_test_entity_enable(ConfigTest $config_test) {

Missing @return

+++ b/core/modules/node/lib/Drupal/node/Tests/Views/StatusExtraTest.phpundefined
@@ -35,7 +35,8 @@ public static function getInfo() {
-    state()->set('menu_rebuild_needed', TRUE);

Why not keep using state(), as it worked so far.

+++ b/core/modules/views/lib/Drupal/views/Tests/ModuleTest.phpundefined
@@ -182,15 +182,15 @@ public function testLoadFunctions() {
+    $this->assertTrue($view->status(), 'A view has been enabled.');
+    $this->assertEqual($view->status(), views_view_is_enabled($view), 'views_view_is_enabled is correct.');

So in general i'm wondering whether we could enabled()/disabled as methods as they might be easier to read then status(). We do have a enable method, so why not also one boolean to check the status? Alternative we could provide a setStatus instead of enable()/disabled() ... sounds odd to have two separate ways.

+++ b/core/modules/views/lib/Drupal/views/ViewStorageInterface.phpundefined
--- a/core/modules/views/tests/views_test_config/test_views/views.view.test_field_output.yml
+++ b/core/modules/views/tests/views_test_config/config/views.view.test_views_move_to_field.ymlundefined

+++ b/core/modules/views/tests/views_test_config/config/views.view.test_views_move_to_field.ymlundefined
@@ -1,25 +1,19 @@
diff --git a/core/modules/views/tests/views_test_config/test_views/views.view.test_field_output.yml b/core/modules/views/tests/views_test_config/config/views.view.test_views_move_to_handler.yml

--- a/core/modules/views/tests/views_test_config/test_views/views.view.test_field_output.yml
+++ b/core/modules/views/tests/views_test_config/config/views.view.test_views_move_to_handler.ymlundefined

Haven't we removed that file already?

+++ b/core/modules/views/tests/views_test_config/test_views/views.view.test_argument_default_fixed.ymlundefined
@@ -1,7 +1,7 @@
-disabled: '0'
+status: '1'

Desperate calling for a review on the plugin_id patch, as all this changes have the potential to break that one again.

damiankloip’s picture

FileSize
2.79 KB
84 KB

Thanks for the feedback! I made those changes, except for the entity_load() one, I'm not sure about that.

EDIT: Those files have been removed too.

Status: Needs review » Needs work

The last submitted patch, 1826602-128.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
83.19 KB

Shouldn't have removed test_view_output file.

damiankloip’s picture

#130: 1826602-130.patch queued for re-testing.

damiankloip’s picture

Just checking this after the plugin id patch.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Its nice to see this code move out of Views. Major props to @damiankloip, you really stuck with this one! :)

olli’s picture

+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/AreaTest.php
@@ -173,7 +173,10 @@ public function testTitleArea() {
+    // Force the rebuild of the menu.
+    menu_router_rebuild();

It is in a test, but I'm not 100% sure we need that anymore.

+++ b/core/modules/views/tests/views_test_config/test_views/views.view.test_display.yml
@@ -1,7 +1,7 @@
-disabled: '1'
+status: '1'

Oops.

Since view->enable() used to save the view, should view->save() be added to views_enable_view?

olli’s picture

Status: Reviewed & tested by the community » Needs work
damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.55 KB
84.33 KB

Good spots! and fair points. We don't need the menu_router_rebuild() anymore, so this should be fine now. This patch has been around a while you see :)

Also, I am happy with views_[en|dis]able_view saving too, as that matches what we had before, and makes a more useful wrapper. Otherwise, you would have to do views_enable_view($view); $view->save(); which kind of defeats the purpose of a helper wrapper I guess.

Also flipped the status back to 0 on the test_display view.

olli’s picture

Status: Needs review » Reviewed & tested by the community
Dave Reid’s picture

Wow, thank you for flipping the 'disabled' property into 'status'. That's always bugged me.

What happens when a contact form category which is set to default is disabled?

damiankloip’s picture

Yeah, it's great to have a status property that isn't inverted :)

In response to your question: Absolutely nothing, unless a. a config entity type chooses to use statuses, and b. the actual implementing module deals with this itself. E.g. You must check $entity->status() and decide how this is dealt with. So this is up to the calling code. The loading of entities is not affected by this. This just standardises the way we should use statuses when we need to.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Yeah I can't stand $view->disabled. While I'm paranoid about disabling of anything, at the moment this is just taking existing concepts of disabled config entities and making them consistent, and doesn't impose usage on anyone. We can then take any implementation of that case by case.

Patch is 99% updating default views, rest looks good, committed/pushed to 8.x.

sun’s picture

Status: Fixed » Active

Yay, thank you for sticking with this :) Really glad to see this in.

A couple of remarks though:

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -27,6 +27,13 @@
+  public $status;

1) Since this is a public property, isn't its value automatically exported for all config entities now? Perhaps I'm missing something...? At least the default langcode property right above the new $status property is exported for all config entities. If this is handled in a special way somewhere, then at least a @see would be helpful.

2) I think we should add a default value of TRUE here, so that status() automatically works correctly, even when not going through the ConfigStorageController?

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
@@ -32,4 +32,27 @@ public function getOriginalID();
+  /**
+   * Returns whether the configuration entity is enabled.
+   *
+   * @return bool
+   */
+  public function status();

I think we should actually add some architectural documentation here — essentially what we've discussed and clarified in-depth in this issue; i.e., that the status does not affect loading, that it only takes effect when explicitly declared in entity_keys, and that each entity type/controller/integration needs to check and handle the status on its own.

Most importantly, we should document that disabled config entities MUST always be available on an API level, and that the disabled status MAY only have UI implications (more or less ~access).

+++ b/core/modules/views/lib/Drupal/views/ViewStorageInterface.php
@@ -13,22 +13,4 @@
 interface ViewStorageInterface extends \IteratorAggregate, ConfigEntityInterface {

Looks like this interface is obsolete now? Do we have a follow-up issue to kill it? :)

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewListController.php
@@ -82,50 +82,14 @@ public function buildHeader() {
   public function getOperations(EntityInterface $view) {
...
     $definition['clone'] = array(
       'title' => t('Clone'),
       'href' => "$path/clone",

Should we create a follow-up for incorporating a 'clone' operation into the ConfigEntityListController?

damiankloip’s picture

FileSize
1.39 KB

I know, it's good to get this in :) The patch was getting rather troublesome to keep re rolling...

1. Yeah, I guess so, if this uses the default implementation of getExportProperties() which works off of reflection. I think having the status all the time is OK though? If it's not used, it doesn't matter. It could be converted much easier in the future if needed then...
2. I think I originally had it defaulting to TRUE, but we changed this for some reason - Happy for this to come back though. see patch
3. Yeah, docs there would certainly not hurt. see patch.
4. No issue for that, I tihnk this can wait though. If at code freeze we have nothing in, we can remove?
5. Yeah, good plan! That could be ok. I will open an issue. Not sure if there are any complexities involved in making this generic.

damiankloip’s picture

Status: Active » Needs review
damiankloip’s picture

FileSize
2.16 KB

We also need to add back the 'ajax' => TRUE to the operations for enable/disable..

Status: Needs review » Needs work

The last submitted patch, 1826602-144.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.77 KB
2.38 KB

That adds the ajax to the generic controller, we just need it in Views.

I really would like to see a setStatus() method.
Let's say you want to react to a value, and enable/disable based on that.

Currently:

if ($foo) {
  $entity->enable();
}
else {
  $entity->disable();
}
$entity->save();

Proposed:

$entity->setStatus($foo)->save();

It was discussed earlier and then removed, not sure why.

dawehner’s picture

Oh I really like that change, you want for sure that on every configuration entity.

I have been asking for a setStatus() command earlier, though sun suggested to use one for enable()/disable().
Would you suggest to have both enable()/disable() and setStatus()?

dawehner’s picture

damiankloip’s picture

Yeah, originally I had enable() and disable()which both called setStatus() to actually do the setting. So they were just convenience methods but you could still use setStatus() too. I guess you get all the flexibility then. So I am happy to go back to something like this as outlined in #146.

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewListController.phpundefined
@@ -86,16 +86,22 @@ public function buildHeader() {
+    // Restore AJAX functionality to enable/disable operations.

Not sure this is 'restoring' the functionality? Otherwise the changes look good. Sorry, I didn't really think about that, I just quickly added the ajax to the base class :?

sun’s picture

The expressiveness of enable() and disable() is generally appealing.

We can have a setStatus(bool $value) in parallel, but for the sake of extensibility/overrides, we'd have to declare which method(s) is/are the "master method(s)"; i.e., which one is supposed to be overridden. I guess if there was a setStatus(), then rather that; and in turn, enable() and disable() would call into setStatus()...?

Sidenote: Not sure whether that wouldn't be 1-2 public methods too much for a facility that shouldn't play a key role in the first place...?

andypost’s picture

Another related issue - I see Enable as a first action for configurables that inherit getOperations() (picture, menu, contact)

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.phpundefined
@@ -31,7 +31,7 @@
-  public $status;
+  public $status = TRUE;

This really required.

andypost’s picture

Issue tags: +Needs tests

And that actially point that we lack of testing for order of operations on entities

damiankloip’s picture

@sun, see #149, I proposed if we add a setStatus then enable and disable would call this.

@andypost, Tests for the order or operations I think should be a follow up.

If no one else does first, I will roll a new patch later this morning.

damiankloip’s picture

I think we should go with this patch to tie up loose ends here.

Then additional tests as described by @andypost and adding a setStatus() method can be follow ups.

damiankloip’s picture

Issue tags: -Needs tests
FileSize
704 bytes
2.38 KB

Oh, and the patches.

damiankloip’s picture

Status: Needs review » Needs work
Issue tags: -Configuration system, -VDC, -Configurables

The last submitted patch, 1826602-154.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#155: 1826602-154.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1826602-154.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +VDC, +Configurables

#155: 1826602-154.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

Fabianx’s picture

Uh, no change notice on that?

andypost’s picture

Priority: Normal » Critical
Status: Closed (fixed) » Needs work
Issue tags: +Needs change record

Yes, just describe properties and methods at least

catch’s picture

Title: Allow all configuration entities to be enabled/disabled » Change notice: Allow all configuration entities to be enabled/disabled
Status: Needs work » Active
dawehner’s picture

andypost’s picture

I think all new methods needs to be described:

  • setStatus() - done
  • enable()
  • disable()
  • status()

Also here's a duplicated change notice http://drupal.org/node/1926374

damiankloip’s picture

Assigned: Unassigned » damiankloip

I don't think we just need to describe the methods, but more the actual concept around this instead. As this was the source of alot of conversation in this issue. I will add to the change notice.

damiankloip’s picture

damiankloip’s picture

Assigned: damiankloip » Unassigned
andypost’s picture

Title: Change notice: Allow all configuration entities to be enabled/disabled » Allow all configuration entities to be enabled/disabled
Priority: Critical » Normal
Status: Active » Fixed

Awesome!

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: -Needs change record

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.