Updated: Comment #N

Problem/Motivation

We added ways store the provider for all views handlers and plugins. This is good for the likes of #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall, as we can use the providers to help build dependencies. However, there is a missing piece; Handlers such as the "field" field handler (confusing, yes) provided by the field module (Drupal\field\Plugin\views\field\Field) will have a provider of 'field' as the field module has provided the plugin. The selected field could then use something like image, for example. So there is actually a dependency on field and image modules. Currently, we have no way of determining this.

Proposed resolution

1. Rely on having the plugin instances available (#2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall will just use config data atm - which is probably the right thing to be doing), we could then have a getDependencies() method that is called that can add optional deps.
2. Add a new mechanism when plugins/handlers are added to a view that call something similar to above to get these optional additoinal dependencies, and add that to the configuration for that plugin/handler. Similar to when we add the provider key for example.

Remaining tasks

Everything

User interface changes

None

API changes

Could break some older default views, needing an update to these additional dependencies...

Files: 
CommentFileSizeAuthor
#31 interdiff-2216071.txt533 bytesdawehner
#31 2216071-31.patch17.33 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,050 pass(es). View
#29 interdiff.txt6.98 KBdawehner
#29 vdc-2216071-29.patch17.33 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,070 pass(es). View
#25 drupal.plugin_deps.2216071.25.interdiff.txt4.24 KBGaelan
#25 drupal.plugin_deps.2216071.25.patch11.02 KBGaelan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,805 pass(es), 9 fail(s), and 2 exception(s). View
#18 interdiff-2216071-18.txt2.24 KBdamiankloip
#18 2216071-18.patch11.68 KBdamiankloip
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,601 pass(es). View
#6 interdiff.txt7.2 KBdawehner
#6 dependencies-2216071-6.patch6.79 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#2 2216071-2.patch2.29 KBdamiankloip
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,791 pass(es). View
#1 2216071.patch2.27 KBdamiankloip
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,789 pass(es). View

Comments

damiankloip’s picture

FileSize
2.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,789 pass(es). View

Something like this to start, as a general idea. This is by no means tested or anything.

damiankloip’s picture

FileSize
2.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,791 pass(es). View

This would actually work though. So we bypass defineOoptions/unpackOptions() logic.

dawehner’s picture

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?

damiankloip’s picture

We can work on that here too, that was literally a 30 second patch :)

We can add the module too, that would keep it in one place. We might want to just rename s/additional_dependencies/dependencies in that case though.

dawehner’s picture

+1 for the renaming.

dawehner’s picture

FileSize
6.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
7.2 KB

Continued a bit.

damiankloip’s picture

damiankloip’s picture

Also, just created #2219689: Separate out logic for getPluginTypes/viewsHandlerTypes from ViewExecutable in an attempt to help improve the messy situation we have with iterating over different types of views plugins.

catch’s picture

Status: Active » Needs work
alexpott’s picture

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.

alexpott’s picture

Priority: Major » Critical
Issue tags: +Configurables, +beta blocker, +missing config dependencies
+++ 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Continued with some work on here

Status: Needs review » Needs work

The last submitted patch, 12: views-dependencies-2216071-12.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
7.48 KB

Let's fix stuff.

Status: Needs review » Needs work

The last submitted patch, 14: views-dependencies-2216071-14.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
11.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,599 pass(es), 0 fail(s), and 25 exception(s). View
4.77 KB

Status: Needs review » Needs work

The last submitted patch, 16: 2216071-16.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
11.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,601 pass(es). View
2.24 KB

Let's try and get a passing patch, and go from there.

alexpott’s picture

Status: Needs review » Needs work
  return array('module' => array($field->get('module')));

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

damiankloip’s picture

Nice, that makes a lot of sense. Will go into the next patch!

dawehner’s picture

+++ 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?

tim.plunkett’s picture

  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.

dawehner’s picture

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

Gaelan’s picture

Assigned: Unassigned » Gaelan

I'll give this a shot.

Gaelan’s picture

FileSize
11.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,805 pass(es), 9 fail(s), and 2 exception(s). View
4.24 KB
Gaelan’s picture

Assigned: Gaelan » Unassigned
Status: Needs work » Needs review

The last submitted patch, 6: dependencies-2216071-6.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 25: drupal.plugin_deps.2216071.25.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
17.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,070 pass(es). View
6.98 KB

There we go.

tim.plunkett’s picture

+++ 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.

dawehner’s picture

FileSize
17.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,050 pass(es). View
533 bytes

Sure, let's do it.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

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.

dawehner’s picture

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?

xjm’s picture

Status: Needs review » Reviewed & tested by the community

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. :)

xjm’s picture

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. :)

xjm’s picture

Assigned: Unassigned » alexpott
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • Commit 5cc27d9 on 8.x by alexpott:
    Issue #2216071 by dawehner, damiankloip, Gaelan: Views plugins need a...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.