Views plugins do not store additional dependencies

I'll work on this. I think we'll want the other issue to go in first.

#2309247: Views do not depend on modules providing their displays is a solution for a very limited subset of this issue by inheriting the provider form the display like it is from handlers. That causes direct schema validation issues with existing shipped views in core, and big enough in itself to resolve that its fine in its own issue IMHO.

I think this should probably be critical - if we start validating something after release that we previously didn't, then sites are going to get into an invalid state that they either need to fix themselves or we have to write an upgrade path.

Tagging with D8 upgrade path and moving to major for now anyway, since this requires resaving all the views in the system to add the correct dependencies to the config YAML once it's been added.

Some more work.

A little bit more work.

@damiankloip
Ah I see. I wonder whether we have to split these dependencies then?

@damiankloip
Now that you will merge the two issues together, can we mark the other issue as duplicate and move this on to be critical?

Apologies if I miss the mark on some things in my review. I probably need a walkthrough of the patch to fully grok it.

1. +++ b/core/modules/views/src/Entity/View.php
   @@ -279,15 +290,22 @@ public function calculateDependencies() {
   +        if (!empty($display['display_options'][$plugin_type]['dependencies'])) {
   

   Missing comment: Add (inherit) the plugin's dependencies.

2. +++ b/core/modules/views/src/Plugin/views/query/QueryPluginBase.php
   @@ -118,6 +118,22 @@ public function summaryTitle() {
   +    foreach ($this->getEntityTableInfo() as $entity_type => $info) {
   +      if (!empty($info['provider'])) {
   +        $dependencies['module'][] = $info['provider'];
   +        $dependencies['entity'][] = $entity_type;
   +      }
   +    }
   

   So once getEntityTableInfo() (or its successor) returns metadata about what the current view filters by (e.g. "only article nodes"), we will add dependencies on the bundle's config entity type?

3. +++ b/core/modules/views/src/Plugin/views/relationship/RelationshipPluginBase.php
   @@ -163,6 +163,22 @@ public function query() {
   +   * @todo Just remove this and rely on query dependencies?
   +   */
   +  public function getDependencies() {
   

   If that's possible, then yes!

4. +++ b/core/modules/views/src/Tests/Entity/ViewEntityDependenciesTest.php
   @@ -15,56 +16,57 @@
   -class ViewEntityDependenciesTest extends ViewTestBase {
   +class ViewEntityDependenciesTest extends ViewUnitTestBase {
   

   Nice! :)

5. +++ b/core/modules/views/src/Tests/Entity/ViewEntityDependenciesTest.php
   @@ -15,56 +16,57 @@
   +      'test_dependency' => array(
   +        'row', 'style',
   +      )
   

   Extreme nitpick: strange code formatting.

6. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_plugin_dependencies.yml
   @@ -0,0 +1,60 @@
   +core: 8.0-dev
   
   +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_relationship_dependency.yml
   @@ -0,0 +1,51 @@
   +core: 8.0-dev
   

   We probably don't want these.

7. Could be a problem with non-entity table data?

   Interesting! And I definitely don't fully understand this, but… doesn't Views need some sort of plugin to tell it to use a certain DB table, otherwise Views will never use it anyway? if that's the case, why can't that plugin provide the necessary dependency information?

1. +++ b/core/modules/views/src/Entity/View.php
   @@ -279,15 +278,23 @@ public function calculateDependencies() {
   +
   +      // @todo This totally does not work for argument validator/default
   +      //   plugins.
    
          // Collect all dependencies of plugins.
   

   Do you think we can do this as part of this patch? It seems odd to not support a specific subset. Wait, but can't we add getDependencies() those dependencies?

2. +++ b/core/modules/views/src/Entity/View.php
   @@ -383,11 +390,4 @@ public function mergeDefaultDisplaysOptions() {
    
   -  /**
   -   * Wraps drupal_get_schema().
   -   */
   -  protected function drupalGetSchema($table = NULL, $rebuild = FALSE) {
   -    return drupal_get_schema($table, $rebuild);
   -  }
   -
   

   Yeah!

3. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
   @@ -392,6 +392,9 @@ public function getDependencies() {
       *
   +   * @todo Update this docblock to reflect the dependencies structure for module,
   +   *   entity etc..
   

   Yeah just remove that todo, noone will see it and we don't really like honest todos.

4. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -297,7 +297,7 @@ class ViewExecutable {
       *
       * @var array
       */
   -  public $relationship;
   +  public $relationship = array();
   

   A new issue to use new Type[] on there would be cool for all those handler types.

+1 for decoupling it!

+++ b/core/modules/views/src/ViewsData.php
index 34167b0..33925df 100644
--- a/core/modules/views/views.views.inc

--- a/core/modules/views/views.views.inc
+++ b/core/modules/views/views.views.inc

+++ b/core/modules/views/views.views.inc
+++ b/core/modules/views/views.views.inc
@@ -160,8 +160,13 @@ function views_views_data() {

@@ -160,8 +160,13 @@ function views_views_data() {
   foreach (\Drupal::entityManager()->getDefinitions() as $entity_type_id => $entity_type) {
     if ($entity_type->hasHandlerClass('views_data')) {
       /** @var \Drupal\views\EntityViewsDataInterface $views_data */
-      $views_data = \Drupal::entityManager()->getHandler($entity_type_id, 'views_data');
-      $data = NestedArray::mergeDeep($data, $views_data->getViewsData());
+      $views_data = \Drupal::entityManager()->getHandler($entity_type_id, 'views_data')->getViewsData();
+      // Make sure each table has a provider.
+      $provider = $entity_type->getProvider();
+      array_walk($views_data, function(&$table_data) use ($provider){
+        $table_data['table']['provider'] = $provider;
+      });
+      $data = NestedArray::mergeDeep($data, $views_data);
     }
   }

Should we also move that code into the generic EntityViewsData implementation somehow?

Can't we take the provider the from entity type definition and be done?

Do you think we can do this as part of this patch? It seems odd to not support a specific subset. Wait, but can't we add getDependencies() those dependencies?

Let's first sort out the storage we use, #2356181: Unify the storage of argument default/validator/summary plugins. will allow us to do that.

A new issue to use new Type[] on there would be cool for all those handler types.

Let's add another follow up

Some tiny work here and there.

Let's fix the failures ...

So yeah I would prefer the casting here over the default value of the ViewExecutable as we might want to distinct between
no relationship and no initialized relationships.

In an effort to nudge this along. This applies nicely in simplytest.me - it's unclear to me though what the steps are for testing this.

What are the steps required to bring this to RTBC?

Bringing the patch more inline with ConfigEntityBase::calculatePluginDependencies() - I think views plugins should use the DependentPluginInterface from #2271419: Allow field types, widgets, formatters to specify config dependencies

We have a problem with relationship plugins are working out their dependencies.

Removed Drupal\Tests\views\Unit\Entity\ViewTest because setting up the dependencies to test under PHPUnit is just not worth it.

Removed Drupal\Tests\views\Unit\Entity\ViewTest because setting up the dependencies to test under PHPUnit is just not worth it.

What?!

re #44 the current test shows precisely why this is not worth it - dependencies on entity provided schemas has been broken for a couple of months and this test did not tell us (or force the entity schema patch to fix) because of the way it was mocking stuff. Dependency management is 99.9% about integrating across different types of things - in views case those type of things are more than any other config entity. If we mock stuff and think we have this tested we are leading ourselves to have a false sense of security.

relationship plugins now add dependencies based on the base table they bring to the party!

renamed the geDependencies() method to calculateDependencies() so we use DependentPluginInterface once #2271419: Allow field types, widgets, formatters to specify config dependencies lands. Also this means views plugins are one step closer ConfigurablePluginInterface.

Added test assertion for the dependency added by Drupal\field\Plugin\views\field\Field

Now that #2271419: Allow field types, widgets, formatters to specify config dependencies has landed we can just use regular plugin dependency code :)

@damiankloip / @dawehner do you think we need to do anything else here? I'd be in favour of handling anything else in followups since this is already a massive improvement over what we have.

Opened #2368767: Implement calculateDependencies() in ArgumentPluginBase to get dependencies from validator and default plugins to handle argument validator/default plugins. Patch attached fixes the @todo.

Thank you for the nice work on the patch @alexpott. I think this patch is almost ready. Just two minor issues.

1. +++ b/core/modules/views/src/Entity/View.php
   @@ -100,6 +100,13 @@ class View extends ConfigEntityBase implements ViewStorageInterface {
   +  protected $viewsData;
   

   Not used anywhere in the class.

2. +++ b/core/modules/views/src/ViewsData.php
   @@ -230,7 +231,18 @@ protected function getData() {
   +            $table['table']['provider'] = $module;
   

   We should add this in docs of hook_views_data().

Thanks for the review @jibran!

Patch addresses both points in #50.

Thanks for the fix it is RTBC for me but I let @dawehner and @damiankloip to have a go at it.

1. +++ b/core/modules/comment/src/Plugin/views/wizard/Comment.php
   @@ -150,6 +150,7 @@ protected function defaultDisplayOptions() {
   +    $display_options['relationships']['node']['dependencies'] = [];
   
   @@ -159,6 +160,7 @@ protected function defaultDisplayOptions() {
   +    $display_options['fields']['subject']['dependencies'] = [];
   

   Can't we drop those entries now?

2. +++ b/core/modules/file/src/Plugin/views/wizard/File.php
   @@ -54,6 +54,7 @@ protected function defaultDisplayOptions() {
   +    $display_options['fields']['filename']['dependencies'] = [];
   
   +++ b/core/modules/node/src/Plugin/views/wizard/Node.php
   @@ -138,6 +138,7 @@ protected function defaultDisplayOptions() {
   +    $display_options['fields']['title']['dependencies'] = [];
   
   +++ b/core/modules/node/src/Plugin/views/wizard/NodeRevision.php
   @@ -99,6 +99,7 @@ protected function defaultDisplayOptions() {
   +    $display_options['fields']['changed']['dependencies'] = [];
        $display_options['fields']['changed']['alter']['alter_text'] = FALSE;
   
   @@ -115,6 +116,7 @@ protected function defaultDisplayOptions() {
   +    $display_options['fields']['title']['dependencies'] = [];
   
   +++ b/core/modules/taxonomy/src/Plugin/views/wizard/TaxonomyTerm.php
   @@ -52,6 +52,7 @@ protected function defaultDisplayOptions() {
        $display_options['fields']['name']['label'] = '';
        $display_options['fields']['name']['alter']['alter_text'] = 0;
        $display_options['fields']['name']['alter']['make_link'] = 0;
   diff --git a/core/modules/user/src/Plugin/views/wizard/Users.php b/core/modules/user/src/Plugin/views/wizard/Users.php
   
   diff --git a/core/modules/user/src/Plugin/views/wizard/Users.php b/core/modules/user/src/Plugin/views/wizard/Users.php
   index 127b84b..24b71df 100644
   
   index 127b84b..24b71df 100644
   --- a/core/modules/user/src/Plugin/views/wizard/Users.php
   
   --- a/core/modules/user/src/Plugin/views/wizard/Users.php
   +++ b/core/modules/user/src/Plugin/views/wizard/Users.php
   
   +++ b/core/modules/user/src/Plugin/views/wizard/Users.php
   +++ b/core/modules/user/src/Plugin/views/wizard/Users.php
   @@ -75,6 +75,7 @@ protected function defaultDisplayOptions() {
   
   @@ -75,6 +75,7 @@ protected function defaultDisplayOptions() {
        $display_options['fields']['name']['table'] = 'users_field_data';
        $display_options['fields']['name']['field'] = 'name';
        $display_options['fields']['name']['provider'] = 'user';
   +    $display_options['fields']['name']['dependencies'] = [];
        $display_options['fields']['name']['label'] = '';
        $display_options['fields']['name']['alter']['alter_text'] = 0;
        $display_options['fields']['name']['alter']['make_link'] = 0;
   

   more places ...

3. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
   index df5de7a..352aaf0 100644
   --- a/core/modules/views/src/Plugin/views/ViewsPluginInterface.php
   
   --- a/core/modules/views/src/Plugin/views/ViewsPluginInterface.php
   +++ b/core/modules/views/src/Plugin/views/ViewsPluginInterface.php
   
   +++ b/core/modules/views/src/Plugin/views/ViewsPluginInterface.php
   @@ -20,14 +20,11 @@
   +  public function getProvider();
   

   Its super confusing that PluginInspectionInterfacedoes not provide getProvider() itself.

4. +++ b/core/modules/views/src/Plugin/views/relationship/RelationshipPluginBase.php
   @@ -164,6 +164,18 @@ public function query() {
   +    return [
   +      'module' => [$table_data['table']['provider']],
   +    ];
   

   Wow this mixup between module and provider is super confusing ... Is there an issue to rename module to provider on there?

re #53
1,2. Fixed - also we can remove provider from being saved as well - there's no need for it.
3. I think this is out of scope
4. I don;t think it is a mix up - provider can be "core" or a module - in config dependencies they are always modules and it would not make sense to call a dependency a provider.

This patch has got big because we no longer store dependencies and the provider of all the plugins used by a view.

4. I don;t think it is a mix up - provider can be "core" or a module - in config dependencies they are always modules and it would not make sense to call a dependency a provider.

The only thing is that its kinda inconvenient to read, anyway yeah we should focus here.

1. +++ b/core/modules/comment/config/install/views.view.comments_recent.yml
   --- a/core/modules/comment/src/Plugin/views/wizard/Comment.php
   +++ b/core/modules/comment/src/Plugin/views/wizard/Comment.php
   
   +++ b/core/modules/comment/src/Plugin/views/wizard/Comment.php
   +++ b/core/modules/comment/src/Plugin/views/wizard/Comment.php
   @@ -150,6 +150,7 @@ protected function defaultDisplayOptions() {
   
   @@ -150,6 +150,7 @@ protected function defaultDisplayOptions() {
        $display_options['relationships']['node']['required'] = 1;
        $display_options['relationships']['node']['plugin_id'] = 'standard';
        $display_options['relationships']['node']['provider'] = 'views';
   +    $display_options['relationships']['node']['dependencies'] = [];
    
   

   Meh, here is a leftover.

2. +++ b/core/modules/node/src/Plugin/views/wizard/NodeRevision.php
   @@ -115,6 +114,7 @@ protected function defaultDisplayOptions() {
        $display_options['fields']['title']['provider'] = 'node';
   +    $display_options['fields']['title']['dependencies'] = [];
        $display_options['fields']['title']['label'] = '';
   

   meh2

3. +++ b/core/modules/views/src/Plugin/views/BrokenHandlerTrait.php
   @@ -20,10 +20,7 @@
   -    $args = array(
   -      '@module' => $this->definition['original_configuration']['provider'],
   -    );
   -    return t('Broken/missing handler (Module: @module) …', $args);
   +    return t('Broken/missing handler');
   
   @@ -61,7 +58,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   -      t('Module: @module', array('@module' => $this->definition['original_configuration']['provider'])),
   +//      t('Module: @module', array('@module' => $this->definition['original_configuration']['provider'])),
   

   This is really sad ...

Here is a follow up

1. +++ b/core/modules/field/src/Plugin/views/field/Field.php
   @@ -938,9 +938,11 @@ function field_langcode(EntityInterface $entity) {
   +    return [
   +      'config' => [$this->getFieldStorageConfig()->getConfigDependencyName()]
   +    ];
   

   Random change? Missing trailing comma.

2. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -908,7 +908,7 @@ public function getHandlers($type) {
   -      foreach ($this->getOption($plural) as $id => $info) {
   +      foreach ((array) $this->getOption($plural) as $id => $info) {
   
   +++ b/core/modules/views/src/Plugin/views/query/QueryPluginBase.php
   @@ -260,9 +275,15 @@ public function getEntityTableInfo() {
   -    foreach ($this->view->relationship as $relationship_id => $relationship) {
   +    foreach ((array) $this->view->relationship as $relationship_id => $relationship) {
   

   Why the added cast?

re #57

1. Fixed
2. Added a comment

re #59 yep these are necessary because provider and dependencies are no longer used.

Thanks for the review @damiankloip

1. Fixed
2. Yep we have code that automatically adds this see ViewsData::getData()
3. I have improved the information on the options form - BUT we should never get into this situation if the config dependency system is working properly.
4. See point 3 above
5. Stuck with the new style
6. It didn't seem to mean that. In testing without the implementation of calculateDependencies() in the relationship plugin this info was not being added (by the query plugin)
7. See point 6 above

I had a look at calling build() - this turns out to be quite a big problem for ViewsUnitTestBase and ViewsTestBase. Basically we have a problem with the fact that these abstract classes create views before the concrete test has a had a chance to set up all the necessary dependencies of the views - eg comment fields etc. I think we should handle this is a followup since the patch does what it necessary to unblock the configuration issues and this will involve changing many ViewsUnitTestBase and ViewsTestBase tests. I've created to #2371751: Build the view executable in View::calculateDependencies() to explore this.

re #66 these are needed because this data is loaded through the views module - so it makes the provider of the node table views if I remove them. So I think we are done here.

Sorry folks this needs a better issue summary.

+ // Figure out what base table this relationship brings to the party and add
+ // a dependency on its provider.

Not sure about 'brings to the party'.

In general is it really the case that while we're removing the provider key from the YAML we don't get any additional dependencies in the default views from these changes at all? i.e. what we end up in the YAML is just the removal of duplicate information that happens to be already covered? Assuming this wouldn't be the case with a more complex default view in core.

The dependencies of the default views are recalculated on config install so in some ways what is in the config/install directory does not matter. I think we should revisit all the default configuration entities before release and re-export so that the yaml matches what would be exported (without the UUID of course).

diff --git a/core/lib/Drupal/Core/Config/BootstrapConfigStorageFactory.php b/core/lib/Drupal/Core/Config/BootstrapConfigStorageFactory.php
index c2abc3b..2462eaf 100644
--- a/core/lib/Drupal/Core/Config/BootstrapConfigStorageFactory.php
+++ b/core/lib/Drupal/Core/Config/BootstrapConfigStorageFactory.php
@@ -18,22 +18,16 @@ class BootstrapConfigStorageFactory {
   /**
    * Returns a configuration storage implementation.
    *
-   * @param $class_loader
-   *   The class loader. Normally Composer's ClassLoader, as included by the
-   *   front controller, but may also be decorated; e.g.,
-   *   \Symfony\Component\ClassLoader\ApcClassLoader.
-   *
    * @return \Drupal\Core\Config\StorageInterface
    *   A configuration storage implementation.
    */
-  public static function get($class_loader = NULL) {
+  public static function get() {
     $bootstrap_config_storage = Settings::get('bootstrap_config_storage');
-    $storage_backend = FALSE;
     if (!empty($bootstrap_config_storage) && is_callable($bootstrap_config_storage)) {
-      $storage_backend = call_user_func($bootstrap_config_storage, $class_loader);
+      return call_user_func($bootstrap_config_storage);
     }
     // Fallback to the DatabaseStorage.
-    return $storage_backend ?: self::getDatabaseStorage();
+    return self::getDatabaseStorage();
   }
 
   /**

Faulty merge.

Edit: The DrupalKernel hunk as well.

Edit 2: In fact this patch simply seems to be generated without having merge 8.0.x first, there's a lot of reverts of recent patches in there.

Rebased - how embarrassing :(

relationship plugins now add dependencies based on the base table they bring to the party!

YAY!!!

260.16 KB

GASP!

I don't want to hold up this patch, but I was always under the impression that this patch would bring support for config entity dependencies as well? I.e. if a view lists entities in a certain view mode (the front page lists entities in 'teaser' view mode) it should depend on the view mode entities in question. If it filters to article nodes, it should depend on the article node type config entity. And so on. I suppose that is now out of scope? Or perhaps I'm mistaken and that's a different issue?

It's essential that we can figure out those config entity dependencies as well, because we'll need to make sure that cached views are invalidated when any of those config entities change. It is also essential to use more granular entity list cache tags in the future (i.e. if we know we're only listing 'Blog' nodes, then we can use a Blog node-specific list cache tag so that we invalidate as rarely as possible).

Manually tested, works as expected!

1. +++ b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_field_filters.yml
   @@ -281,10 +251,6 @@ display:
   -          dependencies:
   -            module:
   -              - views
   -              - views
   

   Hah!

2. +++ b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_field_filters.yml
   @@ -352,11 +315,6 @@ display:
   -          dependencies:
   -            module:
   -              - views
   -              - views
   -              - views
   

   Hahaha!

3. +++ b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_language.yml
   @@ -200,12 +181,6 @@ display:
   -          dependencies:
   -            module:
   -              - views
   -              - views
   -              - views
   -              - views
   

   This keeps getting better.

4. +++ b/core/modules/views/src/Plugin/views/relationship/RelationshipPluginBase.php
   @@ -164,6 +164,18 @@ public function query() {
   +    // Figure out what base table this relationship brings to the party and add
   +    // a dependency on its provider.
   

   Like catch said, this comment should probably be more … sec. :)

1. :)
2. :) :)
3. :) :) :)
4. Fixed

Created #2372855: Add content & config entity dependencies to views to handle the additional dependencies from using entity displays.

+1.1

RTBC+1

-          provider: views
-        uid:
-          admin_label: ''
-          field: uid
-          group_type: group
-          id: uid
-          label: author
-          relationship: node
-          required: '0'
-          table: node_field_data
-          plugin_id: standard
-          provider: views

Is this hunk intentional?

#78 yes - the test view was completely invalid. And this test view exists purely for the test and now it tested.

OK, awesome. Thanks!

Looks great now. Committed/pushed to 8.0.x, thanks!