Problem/Motivation

When deleting a content type field, the UI shows the view as a dependency under configuration deletions and deletes the view when you confirm the delete.
In D7, the view gets automatically updated. (sort of, see #15). Users apparently do not expect this behavior and end up accidentally deleting a whole view. Possible user experience problems with this:

  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.

Proposed resolution

The behavior itself should not be changed, but we can improve the user experience problem:

  • Make deletions more obvious than automated dependency removals, possibly with a confirm form.
  • Provide stronger/clearer warnings in the UI, e.g."Warning: You are about to delete an entire view".
  • Provide instructions on what to do instead,e.g. "To remove this field without deleting these items, first edit the items so they no longer use this field."

Alternate resolution from #51 and onward:

The behaviour should be changed. By default, the View should just be disabled. An attempt can be made to update it by removing handlers that trigger the dependency, if this still leaves dependencies, disable the View so the user can manually update the View before enabling it again. This will prevent any unexpected data loss.
Things to look at (possibly in follow-ups):

Alternate resolution from #91 and onward:

The behaviour should be changed but only for field handlers. The proposed solution would be to remove any field handlers that have a dependency on any configuration being removed. If that solves all the dependencies then the View can just be updated and saved without this field. Dependencies because of other handlers (filters, relationships et al.) will still cause the whole View to be deleted. The removal of a field handler should have little impact on the overall working of the View, so the normal notice that it will be updated should suffice for that.
The deletion of a View because of a field handler is probably the most commonly occurring situation and the one where removing the handler should not have any dramatic effects.
Follow ups will be opened to see how we can handle the other types of handlers on a handler-type basis.

Remaining tasks

Decide on a proposed resolution. Needs designs and usability review for them once proposed.

User interface changes

TBD

Original report by [username]

I created a 'date' field on a content type and a view to for this content type.
Then i deleted the 'date' field from the content type and the whole view got deleted also without any warnings.

Files: 

Comments

dawehner’s picture

Version: 8.1.x-dev » 8.0.x-dev
Priority: Normal » Major

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.

Berdir’s picture

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?

swirt’s picture

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.

no_angel’s picture

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

swirt’s picture

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.

Berdir’s picture

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.

swirt’s picture

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.

geertvd’s picture

Status: Active » Needs review
Issue tags: +drupaldevdays
FileSize
1.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,427 pass(es). View

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.

swirt’s picture

Thanks geertvd I will review this later today.

geertvd’s picture

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.

geertvd’s picture

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()

geertvd’s picture

FileSize
1.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,430 pass(es). View
1.67 KB

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.

geertvd’s picture

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.

Jaesin’s picture

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.

geertvd’s picture

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.

Jaesin’s picture

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.

geertvd’s picture

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.

Berdir’s picture

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.

Jaesin’s picture

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.

jhedstrom’s picture

Assigned: Unassigned » jhedstrom

Working on a test.

jhedstrom’s picture

Assigned: jhedstrom » Unassigned

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

Berdir’s picture

#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 :)

no_angel’s picture

Status: Needs review » Needs work
Issue tags: -drupaldevdays +Needs summary
xjm’s picture

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.

xjm’s picture

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.

xjm’s picture

tim.plunkett’s picture

I agree with #25.

rakesh.gectcr’s picture

Assigned: Unassigned » rakesh.gectcr
Issue tags: +drupalconasia2016
monojit256’s picture

Assigned: rakesh.gectcr » monojit256
monojit256’s picture

Issue tags: -drupalconasia2016 ++drupalconasia2016
lokapujya’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
lokapujya’s picture

Issue summary: View changes
xjm’s picture

Issue tags: -+drupalconasia2016 +drupalconasia2016

(Fixing the tag.)

lokapujya’s picture

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.

mediapal’s picture

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.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

caspervoogt’s picture

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).

Olafski’s picture

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?

caspervoogt’s picture

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

dawehner’s picture

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

sonfd’s picture

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?)

xjm’s picture

Title: When deleting a content type field the related View also is deleted » When deleting a content type field, users do not realize the related View also is deleted
Version: 8.1.x-dev » 8.2.x-dev
Issue summary: View changes
Issue tags: +Usability, +Triaged D8 major
FileSize
52.61 KB

@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.

dawehner’s picture

Assigned: monojit256 » Unassigned

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.

Olafski’s picture

@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)?

xjm’s picture

Related issue for a similar destructive confirm form.

xjm’s picture

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.

dawehner’s picture

#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,

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Jeff Cardwell’s picture

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.

jhedstrom’s picture

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?

aklump’s picture

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.

dawehner’s picture

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

cilefen’s picture

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.

vaplas’s picture

The pain from this problem can not be measured. The administrator wants to improve the structure and the site transformed into a pumpkin. Nobody reads the warning. But all hate me for this effect (and I hate all back, of course). Even subconsciously I am trying to use only displays format for style (even when it is very inconvenient). IMHO, a Fatal Error when deleting field is a thousand times better than plain warning description.

typologist’s picture

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 :(

vaplas’s picture

FileSize
1.09 KB

Just leave it here. Module disabling "Submit button" of delete form until all dependent views not corrected (+ edit link for each views). It works only for next form: 'field_config_delete_form', 'user_role_delete_form', 'taxonomy_vocabulary_confirm_delete'.

Duplicate module code, if anyone is interested:

function views_keeper_form_alter(&$form, \Drupal\Core\Form\FormStateInterface $form_state, $form_id) {
    if (in_array($form_id, ['field_config_delete_form', 'user_role_delete_form', 'taxonomy_vocabulary_confirm_delete'])) {
        if(isset($form['entity_deletes']) && isset($form['entity_deletes']['view'])) {
            $form['actions']['submit']['#disabled'] = TRUE;
            foreach($form['entity_deletes']['view']['#items'] as $view_id => $view_title) {
                $link = \Drupal\Core\Link::createFromRoute(
                    t('Edit'),
                    'entity.view.edit_form',
                    ['view' => $view_id],
                    ['attributes' => ['target' => '_blank']]
                )->toString();
                $value = array(
                    '#markup' => "$view_title - $link",
                    '#allowed_tags' => ['a'],
                );
                $form['entity_deletes']['view']['#items'][$view_id] = $value;
            }
        }
    }
}
sheldonkemper’s picture

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.

cilefen’s picture

#51 and on are leaning the other way.

dawehner’s picture

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
alexpott’s picture

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.

dawehner’s picture

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

Berdir’s picture

That could break things like install order or optional config?

caspervoogt’s picture

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.

dawehner’s picture

@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.

tim.plunkett’s picture

+1 for #59/51

Olafski’s picture

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.

sheldonkemper’s picture

+1 #59

rakesh.gectcr’s picture

So lets go with the approach @dawehner suggested #59

ABaier’s picture

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.

jonathanshaw’s picture

#59 +1

vaplas’s picture

Still there is a moment when the relationship is left in the default settings, although all of the display override fields and not use it.

Example:

  1. display 1: uses image field with Blazy formatter.
  2. display 2 (override fields): uses plain image field.
  3. delete display1 and save
  4. uninstall Blazy, show message about View will delete

Because dependence saved in default settings and need create display with default fields and reset Blazy. Now this may seem obvious, but can someone come in handy.

vaplas’s picture

Or create issue about clearing default settings (fields, filter, style...) if all of the displays are overridden it?

Andrej Galuf’s picture

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.

bdichgans’s picture

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

dawehner’s picture

Assigned: Unassigned » dawehner

I'm working on that atm.

dawehner’s picture

Assigned: dawehner » Unassigned
FileSize
3.09 KB

Here is a start of that.

vaplas’s picture

@dawehner thank you very match. Amazing start!
My patch has 4 changes:

  1. DependentWithRemovalPluginInterface for EntityField (but maybe we can use more short check id?)
  2. New group 'deactivations' for DeleteForm (but maybe just use update group?)
  3. Disabling in ConfigManager (but maybe it not necessary) and ConfigEntityBase. And code:
    $dependent_entity->disable(); // not necessary, but just to be sure
    $dependent_entity->save();
Lendude’s picture

Status: Needs work » Needs review
FileSize
4.72 KB
6.78 KB

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

dawehner’s picture

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) {

Status: Needs review » Needs work

The last submitted patch, 78: 2468045-78.patch, failed testing.

Lendude’s picture

Issue summary: View changes

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

Lendude’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
5.69 KB
11.59 KB

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?

Lendude’s picture

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.

Lendude’s picture

Issue summary: View changes

Updated the IS a bit.

dawehner’s picture

+++ 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

Lendude’s picture

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.

vaplas’s picture

Status: Needs review » Reviewed & tested by the community

the View actually stays enabled if we can remove the field and this solves all the missing dependencies

If the base table for the View is provided by a module being removed, we delete the View because this is not something that can be fixed manually.

Great ideas! All works looks very professional! Many thanks! We have issue about better feedback (#2832558: Add a 'disabled' section to config changes page when removing config), and it requires this patch. Let's will get rid of the irritating behavior!)

The last submitted patch, 77: 2468045-77.patch, failed testing.

johndevman’s picture

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?

Lendude’s picture

@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.
alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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.

dawehner’s picture

+++ 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?

Lendude’s picture

Status: Needs review » Needs work

@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.

dawehner’s picture

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

Good idea!

Lendude’s picture

Status: Needs work » Needs review
FileSize
4.16 KB
11.56 KB

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @Lendude!

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
  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 ConfigurablePluginInterface, 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...

Lendude’s picture

Status: Needs work » Needs review
FileSize
4.12 KB
11.52 KB

@tim.plunkett++

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

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 98: 2468045-98.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
11.52 KB
560 bytes

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

almaudoh’s picture

+++ 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 :)

jonathanshaw’s picture

"the field currently being deleted" is better

catch’s picture

Status: Reviewed & tested by the community » Needs review

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.

Lendude’s picture

Issue summary: View changes

@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.

catch’s picture

#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.

Lendude’s picture

@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.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Lendude’s picture

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

Status: Needs review » Needs work

The last submitted patch, 110: 2468045-110.patch, failed testing.

Lendude’s picture

Status: Needs work » Needs review
FileSize
1.87 KB
12.82 KB

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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.

cilefen’s picture

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).

cilefen’s picture

Issue tags: +SprintWeekend2017
dawehner’s picture

Status: Reviewed & tested by the community » Needs work

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?

cilefen’s picture

It is an instance of FieldStorageConfig.

cilefen’s picture

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',
    ),
  ),
))
Lendude’s picture

@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)

_dcre_’s picture

Lendude’s picture

Status: Needs work » Needs review
FileSize
1.78 KB
11.82 KB

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.

vaplas’s picture

+++ b/core/modules/views/src/Entity/View.php
@@ -558,45 +558,16 @@
-      // If any of the dependencies still exist, the View will be deleted so
-      // there is no need to disable the View or log a message.

I'll miss about it :( but commit of the issue still bring a lot happiness!

dawehner’s picture

+++ 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.

Lendude’s picture

@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.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

  • catch committed 917ed83 on 8.4.x
    Issue #2468045 by Lendude, geertvd, vaplas, tim.plunkett, dawehner, xjm...
catch’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

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.

catch’s picture

Status: Patch (to be ported) » Fixed

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

  • catch committed 8e87130 on 8.4.x
    Revert "Issue #2468045 by Lendude, geertvd, vaplas, tim.plunkett,...
catch’s picture

Status: Fixed » Needs work
Issue tags: +Novice
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.

Lendude’s picture

Status: Needs work » Needs review
FileSize
3.55 KB
11.86 KB

@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.

tim.plunkett’s picture

#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.

vaplas’s picture

Extremely pleasant news!

tim.plunkett’s picture

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

Status: Reviewed & tested by the community » Needs work
  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.

Lendude’s picture

Status: Needs work » Needs review
FileSize
655 bytes
12.11 KB

@alexpott thanks for the feedback!

So something like this?

Lendude’s picture

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

vaplas’s picture

Maybe in testDeleteField() we need something like in testImage()? Because before delete second field dependency 'field.storage.node.field_test' and test coverage it:

    $this->assertText('test_view_field_delete');
...
    // Test that the View isn't deleted and has been disabled.
    $view = View::load('test_view_field_delete');    
    $dependencies = $view->getDependencies() + ['config' => []];
    $this->assertFalse(in_array("field.storage.node.$field_name", $dependencies['config']));
vaplas’s picture

This is what I meant in #139.

Lendude’s picture

@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.

vaplas’s picture