Problem/Motivation
When a module saves third-party settings on a view, uninstalling that module will delete the view (instead of just removing the third-party settings).
This exact problem is normally taken care of by \Drupal\Core\Config\Entity\ConfigEntityBase::onDependencyRemoval(). However, the Views entity class overrides the method but fails to call the parent (or fix the problem itself).
Steps to reproduce
- Write a small module that adds third-party settings to a view.
- Go to the “Uninstall” page and select your module.
- On the confirm form, you will see that the view will get deleted.
Proposed resolution
Unless there is a good reason not to do that, I would suggest just calling the parent method.
Remaining tasks
- Agree on the solution.
- Implement it.
User interface changes
None.
Introduced terminology
None.
API changes
None.
Data model changes
None.
Release notes snippet
N/A
Issue fork drupal-3496867
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3496867-views-third-party-setting-depend
changes, plain diff MR !10760
Comments
Comment #3
drunken monkeyMy trivial suggested solution is implemented in this MR.
Let’s see whether the pipelines succeed, otherwise we might already know the reason why this is currently not done.
Comment #4
smustgrave commentedThis has been seen in better exposed filters, Probably getting test coverage can help show the problem and if the approach works
Comment #5
nicxvan commentedIf after all this time this was all that was needed...
Comment #7
dcam commentedI added a test.
Comment #8
smustgrave commentedBummer doesn't actually solve the problem for better_exposed_filters but how does third_party_settings get added anyway?
Comment #9
dcam commentedYeah, I noticed that, but forgot to mention it last night. Since BEF doesn't use third party settings to modify views, this fix won't apply to that problem.
Third party settings are applied to config entities via the
setThirdPartySetting()function. I think most of the time you do that in a Hook or EventSubscriber.Comment #10
drunken monkeyThanks a lot for writing the test, looks great!
I just made a few minor changes, now I’d say this is RTBC.
Comment #11
smustgrave commentedNit but should change the wording of the last commit. Comes off like someone owns this so not sure read right
Comment #12
nicxvan commentedI added a suggestion for that comment.
Comment #13
dcam commentedI chose:
Comment #14
nicxvan commentedThat is better!
Comment #15
smustgrave commentedWorks for me. Probably good to self RTBC after
Comment #16
dcam commentedRight on.
Comment #17
berdirDon't want to hold this up, but I think a follow-up to change views to _never_ delete due to config dependencies but just disable the view. There are lots of similar issues open where views get deleted due to a role or node type dependency in a condition that will not be fixed by this, this specifically only fixes third party settings.
Comment #18
smustgrave commentedOpened #3520747: Don't delete view due to config dependencies being deleted
Comment #22
catchCommitted/pushed to 11.x, cherry-picked to 11.2.x and 11.1.x, thanks!
Comment #25
xjmNice fix! I feel like this has some old duplicates or related issues out there having to do with config dependencies. These issues are close to my heart. We might want to run issues like this past config maintainers before commit.
There was also an important-sounding followup @berdir suggested in #17 that never got filed?
Comment #26
xjmComment #27
catch@xjm I think that's the one smustgrave opened and linked to in #18?
Comment #28
dcam commentedYes, that's the follow-up. @smustgrave quoted @berdir from #17 in #3520747: Don't delete view due to config dependencies being deleted. I'm setting this back to Fixed.