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
Comment | File | Size | Author |
---|---|---|---|
#155 | 1826602-154.patch | 2.38 KB | damiankloip |
#155 | interdiff.txt | 704 bytes | damiankloip |
#146 | vdc-1826602-146.patch | 2.38 KB | tim.plunkett |
#146 | interdiff.txt | 1.77 KB | tim.plunkett |
#144 | 1826602-144.patch | 2.16 KB | damiankloip |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedHere is a patch to get the ball rolling.
Also tagging with Needs tests as we will need more, or move the views tests too.
Comment #2
damiankloip CreditAttribution: damiankloip commentedComment #4
gddIf 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?
Comment #5
damiankloip CreditAttribution: damiankloip commentedThis 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.
Comment #6
damiankloip CreditAttribution: damiankloip commentedAdded some normal CRUD tests to testCRUD.
Comment #7
damiankloip CreditAttribution: damiankloip commentedComment #9
damiankloip CreditAttribution: damiankloip commentedRerolled after #1763974: Convert entity type info into plugins
Comment #10
damiankloip CreditAttribution: damiankloip commentedComment #12
damiankloip CreditAttribution: damiankloip commentedpostponed 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.
Comment #13
xjmNot having this is a regression from D7 Views, so bumping to major.
Comment #14
tim.plunkettThis is already in Views, we're just working to abstract it for all ConfigEntity.
Comment #15
damiankloip CreditAttribution: damiankloip commentedHere is the latest version, based on @heyrockers patch in #1697256: Create a UI for importing new configuration.
Reviews welcome.
Comment #16
damiankloip CreditAttribution: damiankloip commentedLet's give this a try.
Comment #18
damiankloip CreditAttribution: damiankloip commentedRe rolled.
Comment #19
yched CreditAttribution: yched commentedConfigStorageController::setStatus()/getStatus() look like they could be abstracted to act on any "property" besides 'enabled' ?
Comment #20
aspilicious CreditAttribution: aspilicious commentedThere is alrdy an issue to change this to $enabled. I fully support that.
/**
Damnit how did this get in in previous patches. It will be gone now ;)
Comment #22
damiankloip CreditAttribution: damiankloip commentedFixed those changes, and the suggestion in #19 seems like a good one too.
I haven't had time to fix the fails yet.
Comment #23
aspilicious CreditAttribution: aspilicious commentedpatch is identical
Comment #24
damiankloip CreditAttribution: damiankloip commentedNo. It adds a key parameter to the getStatus and setStatus methods.
Comment #25
andypostIt looks strange to have imageStyle disabled or a whole menu... Suppose better to provide a annotation
disableable
for config entitiesComment #26
damiankloip CreditAttribution: damiankloip commentedTo 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.
Comment #27
damiankloip CreditAttribution: damiankloip commentedFixed (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.
Comment #29
damiankloip CreditAttribution: damiankloip commentedHa, seems it's just our views tests that are failing :) I'll sort this out.
Comment #30
attiks CreditAttribution: attiks commentedTrying 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?
Comment #31
damiankloip CreditAttribution: damiankloip commentedYes, But I think you would need this extra file now anyway, regardless of enabled/disabled status.
Comment #32
gddYes, please see the discussion this came from at #1697256-44: Create a UI for importing new configuration
Comment #33
attiks CreditAttribution: attiks commented#31 for some reason I missed the incarnation of the manifest system.
Comment #34
damiankloip CreditAttribution: damiankloip commented@attiks, yes, it was tucked away in that config UI patch :)
Comment #35
sunThere 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:
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.)
Add a new
ConfigEntityToggleableInterface
(better name welcome).Config entities that implement this interface have a status and can be enabled and disabled.
Enhance
ConfigStorageController
with the proposed enable/disable/isEnabled methods, but checkinstanceof 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 theentity_keys
array in entity info, which is the pattern for enabling additional entity semantics/behavior already (such as UUIDs, revision support, etc).Comment #36
tim.plunkettViews already currently has
protected $status
, see ViewStorageInterfaceHowever, 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.
Comment #37
sunTo 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.
Comment #38
damiankloip CreditAttribution: damiankloip commented@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.
Comment #39
sunWe 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.
Comment #40
damiankloip CreditAttribution: damiankloip commentedOk, how about something like this instead?
Comment #42
damiankloip CreditAttribution: damiankloip commentedSee how this goes...
Comment #44
dawehnerThis patch fixes hopefully all failing tests.
For whatever reason just using
doesn't cause the page to appear, maybe someone has a clue why.
Comment #45
damiankloip CreditAttribution: damiankloip commentedYeah, 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.
Comment #46
damiankloip CreditAttribution: damiankloip commentedJust changed the enable and disable methods on the controller to use save() directly and not $entity->save().
Comment #47
sunThis 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:
(...which, alas, questions whether those should be methods on the entity in the first place, but I don't see why not.)
Comment #48
sunOh, 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.
Comment #49
damiankloip CreditAttribution: damiankloip commentedOk, 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 :)
Comment #50
sunYes, 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.
Comment #52
damiankloip CreditAttribution: damiankloip commentedok, 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.
Comment #54
sunThanks! :)
The default value should be set in the ::create() method of the (config) storage controller, just like uuid is set there by default.
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)
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.
Missing * :)
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.
This method should be obsolete with the above ConfigEntity class changes.
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?
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)?
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.
Specific entity classes do not need to repeat properties of the base class, so this ConfigTest entity property can be removed.
Comment #55
sunHelping you out a bit.
Comment #56
damiankloip CreditAttribution: damiankloip commented@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.
Comment #57
damiankloip CreditAttribution: damiankloip commentedOops, cross post! Maybe we can see how similar the two are ? ;)
Comment #59
sunAttached is a reverse-interdiff between #56 (old) and #55 (new)
Comment #60
sunHeh. Surprisingly similar, even. ;)
ah, removing that condition makes sense.
Comment #61
damiankloip CreditAttribution: damiankloip commentedOk, 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.
Comment #63
damiankloip CreditAttribution: damiankloip commentedThis should pass the tests now. Hopefully we are good to go!
Comment #64
sunExcellent! :)
Comment #65
tim.plunkettExtra +1 from me!
Comment #66
catchI 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?
Comment #67
sun@catch:
I think I understood your concerns. And I think they are irrelevant for this issue. Here's why:
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.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.As you already alluded to, there are use-cases for disabled configuration. Regardless of whether it was supplied as default or not.
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).
Comment #68
catch@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.
Comment #69
attiks CreditAttribution: attiks commented#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.
Comment #70
catchNo 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.
Comment #71
attiks CreditAttribution: attiks commented#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
Comment #72
catch@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?
Comment #73
attiks CreditAttribution: attiks commented#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.
Comment #74
sunTypical use-cases from my POV:
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.Comment #75
damiankloip CreditAttribution: damiankloip commentedI 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? :)
Comment #76
sunGiven 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.
Comment #77
damiankloip CreditAttribution: damiankloip commentedAgain, 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.
Comment #78
sunI'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.
Comment #79
damiankloip CreditAttribution: damiankloip commentedYep, 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?
Comment #80
damiankloip CreditAttribution: damiankloip commentedAnd admittedly, the title is pretty scary too...
Comment #81
Fabianx CreditAttribution: Fabianx commented#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?).
Comment #82
sunThat 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. :)
Comment #83
dawehnerProbably 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.
Comment #84
tim.plunkettComment #85
damiankloip CreditAttribution: damiankloip commentedExample case for this: #1868772: Convert filters to plugins is currently just checking $filter->status. I think it should be using this instead...
Comment #86
xjmSo @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:listAll()
.See also: #1881096: Regression: Site broken when disabling module without removing block instances.
Comment #87
sunThe 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.Comment #88
xjmThere'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
, notnode.views.view.frontpage.yml
.This seems completely counterintuitive to me. If the module isn't there, it can't be processed as usual.
Comment #89
xjmOh, 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.
Comment #90
EclipseGc CreditAttribution: EclipseGc commentedOK, 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
Comment #91
sun@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).
Comment #92
EclipseGc CreditAttribution: EclipseGc commented@sun,
I agree. This was a misunderstanding on my part with regard to the desired outcome here.
Comment #93
EclipseGc CreditAttribution: EclipseGc commentedAlthough, if tests come back green on it, I'd still suggest the module_disable change I made.
Comment #95
xjmGeneral 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.
Comment #96
xjmThe merge conflicts.
Comment #98
xjmAnd fixing the Views blocks.
Comment #100
damiankloip CreditAttribution: damiankloip commentedOk, 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.
Comment #102
damiankloip CreditAttribution: damiankloip commentedOh, the load method was from way back too. Crazy.
Comment #103
xjmIt's entirely possible I screwed up the reroll or that the merge wasn't smart enough. :)
Comment #104
sunMeanwhile, 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.
Comment #105
damiankloip CreditAttribution: damiankloip commented#102: 1826602-102.patch queued for re-testing.
Comment #107
damiankloip CreditAttribution: damiankloip commentedI'll reroll this on the train.
Comment #108
damiankloip CreditAttribution: damiankloip commentedrerolled, I have amended FilterFormat and Block config entities too.
Comment #110
damiankloip CreditAttribution: damiankloip commentedAnother reroll, and don't try to call filter on format arrays.
Comment #112
damiankloip CreditAttribution: damiankloip commentedThis should fix the views tests, let's see how we're looking after this.
Comment #114
damiankloip CreditAttribution: damiankloip commentedI think this is largely due to me completely messing up filter.module
Comment #116
damiankloip CreditAttribution: damiankloip commentedHopefully, this is less fails again...
Comment #118
damiankloip CreditAttribution: damiankloip commentedLooks 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.
Comment #119
tim.plunkett\Drupal
Heh, that was ugly
This comment needs adjusting, and this could probably be
if (!$status) {
Good catch on the status bit, but we still need the save() for the region.
Contains \Drupal
Not needed?
Tests
Nice!
This looks odd. Opposite changes?
This seems unfortunate. But it *is* in a test
Why not just remove it?
Comment #120
damiankloip CreditAttribution: damiankloip commentedNice! Thanks for the review, some good points.
!$status
Comment #122
damiankloip CreditAttribution: damiankloip commentedTo inherit from parent getOperation methods, we need View::uri to have a proper implementation.
Comment #124
damiankloip CreditAttribution: damiankloip commentedWe also need to ACTUALLY inherit the ConfigEntityListController.
Comment #126
damiankloip CreditAttribution: damiankloip commented#124: 1826602-124.patch queued for re-testing.
Comment #127
dawehnerThat is great, no reverse logic anymore.
Shouldn't we use the entity manager to load the entity via the storage controller? Urg, yeah this makes it way harder to read.
Should/Could we use entity uri as well similar to the other part before?
Missing @return
Why not keep using state(), as it worked so far.
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.
Haven't we removed that file already?
Desperate calling for a review on the plugin_id patch, as all this changes have the potential to break that one again.
Comment #128
damiankloip CreditAttribution: damiankloip commentedThanks 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.
Comment #130
damiankloip CreditAttribution: damiankloip commentedShouldn't have removed test_view_output file.
Comment #131
damiankloip CreditAttribution: damiankloip commented#130: 1826602-130.patch queued for re-testing.
Comment #132
damiankloip CreditAttribution: damiankloip commentedJust checking this after the plugin id patch.
Comment #133
tim.plunkettIts nice to see this code move out of Views. Major props to @damiankloip, you really stuck with this one! :)
Comment #134
olli CreditAttribution: olli commentedIt is in a test, but I'm not 100% sure we need that anymore.
Oops.
Since view->enable() used to save the view, should view->save() be added to views_enable_view?
Comment #135
olli CreditAttribution: olli commentedComment #136
damiankloip CreditAttribution: damiankloip commentedGood 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.
Comment #137
olli CreditAttribution: olli commentedComment #138
Dave ReidWow, 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?
Comment #139
damiankloip CreditAttribution: damiankloip commentedYeah, 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.
Comment #140
catchYeah 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.
Comment #141
sunYay, thank you for sticking with this :) Really glad to see this in.
A couple of remarks though:
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?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).
Looks like this interface is obsolete now? Do we have a follow-up issue to kill it? :)
Should we create a follow-up for incorporating a 'clone' operation into the ConfigEntityListController?
Comment #142
damiankloip CreditAttribution: damiankloip commentedI 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.
Comment #143
damiankloip CreditAttribution: damiankloip commentedComment #144
damiankloip CreditAttribution: damiankloip commentedWe also need to add back the
'ajax' => TRUE
to the operations for enable/disable..Comment #146
tim.plunkettThat 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:
Proposed:
It was discussed earlier and then removed, not sure why.
Comment #147
dawehnerOh 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()?
Comment #148
dawehnerJust linking a follow up: #1909878: Fix statuses for cached (tempstore) views data in views_ui
Comment #149
damiankloip CreditAttribution: damiankloip commentedYeah, 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.
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 :?
Comment #150
sunThe expressiveness of
enable()
anddisable()
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 asetStatus()
, then rather that; and in turn,enable()
anddisable()
would call intosetStatus()
...?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...?
Comment #151
andypostAnother related issue - I see Enable as a first action for configurables that inherit getOperations() (picture, menu, contact)
This really required.
Comment #152
andypostAnd that actially point that we lack of testing for order of operations on entities
Comment #153
damiankloip CreditAttribution: damiankloip commented@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.
Comment #154
damiankloip CreditAttribution: damiankloip commentedI 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.
Comment #155
damiankloip CreditAttribution: damiankloip commentedOh, and the patches.
Comment #156
damiankloip CreditAttribution: damiankloip commentedJust created #1911814: Add a setStatus() method for configuration entities
Comment #158
tim.plunkett#155: 1826602-154.patch queued for re-testing.
Comment #160
damiankloip CreditAttribution: damiankloip commented#155: 1826602-154.patch queued for re-testing.
Comment #161
tim.plunkettThanks!
Comment #162
webchickCommitted and pushed to 8.x. Thanks!
Comment #164
Fabianx CreditAttribution: Fabianx commentedUh, no change notice on that?
Comment #165
andypostYes, just describe properties and methods at least
Comment #166
catchComment #167
dawehnerhttp://drupal.org/node/1926376
Comment #168
andypostI think all new methods needs to be described:
Also here's a duplicated change notice http://drupal.org/node/1926374
Comment #169
damiankloip CreditAttribution: damiankloip commentedI 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.
Comment #170
damiankloip CreditAttribution: damiankloip commentedAmended : http://drupal.org/node/1926376
Comment #171
damiankloip CreditAttribution: damiankloip commentedComment #172
andypostAwesome!
Comment #174
xjmUntagging. Please remove the "Needs change notification" tag when the change notice task is complete.