Views plugins need a way to store additional dependencies

SHouldn't we add the module providing the handler add automatically to its dependencies?

Additional we should also provide some method which collects all the dependencies of all handlers/plugins in a view. Do you want to work on that in a follow up?

+1 for the renaming.

Continued a bit.

I think we should be adding config dependency on the field instance or maybe entity display rather than the module. Since the view is broken when the field instance is deleted - not when the module providing the field type is uninstalled.

+++ b/core/modules/views/lib/Drupal/views/Entity/View.php
@@ -365,4 +365,34 @@ public function mergeDefaultDisplaysOptions() {
+  public function getDependencies() {

we already have View::calculateDependencies() I think this should be rolled into that.

Continued with some work on here

Let's fix stuff.

  return array('module' => array($field->get('module')));

Should be return array('entity' => array($field->getConfigDependencyName()));

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/PluginBase.php
@@ -424,4 +424,16 @@ public static function preRenderFlattenData($form) {
+  /**
+   * Returns an array of module dependencies for this plugin.
+   *
+   * Dependencies are a list of module names, which might depend on the
+   * configuration.
+   *
+   * @return array
+   */
+  public function getDependencies() {
+    return array();
+  }

Do we have a clue why we don't call that at the moment?

1. +++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.php
   @@ -882,4 +882,14 @@ function field_langcode(EntityInterface $entity) {
   +    $field = FieldHelper::fieldInfo()->getField($this->definition['entity_type'], $this->definition['field_name']);
   

   In this class, $this->field_info = FieldHelper::fieldInfo()->getField($this->definition['entity_type'], $this->definition['field_name']); is called in init(). This is called after that, so we can reuse that, right?

2. +++ b/core/modules/views/tests/Drupal/views/Tests/Entity/ViewTest.php
   @@ -0,0 +1,122 @@
   +  public function testCalculateDependenciesWithNoDependencies() {
   ...
   +  public function testCalculateDependenciesOnHandlers() {
   ...
   +  public function testCalculateDependenciesOnPlugins() {
   

   These seem like they could have used data providers...

3. +++ b/core/modules/views/tests/Drupal/views/Tests/Entity/ViewTest.php
   @@ -0,0 +1,122 @@
   +if (!function_exists('t')) {
   +  function t($string) {
   +    return $string;
   +  }
   +}
   

   Booooo hisssssss

4. +++ b/core/modules/views_ui/lib/Drupal/views_ui/Form/Ajax/ConfigHandler.php
   @@ -238,6 +238,14 @@ public function submitForm(array &$form, array &$form_state) {
   +    // Add any dependencies as the handler is saved. Put it here so
   +    // it does not need to be declared in defineOptions().
   +    if ($dependencies = $handler->getDependencies()) {
   +      $handler->options['dependencies'] = $dependencies;
   +    }
   +    // Add the module providing the handler as a dependency as well.
   +    $handler->options['dependencies']['module'][] = $handler->definition['provider'];
   

   Yay not mucking up defineOptions more.

@damiankloip
Do you want to bring that one home or should someone else?

I'll give this a shot.

There we go.

+++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.php
@@ -51,7 +51,12 @@ class Field extends FieldPluginBase {
-  public $field_info;
+  public $fieldDefinition;

@@ -141,18 +146,43 @@ public static function create(ContainerInterface $container, array $configuratio
+  protected function getFieldDefinition() {

So we keep the public property, but leave the method protected?
Assuming we'll still need to access the field definition of a field handler, I'd much rather make the method public and add an @todo to make the property protected.

Otherwise, the patch looks great.

Sure, let's do it.

Oh wow, I didn't think that would work!

If I add the body field to the content view rhe dependencies become

dependencies:
  entity:
    - field.field.node.body
  module:
    - field
    - node
    - user

they were:

dependencies:
  module:
    - node
    - user

Adding the field module as well as the field entity is a little odd. Not sure how to fix this though. Setting to needs review to get opinion on whether this is important.

In an ideal world field module would not be required. If we assume this, the user should see that field module is needed, not just a config entity with a quite obscure name.

Do you suggest to get rid of required modules as dependencies?

I don't think it's a bad thing to declare a dependency on the field module. It's for the present a required module, but so is user, and we're declaring a dependency on that. Having the explicit dependency seems correct to me, since the field module is providing the needed plugins as well as the entity the view depends on. Sure, the entity it depends on already has its own dependency on the module, but I don't see anything wrong with it. The important thing is that field.module, its plugins, and its relevant namespaced config entities be available when we work with the view.

NB: I personally haven't reviewed the patch in depth, just @dawehner and I agreeing on a reply to the question @alexpott bumped this back for. :)

Not commit-blocking:

1. +++ b/core/modules/views/tests/Drupal/views/Tests/Entity/ViewTest.php
   @@ -0,0 +1,97 @@
   +<?php
   +use Drupal\views\Entity\View;
   ...
   +use Drupal\views\Entity\View;
   

   Why is this use statement in there twice?

2. +++ b/core/modules/views/tests/Drupal/views/Tests/Entity/ViewTest.php
   @@ -0,0 +1,97 @@
   +class TestView extends View {
   

   We maybe don't care too much since this is provided for a unit test, but no docblock. :)

Committed 5cc27d9 and pushed to 8.x. Thanks!

diff --git a/core/modules/views/tests/Drupal/views/Tests/Entity/ViewTest.php b/core/modules/views/tests/Drupal/views/Tests/Entity/ViewTest.php
index 0aae557..20e0bf4 100644
--- a/core/modules/views/tests/Drupal/views/Tests/Entity/ViewTest.php
+++ b/core/modules/views/tests/Drupal/views/Tests/Entity/ViewTest.php
@@ -1,5 +1,4 @@
 <?php
-use Drupal\views\Entity\View;
 
 /**
  * @file

Fixed during commit.