Views should be dependent on providers of table data via hook_views_data

Something like this.

So yay as expected existing tests show that we have missing coverage.

If you look at core/modules/views/tests/modules/views_test_config/test_views/views.view.test_page_display.yml this should depend on views_test_data because it links to a table provided by that module. It currently doesn't in HEAD. With this patch it does.

From \Drupal\views\ViewsData::getData():

      foreach ($modules as $module) {
        $views_data = $this->moduleHandler->invoke($module, 'views_data');
        // Set the provider key for each base table.
        foreach ($views_data as &$table) {
          if (isset($table['table']) && !isset($table['table']['provider'])) {
            $table['table']['provider'] = $module;
          }
        }
        $data = NestedArray::mergeDeep($data, $views_data);
      }

So we're already ensuring that the provider is set which is really nice.

Missed one.

re #8 That's all handled in \Drupal\Core\Plugin\PluginDependencyTrait::calculatePluginDependencies() already. Which is called by \Drupal\views\Entity\View::calculateDependencies() to add each plugin's dependencies.

@Lendude++ indeed that is missing. On it.

Here's an upgrade path with a test. The update-test-ony.patch is interdiff.

@Lendude++ like it! I copied from system_post_update_recalculate_configuration_entity_dependencies() but your idea is nice.

@Lendude good question! Views handles this already - it's the plugins use the Broken plugin and that won't add the dependency :) so this patch doesn't make any change to the view because no dependencies change.

+++ b/core/modules/views/tests/fixtures/update/views-data-table-dependency.php
@@ -0,0 +1,71 @@
+// We need the views_test_data table to exist and state entries for
+// views_test_data_schema() and views_test_data_views_data().
+$schema = ViewTestData::schemaDefinition();
+$connection->schema()->createTable('views_test_data', $schema['views_test_data']);
+$connection->insert('key_value')
+  ->fields([
+    'collection',
+    'name',
+    'value',
+  ])
+  ->values([
+    'collection' => 'state',
+    'name' => 'views_test_data_schema',
+    'value' => serialize($schema),
+  ])
+  ->values([
+    'collection' => 'state',
+    'name' => 'views_test_data_views_data',
+    'value' => serialize(ViewTestData::viewsData()),
+  ])
+  ->execute();

All of this is to ensure that the field doesn't use the broken handler and therefore adds the dependency as expected.

Missing dependencies is at least a major given how they impact data integrity.

This is actually a critical bug at least for Thunder. This is because without this fix views that use content_lock fields don't depend on content_lock. Therefore if you uninstall content_lock you break your site.

@larowlan re #1 kinda they all eventually get added to config via \Drupal\Core\Entity\DependencyTrait::addDependency() which ensures things only get added once.

Agree re point 2. In fact we can do something even better. Hopefully this will become the new way we do this. Batching and mutli-loading ftw.

I think we can generalise the update process see #2949351: Add a helper class to make updating configuration simple

@catch they don't change because they are all have the dependencies by other means atm. If this did add a dependency we'd get an error in \Drupal\KernelTests\Config\DefaultConfigTest which explicitly re-saves configuration entities to ensure that the provided defaults match what would be there if you just re-save a configuration entity immediately after install.