Enforce formatter dependencies on views fields

Isn't the underlying problem that this class \Drupal\image\Plugin\Field\FieldFormatter\ImageFormatter doesn't have config dependencies?

Well yeah, this is how fields behave at the moment as well, but it will worn you that removing that image style will also drop the views.

1. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -958,6 +959,19 @@ public function calculateDependencies() {
   +      $dependencies[$style->getConfigDependencyKey()][] = $style->getConfigDependencyName();
   

   OK, then what happens with the Views field when its image style gets deleted? With this patch the entire view will be deleted. You must implement onDependencyRemoval() and define an action when the image style gets deleted. Maybe disabling the field? This is what happens, by default, on entity displays.

2. +++ b/core/modules/views/src/Tests/DependenciesTest.php
   @@ -0,0 +1,183 @@
   +      $this->drupalCreateContentType(array('type' => 'page', 'name' => 'Basic page'));
   +      $this->drupalCreateContentType(array('type' => 'article', 'name' => 'Article'));
   

   Let's go with short array syntax.

3. +++ b/core/modules/views/src/Tests/DependenciesTest.php
   @@ -0,0 +1,183 @@
   +    /*$edit = array(
   ...
   +    $this->drupalPostForm('admin/config/media/image-styles/add', $edit, t('Create new style'));*/
   

   ???

4. +++ b/core/modules/views/src/Tests/DependenciesTest.php
   @@ -0,0 +1,183 @@
   +      ->setComponent($field_name, array(
   ...
   +        'settings' => array('image_style' => $style_name),
   ...
   +    entity_create('field_storage_config', array(
   

   Short array syntax. Also entity_create() is deprecated. Use FieldStorageConfig::create();

5. +++ b/core/modules/views/src/Tests/DependenciesTest.php
   @@ -0,0 +1,183 @@
   +  function createImageField($name, $type_name, $storage_settings = array(), $field_settings = array(), $widget_settings = array()) {
   

   s/array()/[]

6. +++ b/core/modules/views/src/Tests/DependenciesTest.php
   @@ -0,0 +1,183 @@
   +    $field_config = entity_create('field_config', array(
   

   [] array syntax. Use FieldConfig::create() instead of entity_create().

7. +++ b/core/modules/views/src/Tests/DependenciesTest.php
   @@ -0,0 +1,183 @@
   +    entity_get_form_display('node', $type_name, 'default')
   

   EntityFormDisplay::create().

8. +++ b/core/modules/views/src/Tests/DependenciesTest.php
   @@ -0,0 +1,183 @@
   +      ->setComponent($name, array(
   

   s/array()/[]

9. +++ b/core/modules/views/src/Tests/DependenciesTest.php
   @@ -0,0 +1,183 @@
   +    entity_get_display('node', $type_name, 'default')
   

   EntityViewDisplay::create().

10. +++ b/core/modules/views/src/Tests/DependenciesTest.php
    @@ -0,0 +1,183 @@
    +
    

    Extraline.

11. +++ b/core/modules/views/src/Tests/DependenciesTest.php
    @@ -0,0 +1,183 @@
    +    $edit = array(
    

    s/array()/[]

12. +++ b/core/modules/views/src/Tests/DependenciesTest.php
    @@ -0,0 +1,183 @@
    +    $edit = array(
    

    s/array()/[]

13. +++ b/core/modules/views/src/Tests/DependenciesTest.php
    @@ -0,0 +1,183 @@
    +    $edit['files[' . $field_name . '_0]'] = drupal_realpath($image->uri);
    

    drupal_realpath() is deprectared. Use $this->container->get('file_system')->realpath($image->uri)

14. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.entity_test_fields_dependencies.yml
    @@ -0,0 +1,179 @@
    \ No newline at end of file
    

    Add newline.

I don't see how this cannot be major. If you have a view on the homepage with images, then if you get bitten by this bug your site is visually down.

+++ b/core/modules/views/src/Plugin/views/field/Field.php
@@ -962,6 +963,19 @@ public function calculateDependencies() {
+    // Add the image style.
+    $field = $this->getFieldDefinition();
+    $style_id = '';
+    if ($field->getType() == 'image') {
+      $style_id = $this->displayHandler->getOption('fields')[$field->getName()]['settings']['image_style'];
+    }
+
+    /** @var \Drupal\image\ImageStyleInterface $style */
+    if ($style_id && $style = ImageStyle::load($style_id)) {
+      // If this formatter uses a valid image style to display the image, add
+      // the image style configuration entity as dependency of this formatter.
+      $dependencies[$style->getConfigDependencyKey()][] = $style->getConfigDependencyName();
+    }

IMHO we should init the formatter, and then call calculateDependencies() Using that we solve a lot more potential issues, without having to hardcode anything.

That sounds interesting, what does it mean to "init the formatter"?

Something like this:

if ($formatter = $this->formatterPluginManager->getInstance($options)) {
  $settings_form = $formatter->settingsForm($form, $form_state);

@lokapujya
No worries, I just went and implemented what I had in mind.

@alexpott suggested an alternative

@dawehner, I worked in the same way but you're faster. However, I'm posting my test that shows this patch is incomplete.

PS: And I think we should replace \Drupal\views\Tests\DependenciesTest in favour of this because is more faster.

EDIT: This patch introduces also a base kernel test for Views. I know that it's somehow unrelated but it's, let's say, a "side effect".

Needs review. I followed the same strategy as in the case of entity displays: unresolved dependencies are causing the field disabling or, in the case of views, field removing.

Simplified, fixed test.

Discussed on IRC with @dawehner and @Berdir that the reaction on dependency removal needs to be handled in a general case issue for Views, taking into account multiple aspects (like filters cannot be removed because it would be a security issue). That should most probably be handled in #2468045: When deleting a content type field, users do not realize the related View also is deleted. I'm reverting the changes to only allow dependency computing.

Small fix in test.

What's the goal here? to be more generic?

Just avoid code duplication.

Eventually, we might not want to delete the view. Is there a way to verify that the view is in the configuration deletes dependency list?

See #34. Right now the view is deleted also in other similar scenarios. We stick to that and after we fix all the cases in a generic fix.

1. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -949,21 +937,48 @@ protected function addSelfTokens(&$tokens, $item) {
   +  protected function getFormatterInstance() {
   

   Note: While this is also about getting rid of duplicate code, just naming things properly (by having a method) helps to understand the code already.

2. +++ b/core/modules/views/tests/src/Kernel/ViewsIntegrationTest.php
   @@ -0,0 +1,81 @@
   +class ViewsIntegrationTest extends ViewsKernelTestBase {
   

   Let's have some actually helpful test class name, maybe we could move it to image module instead?

3. +++ b/core/modules/views/tests/src/Kernel/ViewsKernelTestBase.php
   @@ -0,0 +1,156 @@
   +class ViewsKernelTestBase extends KernelTestBase {
   

   I know we had a different issue for that already, but I'm totally fine with doing it here.

@lokapujya,

What I meant is that the webtest verified that a warning showed up when deleting from the UI. The new test only checks that the view was deleted.

Well, the UI passes the control to the API. So testing the API is just fine.

I guess if it got deleted it's safe to assume it was in the "configuration deletes" and would have thrown a warning.

But we test that the image style is in the View's dependency list. And, yes, if it's there is well known that will be deleted when the style gets deleted. And we test that later.

+++ b/core/modules/views/tests/src/Kernel/ViewsIntegrationTest.php
@@ -0,0 +1,81 @@
+    $view->save();
+
+    $dependencies = $view->getDependencies() + ['config' => []];
+
+    // Checks that style 'foo' is a dependency of view 'entity_test_fields'.
+    $this->assertTrue(in_array('image.style.foo', $dependencies['config']));

Why a new class with same name as existing class?

Well, it's the same case as the base class: KernelTestBase. The new one, that we're introducing here, will replace the old one. I'm also deprecating the old one in favour of this.

isn't this the same as base class?

No. We are extending \Drupal\KernelTests\KernelTestBase, not \Drupal\views\Tests\ViewKernelTestBase.

Let's have some actually helpful test class name, maybe we could move it to image module instead?

Yes, I've discussed with @dawehner on IRC and decided for ViewsConfigDependenciesIntegrationTest. The idea is that, in the future, we can add other tests in this class that are testing the dependencies with other modules.

Not ready.

@lokapujya

I wonder why we need to set these items.

This is how \Drupal\Core\Field\FormatterPluginManager is getting the instance. See \Drupal\Core\Field\FormatterPluginManager::getInstance().

Added update path and test for it.

Sure, why not.

Looks alright for me now.

Like the new title. Going to try to review this today.

What happens if the update runs and the dependency is already missing? Will it just not add the dependency or will there be errors from the update?

@catch
I guess you talk about the case where the dependency is already there.

Well, this would not be distinct from the case where users in the UI saves the view twice, it recalculates the dependency again, and sets it into the configuration.

@catch, I don't understand the question from #60.

Sorry wasn't very clear at all.

+++ b/core/modules/views/src/Plugin/views/field/Field.php
@@ -949,21 +937,49 @@ protected function addSelfTokens(&$tokens, $item) {
+      // Add the module providing the formatter.
+      $this->dependencies['module'][] = $this->formatterPluginManager->getDefinition($this->options['type'])['provider'];
+
+      // Add the formatter's dependencies.
+      if (($formatter = $this->getFormatterInstance()) && $formatter instanceof DependentPluginInterface) {
+        $this->calculatePluginDependencies($formatter);
+      }
     }
 
-    return $dependencies;
+    return $this->dependencies;

I mean what happens if we get here and the dependency (i.e. image style) has already been deleted?

+++ b/core/modules/system/tests/fixtures/update/drupal8.views-image-style-dependency-2649914.yml
@@ -0,0 +1,33 @@
+          image_style: thumbnail

Or to put it differently, do we need test coverage for image_style: missing_image_style?

I mean what happens if we get here and the dependency (i.e. image style) has already been deleted?

So do you argue for emptying $this->dependencies first?

No I don't think we need a code change at all (i.e. it should just skip over the dependent thing it can't find), I'd just like test coverage to confirm that.

Voilà

claudiu.cristea thanks! Sorry I didn't ask for it very well.

@lokapujya yeah not sure that's fixable - just wanted to make sure the update itself didn't error due to the same condition.

Sorry one more question.

1. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -40,7 +42,9 @@
     */
    class Field extends FieldPluginBase implements CacheableDependencyInterface, MultiItemsFieldHandlerInterface {
   +
      use FieldAPIHandlerTrait;
   +  use PluginDependencyTrait;
    
      /**
   

   This would be a problem if a class subclasses Field and also uses PluginDependencyTrait - do we know if that's definitely not happening anywhere in contrib?

2. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -949,21 +937,49 @@ protected function addSelfTokens(&$tokens, $item) {
   +   */
   +  protected function getFormatterInstance() {
   +    $settings = $this->options['settings'] + $this->formatterPluginManager->getDefaultSettings($this->options['type']);
   +
   

   Much more minor but adding the protected method to the base class is something we'd not normally do in in a patch release, fine for 8.1.x though.

Regarding #70.1
This is not a problem, if I understand it correctly, see https://3v4l.org/UYgcq

Much more minor but adding the protected method to the base class is something we'd not normally do in in a patch release, fine for 8.1.x though.

So the alternative is an _getFormatterInstance method?

I was thinking of http://php.net/manual/en/language.oop5.traits.php#language.oop5.traits.c... - but I might be mis-remembering and this only happens if the trait conflicts with a method name in the subclass rather than re-using the same trait directly. Sorry for the confusion.

So the alternative is an _getFormatterInstance method?

Yep with an @internal note - but that bit we should do after an 8.1.x commit - which I've just done. So moving to CNW for 8.0.x.

Slightly happier status...

I'm RTBCing here only the patch port.

I'm not very sure we need to point here when this method was added. IMO, we should only add @internal. Or, maybe, as comment "Should not be used in user code.". @catch, please correct this at commit if it's case. Anyway, if remains, the comment should be 1 line only.

+++ b/core/modules/views/src/Plugin/views/field/Field.php
@@ -949,21 +937,53 @@ protected function addSelfTokens(&$tokens, $item) {
+   * @internal
+   *   This method was added in 8.0.5. The underscore will be removed
+   *   in 8.1.0.

Let's move to 8.1.x first

Let's move to 8.1.

This is in 8.1.x already.

UPs.

1. +++ b/core/modules/system/tests/fixtures/update/drupal8.views-image-style-dependency-2649914.php
   @@ -0,0 +1,22 @@
   +$views_config = Yaml::decode(file_get_contents(__DIR__ . '/drupal8.views-image-style-dependency-2649914.yml'));
   ...
   +    'data' => serialize($views_config),
   
   --- /dev/null
   +++ b/core/modules/system/tests/fixtures/update/drupal8.views-image-style-dependency-2649914.yml
   

   This config is missing a UUID... nice way of creating old config pre an API change though...

2. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -951,7 +951,7 @@ protected function getAllPlugins($only_overrides = FALSE) {
   -    $this->addDependencies(parent::calculateDependencies());
   +    $this->dependencies = parent::calculateDependencies();
   

   Why is this change relevant?

3. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -471,25 +475,9 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +    if ($formatter = $this->_getFormatterInstance()) {
   
   @@ -949,21 +937,53 @@ protected function addSelfTokens(&$tokens, $item) {
   +   * @internal
   +   *   This method was added in 8.0.5. The underscore will be removed
   +   *   in 8.1.0.
   ...
   +  protected function _getFormatterInstance() {
   

   Let's create a version for 8.2.x / 8.1.x first.

4. +++ b/core/modules/views/views.post_update.php
   @@ -6,9 +6,29 @@
    /**
   + * @addtogroup updates-8.0.x
   + * @{
   

   Let's try to get this in 8.1.x... in general we're trying not to commit things with update functions to 8.0.x and as 8.1.x is in beta it feels an ideal time to get this fixed.

5. +++ b/core/modules/views/views.post_update.php
   @@ -6,9 +6,29 @@
   +/**
   + * Update dependencies to image style.
   + */
   

   This is not really about just image styles right - it is about any formatter dependency afaics.

Lol I didn't realise that this was already in 8.1.x. Given this has an upgrade function and we're in 8.1.x-beta I think it is best to not put this in 8.0.x.

Going to put back to rtbc for catch's opinion...

+++ b/core/modules/views/views.post_update.php
@@ -6,9 +6,29 @@
 /**
+ * @addtogroup updates-8.0.x
+ * @{
+ */
...
+/**
+ * @} End of "addtogroup updates-8.0.x".
+ */

This looks weird in an 8.1.x patch... :)

Also we should at least get a followup to fix the hook_update_N description as this is presented to the user and I believe it is currently misleading.

Why is this change relevant?

This is needed because \Drupal\Core\Plugin\PluginDependencyTrait requires it.

I'm happy leaving this at 8.1.x

To be honest I have no idea what to do about update function defgroups. If this goes into 8.0.x, then the degroup is correct in the 8.1.x patch too (since it's the same update and should run on an 8.0.x database), if it doesn't, then we could make it 8.1.x. But also it's added prior to 8.1.0, so in fact will never run on a newly installed 8.1.0 site.

I don't really feel like figuring that all out here, so should we just mark this fixed but open a new issue for upgrade defgroups? Or #2624318: [meta] 8.1.x upgrade path might be that issue.

So let's mark this closed and open a followup to fix the update text as mentioned in #82.5

Created #2683263: Update text for views_post_update_image_style_dependencies() wrong