Add content & config entity dependencies to views

I'd argue this is critical, because it's essential for having efficient cache invalidation of views.

It's essential that we can figure out those config entity dependencies as well

I'm sure we have talked about this numerous times, but I will say it again; This only helps us for entities that use config entities for their bundles, which all will not. So as mentioned before, relying on config entity dependencies from types is not an option. So just using node as an example is not the full problem space. Unless I miss something major here?

So based on that, I would say not critical, unless I am wrong of course, then yes.

Good point! I'd forgotten about that. Except that we now can have content entity dependencies also: https://www.drupal.org/node/2364725. Hence this can address the full problem space, I think?

You can still have entities that have multiple bundles defined in code - the equivalent to https://api.drupal.org/api/drupal/modules%21node%21node.api.php/function...

If we don't track dependencies for those, then they can disappear when the module is disabled, which is what resulted in #1017672: D6 to D7 update process permanently deletes comment bodies and other data, and throws fatal SQL errors during the 6-7 upgrade path.

Only the module defining the entity type is able to determine that though.

damiankloip (or other VDC folks), if you can provide some high-level pointers, I'll work on a patch for this.

Just adding a related issue

And another related issue.

Actually, I think this is more of a soft blocker.

Here's an initial patch that takes care of the Page display plugin's optional menu link and the Bundle filter plugin. When added to the Frontpage view, that view's dependencies look like this:

It'd be great if somebody could confirm I'm on the right track or not.

iirc, calculateDependencies() takes care of the provider, that's why we don't have it here :)

Thank you for getting this started.

One thing we certainly have to look at is the implementation for the fieldapi field handler, called Field.php (do you need even more fields? FIELDS)
That handler would add both the field config entities, as well as

1. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
@@ -388,7 +388,14 @@ public static function preRenderFlattenData($form) {
    * {@inheritdoc}
    */
   public function calculateDependencies() {
-    return [];
+    $dependencies = [];
+
+    $provider = $this->getProvider();
+    if ($provider !== 'views') {
+      $dependencies['module'][] = $provider;
+    }
+
+    return $dependencies;
   }


   If we would drop the $provider != 'views' line, we could implement that functionality as part of the \Drupal\Core\Plugin\PluginBase ... I don't have problems with adding 'views' to be honest.

2. +++ b/core/modules/views/src/Plugin/views/display/Page.php
@@ -425,4 +425,18 @@ public function getPagerText() {
+  public function calculateDependencies() {
+    $dependencies = parent::calculateDependencies();
+
+    $menu = $this->getOption('menu');
+    if ($menu['type'] === 'normal') {
+      $dependencies['config'][] = 'menu.' . $menu['menu_name'];
+    }
+
+    return $dependencies;
+  }
+

   Note: We do allow to configure to use contextual links for views ... so in case we do, we should add a dependency to contextual module, don't we?

3. +++ b/core/modules/views/src/Plugin/views/filter/Bundle.php
@@ -73,4 +74,26 @@ public function query() {
+    $bundle_entity_storage = \Drupal::entityManager()->getStorage($bundle_entity_type);

   Let's not be lazy here ...

4. +++ b/core/modules/views/src/Plugin/views/filter/Bundle.php
@@ -73,4 +74,26 @@ public function query() {
+      if ($bundle_entity instanceof ConfigEntityInterface) {
+        $dependencies['config'][] = $this->entityTypeId . '.' . $bundle_entity_type . '.' . $bundle;
+      }
+      else {
+        $dependencies['content'][] = $bundle_entity_type . ':' . $bundle_entity->uuid();
+      }

   As alex said, getConfigDependencyName() is the way to go.

Adding dependencies for the displays I think should be like this.

EDIT: Patch does do this, so I think we can still just remove the changes to PluginBase here? - Sorry, didn;t mean to get that tested!

Added a few additional dependencies. Its pretty clear that some of the code is kinda redundant.

Note: in order to add dependencies for the corresponding entity fields, we do need #2349553: Store entity field information in the views data

#17: does this issue need to be blocked on #2349553: Store entity field information in the views data then?

Mh k, this should be it to pass again.
I went through all the handlers for now, and hope I found all instances, which are possible to detect at the moment.

Drupal should install again now. #17 introduced view mode config dependencies, but used view_display rather than view_mode, plus it used wrong view mode IDs (they need to be prefixed with the entity type ID). This was introducing so much duplication in the various RSS row plugins, that I added a new RssPluginBase abstract base class.

EDIT: dawehner and I worked on this in parallel. He wrote a different solution (no base class, but a trait), but it's still broken because the IDs it uses are still wrong.

#21 shows how apparently we still have "view modes" and "view displays". I thought we had gotten rid of "view modes". Apparently, view modes exist only to be listed in the view modes/displays UI, essentially to dictate across all bundles of an entity type which view displays are available. Otherwise generic code like Views would not be able to list all nodes in a certain view display, because it'd be possible for bundle A to support view display X, but for bundle B to not support view display X.

This leads to an important question: how do we make sure that whenever a view display is modified (e.g. field F is shown before field G instead of vice versa), that the cached view is invalidated? We thought (or at least I did) we needed to depend on the corresponding view display config entities to make that happen.
But actually, we don't: rendering a view means rendering the listed entities in the configured view mode, which means the corresponding view display will be used for every listed entity, and rendering of an entity using a certain view display means that the cache tag associated with that view display will be set on the rendered entity.
This means only the render cache (Views output cache) will be invalidated. But this is correct: the Views results cache is unaffected by view display changes.

Working on fixing the test failures.

Fixed some failures, but not yet all.

In doing so, found a pretty big bug in the Page display: if it had a menu link, it would not be excluded from the parent link selection. :)

Here is a quick review.

1. +++ b/core/modules/node/src/Plugin/views/row/Rss.php
@@ -28,7 +27,7 @@
-class Rss extends RowPluginBase {
+class Rss extends RssPluginBase {

   I like that, at least on an abstract level!

2. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
@@ -374,4 +374,17 @@ public function getCacheContexts() {
+    $role_storage = \Drupal::entityManager()->getStorage('taxonomy_vocabulary');
+    $vocabulary = $role_storage->load($this->options['vid']);

   let's fix the variable name.

3. +++ b/core/modules/taxonomy/src/Plugin/views/relationship/NodeTermData.php
@@ -103,4 +103,20 @@ public function query() {
+    $role_storage = \Drupal::entityManager()->getStorage('taxonomy_vocabulary');
+    foreach (array_keys($this->options['vid']) as $vocabulary_id) {

   horrible variable name here as well.

4. +++ b/core/modules/views/src/Plugin/views/display/Page.php
@@ -202,7 +202,8 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-          $form['menu']['parent'] = \Drupal::service('menu.parent_form_selector')->parentSelectElement($menu_parent);
+          $menu_link = 'views_view:views.' . $form_state->get('view')->id() . '.' . $form_state->get('display_id');
+          $form['menu']['parent'] = \Drupal::service('menu.parent_form_selector')->parentSelectElement($menu_parent, $menu_link);
           $form['menu']['parent'] += array(

   Ha, but yeah we at least have the API prepared for it. Can we either move this to a different issue or add some dedicated test coverage?

5. +++ b/core/modules/views/src/Plugin/views/row/EntityRow.php
   @@ -213,4 +213,20 @@ public function render($row) {
   +      ->getStorage('entity_view_display')
   +      ->load($this->entityTypeId . '.' . $this->options['view_mode']);

   +++ b/core/modules/views/src/Plugin/views/row/RssPluginBase.php
   @@ -0,0 +1,77 @@
   +   */
   +  public function calculateDependencies() {
   +    $dependencies = parent::calculateDependencies();
   +

   So we want to have entity_display_mode all the time?

6. +++ b/core/modules/views/src/Plugin/views/row/RssPluginBase.php
@@ -0,0 +1,77 @@
+    $view_modes = \Drupal::entityManager()->getViewModes('node');

   Let's use the entity type here ... 'node' is wrong here.

7. +++ b/core/modules/views/src/Plugin/views/row/RssPluginBase.php
@@ -0,0 +1,77 @@
+    $view_mode = \Drupal::entityManager()
+      ->getStorage('entity_view_mode')
+      ->load($this->entityTypeId . '.' . $this->options['view_mode']);
+    if ($view_mode) {
+      $dependencies['config'][] = $view_mode->getConfigDependencyName();
+    }
+

   The reason, why I went with the trait, is that #2322949: Get generic entity link fields from entity_view module. will need that as well, and there we do have a field plugin. Believe me, sometimes even I, do know what I'm doing ...

1. Most of the remaining failures occur because doing *any* test with Views (even KernelTestBase tests) require the Views module to be installed, which requires its default config to be installed, which includes the default Views config of other modules. This includes amongst others the Node module's frontpage and glossary views. The glossary view requires the "main" menu to exist.
   In other words: installing Views on a site that has the Node module already installed requires the "main" menu to exist.
2. Similarly, for e.g. AreaViewTest, we have a view that depends on another view, but they're always imported in alphabetical order rather than taking dependencies into account. This is because typically ViewTestData::createTestViews(get_class($this), array('views_test_config')); is used to import/install tested view config entities, which installs the views under test in *alphabetical* order, rather than in the specified order.

Both of the above are caused by using ViewTestData::createTestViews() instead of ConfigInstaller. That custom config install system does *not* take dependencies into account and hence can install view config entities in the wrong order (2), or not install dependencies on other config (1). Both amount to the same problem: dependencies are not handled.

But… how could that have been the case, if Views so far never specified config dependencies?

Hence this reroll introduces temporary work-arounds: it "improves" the current system by installing system module config first, instead of last, and by allowing protected $testViews = ['a', 'b'] to specify the test views to install in the order they should be installed.

Once we're satisfied with the config/content dependencies being added by all plugins, we should re-export all views and get rid of ViewTestData::createTestViews() altogether.

Finally, for e.g. CacheTagTest, we have a view that is filtering by bundle (node type), but the bundles are created *after* the view config entity is imported.

Addressing #28

1. :)
2. Done.
3. Done.
4. I'm afraid we can't do it elsewhere, because without this, the asserted options would be wrong. We could of course fix it in a child issue.
5. Heh, yes, fixed. I wasn't testing this one, so it slipped through the cracks.
6. Good catch, fixed.
7. I'm totally fine with a trait! I'm sorry I didn't make that clearer in #21. I have no opinion about this at all, I'll go with whatever you want. :) I just happened to have cross-posted a different approach with you.
   (Not yet implemented here though, I have more changes locally. I will do it in one of the next rerolls unless you beat me to it.)

Once we're satisfied with the config/content dependencies being added by all plugins, we should re-export all views and get rid of ViewTestData::createTestViews() altogether.

I'm fine with doing that, as long we get rid of EntityManager::getDefinition()

From an IRC discussion with damiankloip and dawehner: We should keep ViewsTestData::createTestViews(), but make it apply the same dependency-handling logic as ConfigInstaller::createConfiguration() has. Otherwise we'd end up installing every test view for any test that only needs a single test view, plus all their dependencies. That would slow down tests quite a bit.

So, as a next step, I looked at the failures in FilterEntityBundleTest. They show the same problem that CacheTagTest showed:

a view that is filtering by bundle (node type), but the bundles are created *after* the view config entity is imported.

For CacheTagTest, that filtering was actually unnecessary, so I was able to "fix" the test by removing that filtering. In this case, there's no easy way out like that.

The problem here goes much, much deeper:

The parent::setUp() calls ViewsTestData::createTestViews(), which imports the test views. But then the bundles don't exist yet… So we must ensure the bundles *do* exist already when we create the test views.

You'd think that changing it to this:

solves the problem. But that doesn't work, unfortunately. Because drupalCreateContentType() needs WebTestBase::setUp() to already have been executed.

So now we've got a chicken and egg situation. And no clean way to solve it.

Any ideas?

I discussed the question in #35 with dawehner, damiankloip & Alex Pott on IRC last night: add a helper method invoked by ViewsTestData::createTestViews() that is called before the test views are actually imported. But turns out that possibility already exists: ViewTestBase::setUp() allows a FALSE parameter to be passed in, which then skips the importing of the test views. Which allows us to do:

<?php
parent::setUp(FALSE);

// Create dependencies.
//…

// Manually trigger importing of test views.
ViewTestData::createTestViews(get_class($this), array('views_test_config'));
?>

So I did just that :)

This fixes both FilterEntityBundleTest + DisplayPathTest.

All fixed except one test (2 failures, 2 exceptions).

Let's just fix it.

Let's add a proper tag.

Done.

#40 fixed a bug in a test View, which then caused a corresponding assertion to fail. Now fixed.

And clean-up, of both loose ends in the patch and the IS.

There are still quite a few places where we need to use the new method :)

Otherwise, the patch looks good to me.

Done.

Forgot a bit.

Awesome! :)

Discussed with Alex Pott how to write test coverage for the many new calculateDependencies() methods. The problem with testing these in isolation (i.e. as unit tests) is extremely laborious: it requires mocking an insane amount of dependencies, because Views depends reaches out to almost every other subsystem.

Therefore, we decided it would be okay for this test coverage to be implemented wherever it would be easiest to add, just like was done for #2267453: Views plugins do not store additional dependencies. The test coverage her doesn't have to be perfect. We basically just want to make sure we don't easily regress here.

Added tests for:

1. core/modules/field/src/Plugin/views/field/Field
2. core/modules/views/src/Plugin/views/row/RssPluginBase — tested via the frontpage view, because it's the only view in core that lists multiple view modes
3. core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid — here I discovered that we should also have added dependencies on any referenced taxonomy terms, yay for test coverage! :)
4. core/modules/taxonomy/src/Plugin/views/relationship/NodeTermData
5. core/modules/user/src/Plugin/views/access/Role
6. core/modules/views/src/Plugin/views/area/View
7. NOT: core/modules/views/src/Plugin/views/argument_validator/Entity — has zero views using it, hence zero test coverage, hence I did not add test coverage for this one
8. core/modules/views/src/Plugin/views/display/Page
9. core/modules/views/src/Plugin/views/filter/Bundle
10. NOT: core/modules/views/src/Plugin/views/row/EntityRow — has zero views using it, hence zero test coverage, hence I did not add test coverage for this one

I think I found a config schema problem with NodeTermData. The schema looks like this:

The sole example in core is in a test view, and it looks like this:

Two NodeTermData relationships, but note that under the vids key, we don't find a sequence, but a mapping. And, worse yet, in the tests, we modify the value of that mapping, so this was definitely intentional at some point:

Fixed all that, by:

1. implementing ::submitOptionsForm(), to normalize the data
2. fixing the test to actually configure a sequence instead of a mapping
3. fixing the calculateDependencies() method now that the data actually adheres to the schema

Whew. This took many, many hours instead of the 30 minutes or so I expected…

@Wim: that is actually a valid sequence. There is no requirement for a sequence to be numbered. The only difference between a sequence and a mapping is for a mapping you exactly know all the keys that there gonna be. For a sequence you don't know the exact keys. That's it. We could have named it better maybe. So the cited config snippet is valid according to the cited config schema. Examples of string keyed sequences include things keyed by plugin id for example, such as #2391925-5: Ensure page_manager config schema is valid where display variants are a sequence even though it is string indexed. But there can be any number of them and we don't know the keys of them.

Wow, that is tremendously confusing then :(

That last fail was due to something similar.

Should be green now, and needs review.

LOL.

So the reason for the failure in #58, fixed in #60, is that the config schema dictates that value should contain a sequence of strings, even though in the case of taxonomy term filter, we really are dealing with integers (taxonomy term IDs). So we should adjust the schema. Opened a follow-up for that, since it's out of scope here: #2392263: Views taxonomy term filter config schema should contain a sequence of integers, not strings.

1. +++ b/core/modules/comment/src/Plugin/views/row/Rss.php
@@ -62,18 +47,10 @@ public function preRender($result) {
+  public function buildOptionsForm_summary_options() {
+    $options = parent::buildOptionsForm_summary_options();
     $options['title'] = $this->t('Title only');
     $options['default'] = $this->t('Use site default RSS settings');

   Wow, I did forgot about it, totally!

2. +++ b/core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_taxonomy_node_term_data.yml
@@ -57,16 +57,14 @@ display:
           id: term_node_tid
           admin_label: 'Term #1'
           table: node
-          vids:
-            tags: ''
+          vids: {}
           plugin_id: node_term_data
         term_node_tid_1:
           field: term_node_tid
           id: term_node_tid_1
           admin_label: 'Term #2'
           table: node
-          vids:
-            tags: ''
+          vids: {}
           plugin_id: node_term_data
       sorts:
         nid:

   So one thing we don't do at all yet is to update the dependencies of the existing test views ... I would assume there are some we have to update, especially for things like view modes?

So one thing we don't do at all yet is to update the dependencies of the existing test views ... I would assume there are some we have to update, especially for things like view modes?

We discussed this two days ago, and IIRC Alex Pott said we shouldn't update all existing (test) views in this issue, because that'd cause conflicts in many other patches.

I would agree for test views, but I don't agree for actual used default views.
On top of that I also don't see any follow up for that yet.

Talked to Alex Pott again and he re-confirmed that we indeed want to do the re-exporting of views in a follow-up issue. Opened that: #2392601: Re-export views now that all views now that they list content & config dependencies.

(I honestly don't care, so if you disagree, please talk to Alex.)

Other than the re-exporting, any other remarks? Do the tests I added in #54—#60 look good?

The tests seem good.

Some nits:

1. +++ b/core/modules/aggregator/src/Plugin/views/row/Rss.php
@@ -22,7 +21,7 @@
-class Rss extends RowPluginBase {
+class Rss extends RssPluginBase {

   Nice refactor.

2. +++ b/core/modules/node/src/Tests/Views/NodeLanguageTest.php
   @@ -41,11 +42,12 @@ class NodeLanguageTest extends NodeTestBase {
   -    parent::setUp();
   +    parent::setUp(FALSE);

   +++ b/core/modules/node/src/Tests/Views/NodeTestBase.php
   @@ -22,10 +22,12 @@
   +  protected function setUp($import_test_views = TRUE) {
   ...
   +    if ($import_test_views) {

   Neat trick.

3. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyIndexTidUiTest.php
@@ -96,6 +103,22 @@ public function testFilterUI() {
+      ]

   Missing trailing comma.

4. +++ b/core/modules/views/src/Plugin/views/area/View.php
@@ -151,4 +151,19 @@ public function isEmpty() {
+    list($view_id, $display_id) = explode(':', $this->options['view_to_insert']);

   We don't use $display_id here, no reason to have it in the list(). Also, might as well add , 2 to the end of that explode()

5. +++ b/core/modules/views/src/Tests/Entity/FilterEntityBundleTest.php
@@ -70,6 +73,19 @@ protected function setUp() {
+        'node'
+      ]

   Trailing commas.

Thanks for the review! Fixed all nits.

Its a little bit sad that we change the behaviour in one of many many many cases, but well, this itself is a step forward.

Beside from that its a great improvement.

Et voilà!

Gah.

So one thing I am wondering, this is removed to just get the tests to pass really?!And this was just something that was not removed when we ripped out the bundle cache tag logic from views (my fault!).

So what happens when we then need this back? Also, the creation of bundles is still in the test too.

1. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
@@ -405,6 +405,17 @@ public function getTypedData();
+  public function getConfigDependencyKey();

   We're also adding yet another method to entities :/

2. +++ b/core/modules/views/src/Plugin/views/display/Page.php
@@ -425,4 +475,19 @@ public function getPagerText() {
+    if ($menu['type'] === 'normal') {

   Do we need a @todo here when we fully support all menu things?

3. +++ b/core/modules/views/src/Plugin/views/filter/Bundle.php
@@ -73,4 +113,21 @@ public function query() {
+    $bundle_entity_storage = \Drupal::entityManager()->getStorage($bundle_entity_type);

   Shouldn't that be using the injected manager now?

Also, not sure RTBC'ing your own patch after 20kb of changes is the done thing? :)

Also, not sure RTBC'ing your own patch after 20kb of changes is the done thing? :)

Agreed

Do we need a @todo here when we fully support all menu things?

Well ... technically I can't think that we need another one. local actions/tasks and contextual links can't be added to a menu itself.

So all those menu dependencies are satisfied then? Why do we need to check the type?

So all those menu dependencies are satisfied then? Why do we need to check the type?

Well ... for those other types we don't use menu entities ... we just want to export the config dependency, in case we have to.

Sure, that makes sense. Can you tell I have not really looked at ANY menu changes ;)

#73: The alternative is to fix the test to create the bundles early enough, *before* the view is imported. We could do that, but these filters on the view are simply unnecessary for doing the tests that we care about, so it's more logical to just remove those filters. And finally, the reason we don't remove the bundles, is that they're fine to keep. We need to create *some* bundle anyway, and the fact that this happens to create two bundles doesn't hurt.
(So: the filters *do* hurt, the multiple bundles instead of a single bundle *don't* hurt.)

#74/#75: the only reason I RTBC'd after 20K of changes, is that it's 20K of boilerplate AKA super trivial changes. Otherwise I wouldn't even have considered it. But you're right, I shouldn't have. Sorry. Won't happen again.

#74.1: yep, but that already was there when it was RTBC'd in #68. It's a sad necessity. Alex Pott suggested/approved it.
#74.2: answered by dawehner in #75/#77/#78/#79.
#74.3: good catch, fixed!

#76: thanks :)

Did another review here ...

1. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -28,6 +32,54 @@ class TaxonomyIndexTid extends ManyToOne {
   +    $this->is_handler = TRUE;

   +++ b/core/modules/taxonomy/src/Plugin/views/relationship/NodeTermData.php
   @@ -22,6 +24,43 @@
   +    $this->is_handler = TRUE;

   +++ b/core/modules/user/src/Plugin/views/access/Role.php
   @@ -32,6 +34,43 @@ class Role extends AccessPluginBase {
   +    $this->is_handler = TRUE;

   it is not obvious why we need this here? C&p from HandlerBase

2. +++ b/core/modules/views/src/Plugin/views/display/Page.php
@@ -425,4 +475,19 @@ public function getPagerText() {
+      $menu_entity = \Drupal::entityManager()->getStorage('menu')->load($menu['menu_name']);

   We would also inject that here.

3. +++ b/core/modules/views/src/Plugin/views/row/RssPluginBase.php
@@ -0,0 +1,116 @@
+    $view_modes = \Drupal::entityManager()->getViewModes($this->entityTypeId);

   Let's use the injected object.

4. +++ b/core/modules/views/src/Tests/ViewTestData.php
@@ -45,9 +45,10 @@ public static function createTestViews($class, array $modules) {
-        foreach ($file_storage->listAll('views.view.') as $config_name) {
-          $id = str_replace('views.view.', '', $config_name);
-          if (in_array($id, $views)) {
+        $available_views = $file_storage->listAll('views.view.');
+        foreach ($views as $id) {
+          $config_name = 'views.view.' . $id;
+          if (in_array($config_name, $available_views)) {
             $storage

   the new logic does indeed make a little bit more sense.

5. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_page_display_menu.yml
   index 4399c42..802f06f 100644
   --- a/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_tag_cache.yml

   --- a/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_tag_cache.yml
   +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_tag_cache.yml

   +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_tag_cache.yml
   @@ -57,47 +57,6 @@ display:
   -      filters:
   -        type:
   -          id: type
   -          table: node_field_data
   -          field: type
   -          relationship: none
   -          group_type: group
   -          admin_label: ''
   -          operator: in
   -          value:
   -            page: page
   -          group: 1
   -          exposed: false
   -          expose:
   -            operator_id: ''
   -            label: ''
   -            description: ''
   -            use_operator: false
   -            operator: ''
   -            identifier: ''
   -            required: false
   -            remember: false
   -            multiple: false
   -            remember_roles:
   -              authenticated: authenticated
   -            reduce: false
   -          is_grouped: false
   -          group_info:
   -            label: ''
   -            description: ''
   -            identifier: ''
   -            optional: true
   -            widget: select
   -            multiple: false
   -            remember: false
   -            default_group: All
   -            default_group_multiple: {  }
   -            group_items: {  }
   -          plugin_id: bundle
   -          entity_type: node
   -          entity_field: type

   wat?

6. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -1129,6 +1129,12 @@ public function calculateDependencies() {
       */
   +  public function getConfigDependencyKey() {
   +  }
   +
   +  /**
   +   * {@inheritdoc}

   +++ b/core/modules/views_ui/tests/src/Unit/ViewUIObjectTest.php
   @@ -42,7 +42,7 @@ public function testEntityDecoration() {
          // dependency management.
   -      if (!in_array($reflection_method->getName(), ['isNew', 'isSyncing', 'isUninstalling', 'getConfigDependencyName', 'calculateDependencies'])) {
   +      if (!in_array($reflection_method->getName(), ['isNew', 'isSyncing', 'isUninstalling', 'getConfigDependencyKey', 'getConfigDependencyName', 'calculateDependencies'])) {

   Lazyness ... you can easily just write $this->storage->getConfigDependencyKey() ... and no, the other cases of config relation methods are a bug as well ... Most of the decorator functions got implemented

1. Wow, so I did mess up the trivial changes/boilerplate… Sorry guys :( Fixed!
2. Wim--; fixed.
3. Done.
4. :)
5. It's not a strange as it seems at first sight — see #73 and the reply to that in #80.
6. Indeed. But indeed, the reason I did it, is because the other config method doesn't do it. I've fixed all of them now.

I uploaded the interdiff as the patch. Sorry about that.

Thank you

Can we get a followup to inject this service now we have all the boilerplate.

Done: #2394021: Inject menu parent form selector and module handler services into Views' Page display plugin.

(So: the filters *do* hurt, the multiple bundles instead of a single bundle *don't* hurt.)

But if we are removing the bundle filters, we should have also just removed the creation of the bundles too maybe? IF/WHEN bundle level cache tags come back, these filters will be needed again anyway, so I would have rather it was just fixed in this issue and left alone, but hey, I'm only a views maintainer.. :) I'll take your word for it that it should work ok when we do need to do that.

If you want me to get rid of the second bundle, I'll do that. Do you want me to do it in a follow-up?