When deleting a content type field, users do not realize the related View also is deleted

OH, yeah it should ideally just remove the corresponding field from the view.

Btw. I disagree that this should be done in 8.1.x but instead fixed in 8.0.x directly. It also sounds at least major for me.

Do fields not have a delete confirmation form? That should list what will be removed/updated.

Yes, this something that can be fixed in 8.0.x, I think the assigment was just done incorrectly. Sounds like a non-trivial thing to implement, though. A dependency could be *anywhere*, in a field, argument, filter, it could be a menu the view is displayed in.. every plugin could be responsible.. so we would kind of need to go through all them, find those that depend on it and somehow automatically drop them?

Too add some more details here. I did some testting. Deleting a date does indeed delete the view IF the view is a view of fields. It does not delete the view if it is a view of content teasers. Also confirmed this is not specific to just date fields.

I can also verify that when the date field is deleted, the view is also deleted but the view of teaser is not deleted.

Additional info: If the same field is used as an existing field on another content type, deleting the field from either content type does not result in the view being removed. So this is only happening when the field data is completely removed. Still digging for what service or hook is deleting the view instead of updating it.

The process starts in ConfigEntityBase::preDelete(), there it's looking for configuration that depends on the entity that is being deleted and deletes those things, unless they implement the methods to update themself.

See EntityDisplayBase::onDependencyRemoval() on how the view/form display configuration updates itself when a field is deleted. Views will have to implement something like that.

That helps a lot Berdir. Thank you for pointing me in the right direction. We had 5 well established engineers looking at this issue on a codesprint at a drupal camp yesterday and we came up empty. This helps.

This is still a work in progress but already prevents forms from being deleted and just removes the field on the view instead.
We still have to make sure the same happens for filters and sorts (and maybe some other stuff I'm forgetting) I assume.

If somebody could already have look at this to see if this is going in the right direction that would be great.

Thanks geertvd I will review this later today.

Just checked this for filters, sorts and contextual filters. The view doesn't get removed for those but the "Broken/missing handler" label appears.
Ideally onDependencyRemoval() should be triggered for those also though.
Having a look now to find out why these are not dependencies to the field.

These dependencies should have been fixed in #2372855: Add content & config entity dependencies to views is there any reason why it wasn't added there?
IS states that "fields used in filters" should also be dependent but at the moment that's not the case.

If I were to make them dependent I'n not sure where's the best place to implement calculateDependencies()

This should handle removal of all dependent handlers (including sorts, filters, etc.)

Still working on making filters and other handler types dependent since this is just the case for fields right now.

Kinda stuck here.

The following plugins are still missing config dependency:

• Fields used in any filters
• Fields used in any sorts
• Fields used in any arguments
• Fields used in any relationships

Since these plugins don't have a baseclass that is used for entity fields, I don't think we can use calculateDependencies() here.
I discussed this briefly with dawehner at drupaldevdays, he helped me in the right direction to write the onDependencyRemoval() method more generically for all handler types.
We were thinking about doing something simular to calculate the dependencies for handler types also if I remember correctly but I can't figure out where is the best place to do that.

Most views plugins would depend on the field storage and that doesn't get removed until the the last instance of that field is removed (obviously).

I would argue that modules should be hard dependencies for views but fields should be soft dependencies (Is there such a thing?) since there are broken handler fallbacks.

It would be great if we showed a view has broken dependencies in the view list but I disagree with trying to remove the missing components. You want to see the broken handlers so you can fix them which probably means replacing one field with another if you are refactoring a content type.

Thanks for reviving this issue.

How this is working in d7 now (for fields):

1. I add the tag field to a test view:
   
2. I remove the tag field, notice we are getting a notification message explaining that the view has been updated.
   

Let's go over how it currently works in d8:

1. I create my test view again and add the tag field:
   
2. I delete the field, I get a notice in between saying I have some dependencies that will be updated. My test view is not there, it should be there (my patch in #12 didn't fix that yet, this could serve as a replacement for the notification message we are getting in d7).
   
3. When I check my views overview, my view has disappeared :(.

Now when I apply patch #12 steps 1 and 2 remain the same, but when I go back to my view, it's still there and my field has been cleaned up from the field handlers.

I would argue that modules should be hard dependencies for views but fields should be soft dependencies (Is there such a thing?) since there are broken handler fallbacks.

I wouldn't call showing a broken/missing handler a soft dependency, I think we should really only have the broken handlers when something has been changed/removed programmatically. If we can avoid using them we should.

You want to see the broken handlers so you can fix them which probably means replacing one field with another if you are refactoring a content type.

I don't know if that's an assumption we can make, this just adds more steps for the site builder.

I took a poll around the office and got very mixed opinions about removing the views component when a field is removed.

What about contextual filter?. If you remove a contextual filter you could possibly expose data that wasn't indented to be exposed such as student contact information on a view page that is intended to only have staff contact information exposed.

Another consideration is if rewrite is being used in a field to include other hidden fields.

I guess the point of these two is that there is more configuration in a views plugin than just the field and if you remove a field, filter, sort or contextual filter you are removing the rest of the configuration for that plugin as well.

Lastly, have we considered blocking the field removal because of views dependencies instead of removing the field from the view.

I took a poll around the office and got very mixed opinions about removing the views component when a field is removed.

:) Awesome, I'll do the same at my office.

I guess the point of these two is that there is more configuration in a views plugin than just the field and if you remove a field, filter, sort or contextual filter you are removing the rest of the configuration for that plugin as well.

Fair point, but I don't think we can store every possible setting of every possible handler in the missing/broken handler (not sure)

Lastly, have we considered blocking the field removal because of views dependencies instead of removing the field from the view.

For me this would be a very disturbing thing to do.

We discussed this issue, and the agreement is that the critical part about this issue is the fact that you are not told that the view will be deleted.

For this, I've just promoted #2442757: Move the field storage deletion if the last field has been removed to the UI layer to critical. Then you can at least manually fix your view before it gets deleted and you know about it. This is something that we can also fix after 8.0.0 if we don't get to it before.

I created #2575547: Formalize patterns for delete/uninstall of configuration dependencies as well that references this issue.

The concept in that issue is that there should also be soft dependencies which give warnings but are not strictly enforced. Modules must provide a fall-back if a dependency it declares as soft does not exist.

Working on a test.

Weird, I manually reproduced this issue a few weeks ago, but now cannot reproduce it.

#2575605: Field delete form does not display configuration dependencies that will be updated/deleted added a test that makes sure the view is at least shown as being deleted, you can probably steal some code from there :)

So to clarify, now that #2575605: Field delete form does not display configuration dependencies that will be updated/deleted is resolved, this issue is about allowing the field configuration to be removed from the view without deleting the view?

I've mixed feelings about this because I think it's complicated and also depends on how the field is used in the view. If the field is used for a field plugin, then it makes sense to simply remove the data the same as it is removed from the content type itself. However, if the field is used as a filter for the view, then this could result in the view disclosing more information than it did previously.

The other option is to disallow removing the field until it is removed from the view, which would be more in line with the config dependency expectations I think.

I agree with #25.

(Fixing the tag.)

However, if the field is used as a filter for the view, then this could result in the view disclosing more information than it did previously.

That's not dependencies problem; The problem here is that someone used a field as a filter. A user that with permission to delete fields should certainly be able to make the choice to delete fields from a view.

The other option is to disallow removing the field until it is removed from the view, which would be more in line with the config dependency expectations I think.

It's possible that there are irrelevant views (even disabled views) that could block someone from deleting a field.

OMG I just screwed up all my views are gone without any warning when I deleted a field.

Thank so much for ruin my day.

I second #25 as well. How a field gets used within a view can vary greatly, and the view might have a zillion other fields, relationships, contextual filters and what not. Recreating the view could be a major pain. If the field is used in a view, then the field should not be deletable yet, and it should show which views are using this field, to let the user decide to remove it from those views, or perhaps changes their mind and keep the field after all.

I feel either this or "1.) Allowing the field configuration to be removed from the view without deleting the view." would be far preferable to simply deleting the view (even with a confirmation / warning).

As a site builder who doesn't want to rebuild complex views, I like #25 but I'm also fine with other approaches where the relevant view won't be simply deleted. In my opinion it's critical to inform the users as fast as possible more clearly, even if the technical and conceptual questions of this issue need more time. For the time being, a confirmation dialog while field deletion tells you for example:

Configuration deletions
The listed configuration will be deleted.
View
- Articles

"The listed configuration will be deleted" may be correct from a technical view, but in my opinion it's not a very fortunate choice. In my case it was misleading: Quite experienced with Views in D7 but not in D8, I was not expecting at all that a view containing about 10 other fields, two relations and a contextual filter could be deleted because of a single deleted field. So I must have read the information wrongly as something like "The field configuration in the following View will be deleted." My fault not to check the meaning, no doubt, but I wouldn't have read the message wrongly if it said "The following View will be deleted." Looking at the structure of the warning text, it's however difficult to change the wording to this effect. Any suggestions?

I concur with Olafski. The message needs to be really blunt and obvious.

Given how many issues are floating around for that, this really feels like a uber-major for me.

I think I just experienced the same underlying issue, but triggered it a different way. I uninstalled views_filter_autosubmit (https://www.drupal.org/node/2475595) and the view I had enabled auto-submission on was deleted. The module just adds a Views Exposed Form Plugin and the deleted view was configured to use it. View Filter Auto Submit doesn't have any uninstall function so I think the view was deleted for the same underlying reason(s) views are deleted if a field is deleted.

(Maybe this is its own issue?)

@alexpott, @timplunkett, @dawehner, and I discussed this issue awhile back at DrupalCon New Orleans.

In a way, this issue is working as intended: the config dependency system necessarily cleans up dependencies when the user removes configuration. This is the solution we chose in #2575605: Field delete form does not display configuration dependencies that will be updated/deleted and #2442757: Move the field storage deletion if the last field has been removed to the UI layer. For important background on the overall problem space, see #2416409: Delete dependent config entities that don't implement onDependencyRemoval() when a config entity is deleted. (Also see e.g. #2212081: Remove views optional handler handling for how we changed Views in D8.)

It is not correct to "update" existing configuration automatically as we do with the entity displays. Say for example the field was being used to restrict access on the view. If we tried to "guess" the user's intentions and magically update the view, it could result in unintended information disclosure when suddenly all data was accessible. So, the correct way for the user to address the issue is to remove the field from the view first. That way, the user can reconfigure the view as appropriate.

The user is even informed on a confirmation form:

(I was unable to reproduce any way of not getting this confirmation message.)

However, it is clearly a major usability bug if users accidentally delete their configuration and lose the data. I think there are three problems here:

1. The delete section is at the bottom of what is potentially a long list of updated configuration.
2. Users get used to just confirming the less severe configuration entity updates and so are not aware they are deleting a whole view.
3. Even if the user sees the message about deletion, there is no information on how to fix it instead.

Therefore, we probably need to warn users more strongly before they delete configuration. One resolution we discussed is using a modal (similar to the modal that warns you before you click on a link in a preview and thus lose your user input). That message could also display short help text about editing the other thing first to remove the dependency as an alternative.

There are two major issues when a field is removed:

1. A field in a configured views field is removed: In this case its safe to remove the field and simply call it a day. IMHO we should do this, especially because its the way more common case.
2. A field in a configured views filter is removed: This case is more tricky, because removing a filter could mean information leakage. I think it would be fine to replace that with a filter which results in no results.

Does anyone has opinions about it? IMHO these two actions would solve most of the problems out there.

@dawehner, #43: Good idea to distinguish the two cases! I would be happy about this solution.

Said that, there will be other cases, e.g. a field used as sort criteria, but I guess even more, some of them not so obvious. Can we treat them all like case 1 (a field in a configured views field)?

Related issue for a similar destructive confirm form.

We now have #2773205: Come up with a design for highly destructive operations in confirm forms for an overall design for these highly destructive operations. This issue could start though by putting the Delete fieldset up top, improving the form text, etc., which would be independent of that interaction.

I'm not sure about #43. Who knows what custom behavior someone could have created for their view? I guess we could say the contrib/custom author is responsible for making the configuration system do the right thing in that case, but I doubt they will if we have struggled to with core. Maybe it's three separate issues: the one linked above, one for improving the config confirm form internally, and one for the dependency question for Views and their plugins.

#46
IMHO a broken site in that sense is still MUCH better than what we do at the moment. I cannot tell how many people asked me about this behaviour already and all of them lost significant work or at least significant time,

Is simply disabling the view out of the question? Seems like it might be a bit of all worlds. It "breaks" the site, but does not "destroy" any configuration. Not sure how it falls into line with the rest of the UX stuff though.

There's already some logic baked in for automatically updating configurations that depend on a deleted configuration. For instance, deleting a field does not delete the corresponding core.entity_view_display.* configurations that specify a dependency on that field. That dependent configuration is updated to remove the dependency instead. I'm not exactly sure of the logic in charge of this, and don't know if it's generic enough to be used in the case of views.

If that logic isn't generally reusable, I think simply leaving the view to use the broken handlers is acceptable, and we could potentially even create a report that highlights broken configurations?

I'd like to add that when I deleted a user role today, I also lost two views that used that role in the permissions. This was totally unexpected and a huge departure from what Drupal 7 "trained me" to expect.

Is there actually an issue which discusses to take a snapshot of config when these kind of deletions happens?

Re #51: If it isn't obvious deleting a content type does this also, although that is not as sympathetic as case as is a field deletion or a user role. This issue makes me sad because a view can be such a valued piece of configuration. I can understand the argument towards removing one because we cannot predict what the effect on a given view would be. But as a site builder, I would be furious at myself and everyone else by accidently deleting a view I hadn't backed up.

AARRGH, I JUST DELETED a View with 5 pages inside!! The current warning is not enough, even worse this being not the default behaviour in D7. Definitely a more prominent warning is needed :(

Why is this discussion leaning towards notifying users the views is to be deleted?? Better to have a broken view than no view at all.

#51 and on are leaning the other way.

Here is alternative approach:

• Don't delete existing views
• Disable existing views and tell people which ones got disabled
• Then the user can go back to this view, fix missing fields and call it a day

So make it a soft dependency? Seems like it might work. I'm not sure what dependencies the disabled view would be though. Because it can not depend on something that is not there.

Well, we could return nothing for the dependencies for disabled views.

That could break things like install order or optional config?

I would even say: don't delete existing views, and don't disable them. Let the view have big fat errors, but maybe still provide a warning that this view will need attention. That way a) the admin will know the view needs work and b) if the admin does not notice the warning, it's on them but then at least the view is not gone. This is more in line with D7 default behavior for Views IMHO.

@caspervoogt
Well, I wanted to make a new suggestion, because we circle around the do nothing at all idea, already, and this didn't lead to any pratical outcome.

+1 for #59/51

I like @dawehner's approach in #59. There were other reasonable approaches, but let's finally decide how to fix this huge problem. So, let's be pragmatic and take the #59 approach. And one appeal in case anybody has objections: please make clear if your objections are fundamental, tell us by which degree you disagree, and suggest another manageable approach.

+1 #59

So lets go with the approach @dawehner suggested #59

I also experienced this right now … lost three views, which took me quite some time to setup.
Also +1 for #51/#59 because I would expect to have "only" some broken fields instead of a deleted view.

#59 +1

Agree with #59, deleting a configuration / view is a BIG no-no. Disable, sure. Big fat errors, sure. Losing an entire configured view because the user had for a moment not paid attention? Disaster for UX - even if there is a warning before deletion.

+1 #59! Just lost a lot of views...

I'm working on that atm.

Here is a start of that.

Ran into this too, time to fix this, REALLY annoying.

Started with #76 as a basis and borrowed some ideas from #77.

I don't think we need to go into a whole new area of disabled stuff, we can always do that in a follow up. Logging the disabling of the View is in line with what already happens in \Drupal\Core\Entity\EntityDisplayBase, so lets go with that now.

Manually tested this and works for removing fields.

This incorporates the idea in #61 to make disabled Views return no dependencies, updated it to make it return no handler based dependencies.

Only thing I'm not totally happy with is that uninstalling the taxonomy module doesn't remove all taxonomy based Views (it just disables them).

Putting on needs review to trigger the bot, but obviously still needs work

diff --git a/core/modules/views/src/Entity/View.php b/core/modules/views/src/Entity/View.php
index 09808a7..7fa90dd 100644
--- a/core/modules/views/src/Entity/View.php
+++ b/core/modules/views/src/Entity/View.php
@@ -537,6 +537,7 @@ public function onDependencyRemoval(array $dependencies) {
       $arguments = [
         '@id' => $this->id(),
       ];
+      // @todo IMHO we should display that somehow in the UI, if possible.
       $this->getLogger()->warning("View '@id': View was disabled because its settings depend on removed dependencies.", $arguments);
       $changed = TRUE;
     }
diff --git a/core/modules/views/src/Plugin/views/field/EntityField.php b/core/modules/views/src/Plugin/views/field/EntityField.php
index f673e6b..aa9f7f8 100644
--- a/core/modules/views/src/Plugin/views/field/EntityField.php
+++ b/core/modules/views/src/Plugin/views/field/EntityField.php
@@ -1075,6 +1075,7 @@ public function getValue(ResultRow $values, $field = NULL) {
    * {@inheritdoc}
    */
   public function onDependencyRemoval(array $dependencies) {
+    // @fixme: This needs some serious documentation.
     $remove = FALSE;
     $current_dependencies = $this->calculateDependencies();
     foreach ($current_dependencies as $group => $dependency_list) {

Updated the issue summary to reflect the current direction of the fix and some points that @dawehner and I discussed on IRC.

Fixed/modified the existing tests to fit the new way of doing things. Also added a test for removing a dependency that causes the view to be disabled.

Added some extra comments based on #79. Probably need a follow-up issue to add to the @todo, so will look into that.

Still need to look at 'delete (don't just disable) views when the base entity gets removed' part of this. Or should that be a follow up too?

Added a follow up for better feedback #2832558: Add a 'disabled' section to config changes page when removing config and added that to the @todo in the patch.

Added the check for a removed base table for the View and a test for that.

Updated the IS a bit.

+++ b/core/modules/field_ui/src/Tests/FieldUIDeleteTest.php
@@ -91,13 +91,13 @@ function testDeleteField() {
 
     // Check the config dependencies of the first field.
     $this->drupalGet("$bundle_path2/fields/node.$type_name2.$field_name/delete");
-    $this->assertText(t('The listed configuration will be deleted.'));
+    $this->assertText(t('The listed configuration will be updated.'));
     $this->assertText(t('View'));
     $this->assertText('test_view_field_delete');
 
     $xml = $this->cssSelect('#edit-entity-deletes');
-    // Remove the wrapping HTML.
-    $this->assertIdentical(FALSE, strpos($xml[0]->asXml(), $field_label), 'The currently being deleted field is not shown in the entity deletions.');
+    // Test that nothing is scheduled for deletion.
+    $this->assertFalse(isset($xml[0]), 'The currently being deleted field is not shown in the entity deletions.');
 

I think we should actively check the state of the view afterwards, aka. disabled, with field removed. The other test coverage in the patch just deals with roles but not with entity fields

Added additional status check per #85.

As discussed on IRC with @dawehner, the View actually stays enabled if we can remove the field and this solves all the missing dependencies.

Been reading through the lines;
If we create the view using say default configuration in a module config/install, would that mean we would need to update or exported views file to have it include the module as dependency if we wanted it to deleted on uninstall?
If it's not deleted, an reinstall on the module will FAIL because the View is already in active configuration. Can someone confirm this behaviour?

@johndevman Couple of things:

• If you set the 'module' value to you module in the optional config you include in your module, the View will get deleted, same as with the current HEAD. Since disabling the View won't solve that dependency with the current fix proposal.
• If you don't include a dependency on your module in the optional config, the View won't get deleted, but again, this is the same as in the current HEAD.
• If a View already exists and you enable a module that includes that View, the install won't fail, it works as normal.

+++ b/core/modules/views/src/Entity/View.php
@@ -272,6 +273,11 @@ public function calculateDependencies() {
+    // For disabled views don't add any handler specific dependencies.
+    if (!$this->status()) {
+      return $this;
+    }

I don't think this is a valid approach. If disabled views don't have their dependencies then we are in the same position as disabled modules.

+++ b/core/modules/views/src/Entity/View.php
@@ -468,4 +474,96 @@ public function invalidateCaches() {
+    // Disable the View if we made no changes or the handlers were not able to
+    // remove the dependencies. This will cause all handler dependencies to be
+    // ignored on dependency calculation.
+    // @todo https://www.drupal.org/node/2832558 Give better feedback for
+    // disabled config.
+    if ($disable) {
+      $this->disable();
+      $arguments = [
+        '@id' => $this->id(),
+      ];
+      $this->getLogger()->warning("View '@id': View was disabled because its settings depend on removed dependencies.", $arguments);
+      $changed = TRUE;
+    }

I guess for now its okay to actually delete those views in this particular case. We at least have been able to solve the particular big problem of removed fields, in which case we don't have to disable it already as part of the patch. Any further work could be IMHO moved into a follow up. @Lendude do you agree with that?

@dawehner yes that is what I was thinking too. This would still fix the most annoying problem and would allow us to expand in follow ups what we can remove without deleting the whole View. Maybe set up a meta and open child issues per handler type and see how we want to tackle each case.

Maybe set up a meta and open child issues per handler type and see how we want to tackle each case.

Good idea!

Ok, so something like this then. Removed all the disabling stuff and updated the test to reflect the change.

Thank you @Lendude!

1. +++ b/core/modules/views/src/Plugin/DependentWithRemovalPluginInterface.php
   @@ -0,0 +1,31 @@
   +namespace Drupal\views\Plugin;
   ...
   +interface DependentWithRemovalPluginInterface {
   

   I think this should be in Drupal\Core\Plugin, it is generic. And it will help with #1764380: Merge PluginSettingsInterface into ConfigurableInterface, since \Drupal\Core\Field\PluginSettingsInterface::onDependencyRemoval() doesn't have a replacement yet

   Alternatively, please mark this @internal with an @todo to move it later, whichever is easier.

2. +++ b/core/modules/views/tests/src/Kernel/ViewsConfigDependenciesIntegrationTest.php
   @@ -69,7 +80,75 @@ public function testImage() {
   +    $display =& $view->getDisplay('default');
   

   Nit, should be $display = &$view...

@tim.plunkett++

Moved it to Drupal\Core\Plugin and fixed nits.

Thanks!

Missed a spot, I think it's safe to set back to RTBC.

+++ b/core/modules/field_ui/src/Tests/FieldUIDeleteTest.php
@@ -91,13 +92,17 @@ function testDeleteField() {
+    $this->assertFalse(isset($xml[0]), 'The currently being deleted field is not shown in the entity deletions.');

Weird English :)

"the field currently being deleted" is better

Just removing fields from the View could leave it non-functiona for the sitel. For example if it has contextual (or any filters really) on the field being deleted.

What happened to the solution in the issue summary to disable the view and show a message if fields are removed? That would at least prompt admins to review the configuration after this has happened.

Also is this definitely going to work if there's a relationship or similar using the field that's removed here? i.e. does the relationship get removed, and do the fields that depend on that relationship also get removed? Don't see this case in the test coverage.

@catch sorry, the issue summary is again out of date for the current attempt at fixing this. I updated the IS to add the current direction.

Some more specific answers to your questions

What happened to the solution in the issue summary to disable the view and show a message if fields are removed?

@alexpotts comment in #91 happend to it. If you have a suggestion on how to get back to that, I'm all ears, 'cause I really like that route.

Also is this definitely going to work if there's a relationship or similar using the field that's removed here?

Updated the issue summary to add my plan from #93 to open a follow-up meta with issues for all the different handler types and how to tackle them. Since you are absolutely correct that just removing them sounds like a bad idea, and different types would probably require different approaches.

#91 is pointing out a different problem to #104.

If we don't remove dependencies, and disable the views, then we have data integrity issues, 100% agreed we should not end up in that situation.

I'm suggesting if we really want to remove the dependencies, then we should also disable the view. This will then force the admin to review the view that's been altered, and either fix it or not.

@catch Ah! Ok, thanks for the clarification.

So, if I understand everything correctly the workflow would then be as follows:
- On delete, check if any handlers want to remove themselves (first phase: only field handlers)
- Remove any handlers from the View that want to be removed
- If any modification was done, disable the View
- If any dependencies still remain, delete the View

Sounds good to me. And would give a good first step on which we can build for the other handler types.

Ok so this is a patch using this steps in #108

Forget #110, removed the wrong stuff, this should be green.

I agree with the new logic. This results into maximum safety, while still making it possible to not totally break people's side.

core/modules/views/tests/src/Kernel/ViewsConfigDependenciesIntegrationTest.php:16
In an ideal world, this test would have a dedicated test handler which does stuff instead of relying on the roles filter / fieldable field. Still I don't think this should block the patch.

I was going to complain, then saw #2832558: Add a 'disabled' section to config changes page when removing config. #112 is definitely a UX improvement on its own.

I am able to cause this warning:

Warning: array_key_exists(): The first argument should be either a string or an integer in Drupal\views\Entity\View->onDependencyRemoval() (line 528 of /Users/cjm/Sites/drupal8x/core/modules/views/src/Entity/View.php)

...by deleting a content type field (the default Article content type image field).

Warning: array_key_exists(): The first argument should be either a string or an integer in Drupal\views\Entity\View->onDependencyRemoval() (line 528 of /Users/cjm/Sites/drupal8x/core/modules/views/src/Entity/View.php)

Interesting, is config_key, NULL in that case?

It is an instance of FieldStorageConfig.

Drupal\field\Entity\FieldStorageConfig::__set_state(array(
   'id' => 'node.field_image',
   'field_name' => 'field_image',
   'entity_type' => 'node',
   'type' => 'image',
   'module' => 'image',
   'settings' => 
  array (
    'uri_scheme' => 'public',
    'default_image' => 
    array (
      'uuid' => NULL,
      'alt' => '',
      'title' => '',
      'width' => NULL,
      'height' => NULL,
    ),
    'target_type' => 'file',
    'display_field' => false,
    'display_default' => false,
  ),
   'cardinality' => 1,
   'translatable' => true,
   'locked' => false,
   'persist_with_no_fields' => false,
   'custom_storage' => false,
   'indexes' => 
  array (
    'target_id' => 
    array (
      0 => 'target_id',
    ),
  ),
   'deleted' => false,
   'schema' => NULL,
   'propertyDefinitions' => NULL,
   'originalId' => 'node.field_image',
   'status' => true,
   'uuid' => '966c1728-10d3-4684-8fd0-1abaa1007eb6',
   'isSyncing' => false,
   'isUninstalling' => false,
   'langcode' => 'en',
   'third_party_settings' => 
  array (
  ),
   '_core' => 
  array (
    'default_config_hash' => 'SkXIPKZYiIMMtnBmfnxk58RYfbZ8cHSw5NZPY_JByME',
  ),
   'trustedData' => false,
   'entityTypeId' => 'field_storage_config',
   'enforceIsNew' => NULL,
   'typedData' => NULL,
   'cacheContexts' => 
  array (
  ),
   'cacheTags' => 
  array (
  ),
   'cacheMaxAge' => -1,
   '_serviceIds' => 
  array (
  ),
   'dependencies' => 
  array (
    'module' => 
    array (
      0 => 'file',
      1 => 'image',
      2 => 'node',
    ),
  ),
))

@cilefen yeah I can reproduce this (well sorta, I see the wrong data in xdebug anyway). Something else I saw while trying to reproduce is that the timing of the log message is still wrong. A log entry is made when you are shown the list of config that needs updating, not when you actually do the update (so if you cancel, it's still logged)

Ok, new stab at this.

Since the timing of logging a message is wrong we can lose the logic that causes #118, since that was only for determining if a message needed to be send.

I don't really see a way of logging a message were the timing is so that it's only logged when the disabling is actually done (so it's not done when you cancel your config change). Since the logging was a pretty weak effort at giving some feedback anyway, I'd opt for just leaving it out and moving forward with #2832558: Add a 'disabled' section to config changes page when removing config so we actually have some decent feedback.

Reroll for #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase

+++ b/core/modules/views/src/Entity/View.php
@@ -512,4 +513,61 @@ public function invalidateCaches() {
+    // Disable the View if we made changes.
+    // @todo https://www.drupal.org/node/2832558 Give better feedback for
+    // disabled config.
+    if ($changed) {
+      // Force a recalculation of the dependencies if we made changes.
+      $this->getExecutable()->current_display = NULL;
+      $this->calculateDependencies();
+      $this->disable();
+    }

Can we at least log that the view was disabled at all? I think that would be a valueable information.

@dawehner as far as I can tell there is no way to get the timing right for logging a message. \Drupal\Core\Config\ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval can get called with $dry_run TRUE or FALSE, but the onDependencyRemoval call doesn't get this context, so it has no idea if it's doing a dry run or not. So if we log a message, we might be logging it during a dry run, when the View never actually get disabled at all (like during building the confirmation form).

So the only options I see are: logging too often, or not at all. And 'too often' doesn't sound like an option really.

So if you have an idea about how we could do this, I'd be happy to implement it, but I think this will just need #2832558: Add a 'disabled' section to config changes page when removing config for better feedback then just the 'will be updated' one.

We can always work on the messages later but not deleting them is the important step.

Committed/pushed to 8.4.x, thanks! Tricky one this and good to see it green.

This could probably go into 8.3.x, but will get feedback from another committer first.

Spoke to xjm and we agreed this is good for 8.3.x too, so cherry picked. Thanks!

index 0000000..a123f8a
--- /dev/null

--- /dev/null
+++ b/core/lib/Drupal/Core/Plugin/DependentWithRemovalPluginInterface.php

+++ b/core/lib/Drupal/Core/Plugin/DependentWithRemovalPluginInterface.php
@@ -0,0 +1,31 @@

@@ -0,0 +1,31 @@
+<?php
+
+namespace Drupal\Core\Plugin;
+
+/**
+ * Provides an interface for a plugin that has dependencies that can be removed.
+ *
+ * @ingroup plugin_api
+ */
+interface DependentWithRemovalPluginInterface {
+

Spoke to @alexpott as well. This is in the global namespace, but nothing uses it apart from Views. This leaves open the possibility that something implements it thinking it'll be handled, when it won't.

Suggested moving to Views namespace, if we eventually figure out something generic, we could deprecate the Views interface when it moves to core.

reverted from both branches but should be a quick change for someone.

@tim.plunkett requested it be put in the global namespace in #97 because he had a use for it, but here it is, back in the Views namespace.

#131 is entirely fair.

+++ b/core/modules/views/src/Plugin/DependentWithRemovalPluginInterface.php
@@ -0,0 +1,31 @@
+ * @ingroup plugin_api

Not part of the plugin_api group anymore.

1. +++ b/core/modules/field_ui/src/Tests/FieldUIDeleteTest.php
   @@ -106,6 +107,11 @@ public function testDeleteField() {
   +    // Test that the View isn't deleted and has been disabled.
   +    $view = View::load('test_view_field_delete');
   +    $this->assertNotNull($view);
   +    $this->assertFalse($view->status());
   
   +++ b/core/modules/views/tests/src/Kernel/ViewsConfigDependenciesIntegrationTest.php
   @@ -69,7 +80,77 @@ public function testImage() {
   +    // Checks that the image field was removed from the View.
   +    $display = $view->getDisplay('default');
   +    $this->assertFalse(isset($display['display_options']['fields']['bar']));
   +
   +    // Checks that the view has been disabled.
   +    $this->assertFalse($view->status());
   

   Can we also test that the view no longer depends on the thing that has been deleted?

2. +++ b/core/modules/views/src/Entity/View.php
   @@ -512,4 +513,61 @@ public function invalidateCaches() {
   +              // Remove the handler and indicate we made changes.
   +              unset($this->display[$display_id]['display_options'][$handler_types[$handler_type]['plural']][$handler_id]);
   +              $changed = TRUE;
   

   Otherwise we don't have explicit coverage of this.

@alexpott thanks for the feedback!

So something like this?

@alexpott rightly pointed out that \Drupal\Tests\views\Kernel\ViewsConfigDependenciesIntegrationTest could really use this too, so here we go!

@vaplas yeah that's better but would also need a 'before' test, otherwise we just have a negative assertion (so if somebody changed the config name and it didn't get removed, the test would still pass).

So this adds the 'before' test.

The feedback from @alexpott got addressed, maybe its now time to get it in, finally :)

Unrelated fail https://www.drupal.org/pift-ci-job/646538

Opened #2868880: Random fails in ContextualFilterTest::testAddContextualFilterUI

Should have spotted this already but we need a change record. We've got a new interface and way for views plugins to react to dependency removal. Sorry.

Attempt at a CR: https://www.drupal.org/node/2871981

Thank you @Lendude!

Committed f7520a2 and pushed to 8.4.x. Thanks!

I think there is some scope for committing this to 8.3.x since the interface is an addition and added to a plugin - which by definition is not API. Will ping the other committers.

Thank you alex for getting this in, finally!

Thanks everybody for working on this!

Can we mention that in the release notes for 8.4?

@dawehner sure.

In discussion about backporting this patch with @xjm and @catch, @xjm raised valid concerns about backporting and committing this patch. These are:

• Does anything inform the user the view will be disabled? This was delayed to #2832558: Add a 'disabled' section to config changes page when removing config
• In some ways this a worse UX than before. We were telling the user that we were deleting the views and now we're just telling them we're updating them - but this change does something that might not be expected - it's disabling them. However a mitigation is that, before if you didn't read and understand the confirmation messages, the view was gone forever and now it is just disabled. So recovery is possible.
• It would be great to UX input into #2832558: Add a 'disabled' section to config changes page when removing config and #2773205: Come up with a design for highly destructive operations in confirm forms
• On backporting this to 8.3.x - the UX concerns make this more challenging. @xjm also has concerns about breaking the BC policy in adding new API surface area.

On reflection, I feel that #2832558: Add a 'disabled' section to config changes page when removing config should have been fixed before committing this fix and would have reverted this change but for the fact that the 8.4.x behaviour is probably better for users. However doing things this way around sets a bad precedent.

Adding to #157, splitting the usability issue and review for it out into a followup issue breaks the UX gate, especially in an issue that's specifically about fixing a usability problem.

I think the current usability bug (of disabling the view without warning and not giving even an attentive user information on how to fix it) is less bad than the previous bug (of telling a user that we will delete their view and not having them read it) because they can recover from the former but not the latter. So I don't necessarily think we should revert, but we also should be careful to not skip the usability gate in the future.

8.4 can't come soon enough. I just lost 3 hours work to this UI fail as I tidied up the experiments that got me to my destination. :-(

subscribing...

@tomremner you don't need to leave a comment to subscribe (and doing so creates noise for other participants) - instead use the "Follow" toggle link in the right panel.

The CR for this was never published; took care of that just now.

Is this not fixed? I just deleted a content type and was warned it would delete a view so I edited the view and removed the content type, went ahead and deleted it and the view has gone! It was hours of work and I've now discovered my host had dbase backups turned off! Shouldn't the view just have been disabled?

@rosbiffer That is not good. Please open a new bug issue at critical priority and reference the new issue in a comment here. We will be interested to see the steps to reproduce, especially if it can be derived from a clean install. But having a backup on which this data loss could be demonstrated is definitely useful.

Drupal 8 is a pile of trash, they completely ignored everything good about drupal 7.

This just happened to me. A big waste of my time working with 8 and will now need to start again applying changes and hope another bug doesn't completely wipe something.. The entire framework for Drupal 8 is destroying any reputation drupal has.

What version of Drupal 8? Also, please report it as suggested in comment #165.

Before deleting any content type first check if the content type is used in any view and make sure to Always show the master display and then check the master display.

To show the master display https://www.drupal.org/docs/8/core/modules/views/manage-display-settings-for-the-views-configuration-screen

I don't think this works when deleting fields programmatically.

https://drupal.stackexchange.com/questions/298957/how-to-display-configu...

#170 agree - apart from that (another issue then): I doubt that this is the best approach here. Even a warning, especially between all the other warnings about deleted configs etc - could get overseen easely. And building Views can be very time consuming and frustrating in case of being away forever. We should not remove Views views when an involved field get removed. The issue we came from was that Views views was somewhat broken and warning about a missing field. Which was fine for me and others I discussed about it. Actually even better than what we have atm.

Maybe we should separate the warning from the others in chain and add the infos from #168 could be a good compromise.

This is still happening in Drupal 10.1, at least for taxonomy term fields/views.

I must be missing something, but surely by design when you remove a field from a taxonomy/content type it should not then remove views associated with not just that field, but the entire term/content type? If so, is there some sort of recommended way to remove unused fields to preserve keeping any associated views in place?

In Drupal 9.5.11, after deleting a float field of a custom content type, the corresponding view having table format can still be edited, saved, enabled, disabled and previewed, but its path returns a "page not found" error.

And please log all the concerned views in recent log messages.