ImageStyles can be deleted while having dependent configuration.

Hi,

I got the same issue with Drupal 8.0.0-beta10. If you've found any solution, please help.

Best regards,
Dmitry Nikitchenko.

I'm unable to reproduce this issue on HEAD with the steps supplied in the issue summary.

Fortunately I am able to reproduce this issue with the following steps:

1. Create a new image style.
2. Configure Article's image field to use the new image style.
3. Delete the new image style.
4. Add new article with an image.
5. View the article.

The error is caused by attempting to load a non-existing image style and using the result without checking if an image style was loaded. The attached patch fixes this.

Since the OP is on a windows system this is probably related to #2276705: Cannot create image styles on Wamp server (Apache/windows), which also causes a fatal error before this bug is triggered.

Attached patch adds tests.

I could reproduce the issue manually following the steps in the IS. The patch fixes the issue and adds tests. Also tested manually. RTBC

This is a config dependencies issue. When you delete the image style it should warn you that the image style is in use and then "fix" the entity display.

Unassigning since I'm not able to address this further in the coming week. I might be able to pick up work on this later.

Working on this.

Attached patch takes a new approach and adds config dependencies and adds dependency handling for when the image style is deleted.

Test is failing because it doesn't account for the newly added dependency. Adjusted the test accordingly.

Ignore previous patch, which was incorrectly formatted.

It would be nie to update the issue summary a bit here to explain what happens and what the solution is.

1. +++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
   @@ -19,37 +21,93 @@ class ImageStyleDeleteForm extends EntityDeleteForm {
   +  protected function isDependency() {
   

   Maybe we could have a better method name, this doesn't say much IMHO

2. +++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
   @@ -19,37 +21,93 @@ class ImageStyleDeleteForm extends EntityDeleteForm {
   +        return true;
   

   nitpick: upper case

3. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
   @@ -235,4 +238,11 @@ public function viewElements(FieldItemListInterface $items) {
   +  public function calculateDependencies() {
   

   Let's add {@inheritdoc}

4. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
   @@ -235,4 +238,11 @@ public function viewElements(FieldItemListInterface $items) {
   +    if (!empty($this->getSetting('image_style'))) {
   +      $this->addDependency('config', 'image.style.' . $this->getSetting('image_style'));
   

   We could do if ($image_style = $this->getSetting('image_style')) { and get rid of the second getSetting method

1. done
2. done
3. done
4. done

Will update the summary aswel.

+++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
@@ -46,10 +60,57 @@ public function form(array $form, FormStateInterface $form_state) {
+  protected function isConfigurationDependency() {

This doesn't look cheap. Is it? If not, is it worth storing in a property?

Also, why is this config entity the only one that needs to do this?

#2562107: EntityDisplayBase should react on removal of its components dependencies and #2565513: Replace ImageStyle removal workaround with the entity display native behavior are related.

Indeed. The problem is that dependencies on image styles are not declared, from e.g. the image formatter. Once we do that, the dependencies will be deleted, like everything else too.

Of course, the problem is that then we'll actually delete and that won't really fly with the replacement stuff. So we'll need find a solution for that. #2565513: Replace ImageStyle removal workaround with the entity display native behavior does't make sense, because the config dependency system has no way to do that kind of replacement.

So I think we should save the replacement ID to the image style as part of the confirm form. And then we need to delegate to formatter during on dependency removal to so the style replacement... fun. And then we shouldn't need a multi-step form.

IMHO, we should go back to the original scope in this issue, and leave the config dependency fix to the two issues that were filed in the meantime, see related issues.

Patch in #4 still applies, and does something sane that could be a stop gap solution while config dependency gets fixed. Besides, contrib/custom code can very well add render arrays for image_formatter or image_style themes outside of the entity display formatters with a wrong image style name... and the patch would prevent a fatal in that case.

Not RTBCing yet, opinions?

This needs to be handled via config dependencies. Will provide a patch, later today.

This have to benefit of #2562107: EntityDisplayBase should react on removal of its components dependencies and is an effect of that. IMO that is critical while now blocks 2 major issues (this & #2412569: Allow setting the auto-create bundle on entity reference fields with multiple target bundles).

Provided 2 patches. One to be committed but not tested and one just for testing, built on top of #2562107-33: EntityDisplayBase should react on removal of its components dependencies. No interdiff while it takes a total different approach.

I added 2 tests: one for UI and one for API (my first KernelBaseTestNG, yay!)

Not sure.

Fixed the kernel test.

This is the first use-case of #2562107: EntityDisplayBase should react on removal of its components dependencies.

Patch for review,

Improved issue summary.

Just a few nits/questions:

1. +++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
   @@ -16,29 +20,69 @@
   +      // No possible replacements, just announce the user that he needs to
   

   Instead of 'he', how about 'the user'?

2. +++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
   @@ -16,29 +20,69 @@
   +          //$description .= '<p>' . $this->t("@label:", ['@label' => $info['label']]) . '</p>';
   

   This can be removed?

3. +++ b/core/modules/image/src/Tests/ImageStyleDeleteTest.php
   @@ -0,0 +1,154 @@
   +namespace Drupal\image\Tests;
   +use Drupal\Core\Entity\Entity\EntityFormDisplay;
   

   Missing space here.

4. +++ b/core/modules/image/src/Tests/ImageStyleDeleteTest.php
   @@ -0,0 +1,154 @@
   +  /**
   +   * An image style.
   +   *
   +   * @var \Drupal\image\ImageStyleInterface
   +   */
   +  protected $style1;
   +
   +  /**
   +   * A second image style.
   +   *
   +   * @var \Drupal\image\ImageStyleInterface
   +   */
   +  protected $style2;
   +
   +  /**
   +   * A third image style.
   +   *
   +   * @var \Drupal\image\ImageStyleInterface
   +   */
   +  protected $style3;
   

   Do our coding standards allow for compacting this into:

   /**
    * Image styles.
    *
    * @var \Drupal\image\ImageStyleInterface
    */
   protected $style1, $style2, $style3;
   

Thank you for review @jhedstrom.

IMO this is almost Critical because it makes the UI crash, so I don't see us going in RC with this bug. I tentatively tag this with 'rc deadline'.

class ImageStyleDeleteForm extends EntityDeleteForm {
  ...
  /**
   * {@inheritdoc}
   *
   * @todo Convert the response to a nice rendered array once
   *   https://www.drupal.org/node/2575375 gets committed.
   */
  public function getDescription() {
    $description = '';
    ...

Also looking to this ugly string that we're concatenating in this implementation, I would love to see #2575375: Allow ConfirmFormInterface::getDescription() to alternatively return a renderable array in so that I can return there a nice renderable array.

Ops, the interdiff

FWIW, I don't know that it's critical (doesn't impact the entire system), nor that it need block RC--we can still fix bugs after RC.

+++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
@@ -43,7 +43,7 @@ public function getDescription() {
-      // No possible replacements, just announce the user that he needs to
+      // No possible replacements, just announce the user that the user needs to
       // update its formatters and widgets.

Sorry about this, but I didn't really think through in my previous suggestion. How about "No possible replacements, announce that the formatters and widgets need updating."

@jhedstrom, Yes, that sounds perfect. Thank you!.

Patch is looking good. Since the last test-only patch was uploaded, the tests here have changed quite a bit--could somebody upload just the tests again to illustrate the fix?

There are 2 tests that we provide here: one is a KernelTestBaseTNG that tests this from the API perspective and the other one tests the UI because the image style replacement is something that happens in the UI.

More nits, fixes, doc improvements.

Manual review, running the patch in #48 on simplytest.me

Steps:

1. added an article with an image
2. removed all the predefined image styles

My considerations:

1. the image style delete form is a bit confusing - it seems the information is there twice, once in 'formatter/widget' terminology, once in config entity terminology:
   
2. The action performed by selecting 'No replacement' from the Replacement style dropdown is disabling the field formatter or widget. I think this may be confusing for users. I would suggest to keep them enabled and simply reset the image style to 'None (original image)', which is the default option when adding a new image field formatter and the 'legacy' behavior from D7.

Will review code later.

@mondrake,

I would suggest to keep them enabled and simply reset the image style to 'None (original image)',

In this case we pretend that *we know* what the site builder wants. If the image is huge the site will be broken and maybe he'll not notice this. That's bad. By getting this message he is forced to make an informed choice: replace or disable the widgets/formatters. He can still go back and add a replacement image style.

Agree with the duplication of widgets/formatters but I like more what I've implemented in the scope of this issue. How to disable the standard output?

EDIT: Also, I want to show only widgets/formatters in the list but the standard info box shows all dependencies.

Alternatively, I can remove the custom list and let there a generic message. But that would be a UX lost compared to the patch.

In this case we pretend that *we know* what the site builder wants. If the image is huge the site will be broken and maybe he'll not notice this. That's bad. By getting this message he is forced to make an informed choice: replace or disable the widgets/formatters. He can still go back and add a replacement image style.

I tend to agree with this. Simply defaulting back to the original image might have very unintended consequences.

I think this is pretty close to RTBC now.

Thnaks @claudiu.cristea.

1. +++ b/core/profiles/standard/config/install/core.entity_view_display.node.article.teaser.yml
   @@ -7,6 +7,7 @@ dependencies:
   +    - image.style.medium
   

   The patch adds the new dependencies in default config, but don't we also need an update function ?

2. +++ b/core/modules/image/config/install/image.style.thumbnail.yml
   @@ -12,3 +12,4 @@ effects:
   +replacementID: null
   

   That feels inconsistent with the usual casing of properties in our config yamls.
   replacement_id ?
   replacement_style ?

   But well, more importantly see below :

3. +++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
   @@ -47,9 +90,61 @@ public function form(array $form, FormStateInterface $form_state) {
   +    // If a replacement has been selected, save it in the entity to be used
   +    // later, when resolving dependencies.
   +    if (!empty($replacement = $form_state->getValue('replacement'))) {
   +      ImageStyle::load($this->entity->id())
   +        ->set('replacementID', $replacement)
   +        ->save();
   +    }
        parent::submitForm($form, $form_state);
   

   Ooh. That seems a bit wicked...

   If I get things right, this "replacementID" info is only placed here and saved back to the config, so that it's be available in the various implementations of onDependencyRemoval(), just before we delete that config anyway.

   I'd tend to think that information belongs somewhere else than in the image_style entity itself, in something like State, or maybe in a readable static registry somewhere (since the information is emitted and consumed within the same request anymay). At any rate, it feels a bit wrong to clutter the config shchema and all the config records with a property that is only ever non-null and useful during a couple milliseconds within one request in case the entity gets deleted ?

4. +++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
   @@ -47,9 +90,61 @@ public function form(array $form, FormStateInterface $form_state) {
   +          $component_type = $context == 'view' ? 'image' : 'image_image';
   +          $setting = $context == 'view' ? 'image_style' : 'preview_image_style';
   

   It feels a bit weird that ImageStyleDeleteForm hardcodes behavior on a specific formatter and widget, with knowledge of the specific setting name involved each time.

   Any contrib/custom formatter that also uses image styles will be left out here.

   If this is about "knowing which displays have widgets/formatters that depend on a given image style", don't we have all the info by calling the widget's calculateDependencies() ?

   (to avoid instanciating every widget and formatter plugin in every display for every bundle of every entity type, we could first check the dependencies of the EntityDisplays ?)

5. Also, yeah, the UI aspect is not trivial. No real suggestion atm, but the current does clash a bit with the regular "CMI dependencies" info...

All in all, I have a feeling we're inventing a system specific to image styles, to address a case that is more generic than that (allowing replacements in dependant config when a dependency is deleted). The "image style" use case is a bit prominent, but not really singular conceptually ?

IMHO, this is critical. Tentatively setting like that, please revert if does not qualify.

Testing RC1 on simplytest.me without this patch:

1. add an Article, uploading an image
2. remove all the predefined image styles, leaving 'No replacement, just delete'
3. go to the main page

you get the error

Fatal error: Call to a member function getCacheTags() on a non-object in /home/dfvjn/www/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php on line 200

which makes the entire home page not accessible. Recovering via UI is non-trivial because you have to re-add the deleted image styles with the exact machine names ('large', 'medium', 'thumbnail'), provided you know how to access admin UI typing the url in the browser. Not easy unless you are really know what to do.

---------------

@claudiu.cristea re #52:

In this case we pretend that *we know* what the site builder wants

Fine, but with your reasoning then we are pretending that the site builder is OK to disable the formatter :)
I think it's a matter of opinions - do we prefer to see an unformatted image, or no image at all? Couple of more considerations:

• D7 behaviour: first of all, it's not possible to delete the predefined styles in D7, they're predefined in code and can be overridden, not deleted. So this is not a D7 case. Second, if you change the formatter to use a style of your own, then delete the style with no replacement, then the original image is shown.
• In any case, in D8 I can imagine that this operation would rarely be done directly on productive sites, at least large ones - more probably will be done in development, then deployed with config exports.

1. +++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
   @@ -16,29 +20,68 @@
   +    if ($this->getAffectedComponents()) {
   ...
   +      foreach ($this->getAffectedComponents() as $context => $info) {
   

   In case you want, you can assign a variable and use that in the foreach.

2. +++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
   @@ -16,29 +20,68 @@
   +    $description .= '<p>' . $this->t("All images that have been generated for this style will be permanently deleted.") . '</p>';
   +
   +    return $description;
   

   Doens't concat strings like that lead to escaping?

3. +++ b/core/profiles/standard/config/install/core.entity_form_display.node.article.default.yml
   @@ -6,6 +6,7 @@ dependencies:
   diff --git a/core/profiles/standard/config/install/core.entity_form_display.user.user.default.yml b/core/profiles/standard/config/install/core.entity_form_display.user.user.default.yml
   
   diff --git a/core/profiles/standard/config/install/core.entity_form_display.user.user.default.yml b/core/profiles/standard/config/install/core.entity_form_display.user.user.default.yml
   index 107d363..466b6e0 100644
   
   index 107d363..466b6e0 100644
   --- a/core/profiles/standard/config/install/core.entity_form_display.user.user.default.yml
   
   --- a/core/profiles/standard/config/install/core.entity_form_display.user.user.default.yml
   +++ b/core/profiles/standard/config/install/core.entity_form_display.user.user.default.yml
   
   +++ b/core/profiles/standard/config/install/core.entity_form_display.user.user.default.yml
   +++ b/core/profiles/standard/config/install/core.entity_form_display.user.user.default.yml
   @@ -3,6 +3,7 @@ status: true
   
   @@ -3,6 +3,7 @@ status: true
    dependencies:
      config:
        - field.field.user.user.user_picture
   +    - image.style.thumbnail
      module:
   

   Do we need an update path for them, which could be to simply resave all the affected configuration?

Trying to comment some of concerns before jumping to coding:

@yched, #55:

I'd tend to think that information belongs somewhere else than in the image_style entity itself, in something like State, or maybe in a readable static registry somewhere (since the information is emitted and consumed within the same request anymay). At any rate, it feels a bit wrong to clutter the config shchema and all the config records with a property that is only ever non-null and useful during a couple milliseconds within one request in case the entity gets deleted ?

Yes, I felt this from the very first moment but then I tried to stick as much as I could to the current implementation of ImageStyle config. But now, looking in the code more I think that we can drop the replacementID. Even that is part of ImageStyle interface that, right now in HEAD, stores nothing -- it's not used on existing sites. It's not even part of schema. So removing it from the config entity it would be almost trivial.

Now the question is: do we need to persist the replacement ID somewhere, for example in a state? I think storing in memory should be enough. But what about the API? What will happen with an hypothetical existing module doing this, based on HEAD code?

$style = ImageStyle::load('foo');
$style->set('replacementID', 'bar');
$style->delete();

This will fail even with the current patch. To keep such a BC we have to override the ::delete() in ImageStyle and force a save if 'replacementID' is not NULL:

public function delete() {
  if ($this->getReplacementID()) {
    $this->save();
  }
  parent::delete();
}

The question is: Do we need to support BC for such an super edge-case? I would say NO. That's why, I think, we can change the replacementID to a static storage that we can read during the onDependencyRemoval() phase. We are not implementing a new feature here, we are fixing a critical bug. We need a decision here.

Also, yeah, the UI aspect is not trivial. No real suggestion atm, but the current does clash a bit with the regular "CMI dependencies" info...

Yes. But I still think that the site builder needs to know explicitly who are the formatters/widgets affected by this operation. This is part of the delete confirm process and I think that's why it should be there. An idea would be to rewrite the addDependencyListsToForm() to subtract the widgets/formatters from there, leaving in CMI dependencies only the rest.

All in all, I have a feeling we're inventing a system specific to image styles, to address a case that is more generic than that (allowing replacements in dependant config when a dependency is deleted). The "image style" use case is a bit prominent, but not really singular conceptually ?

This is so true, but wouldn't we introduce a UX regression by dropping the replacement feature? IMO this would work as a simple delete confirmation form where the site builder is warned about disabling those widgets/formatters. But the UX regression?

@mondrake, #56:

Fine, but with your reasoning then we are pretending that the site builder is OK to disable the formatter :)

I think disabling is an action that makes the site builder more aware about what he's doing. In fact disabling is the native action if no specific action is implemented.

@dawehner, #57:

Doens't concat strings like that lead to escaping?

I'm not sure I understood. (/me admits that is not up-to-date with the latest changes to t(), TranslatableMarkup, SafeString & Co.)

Other aspects:

+++ b/core/modules/image/src/Entity/ImageStyle.php
@@ -53,6 +53,7 @@
@@ -131,14 +132,6 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti

@@ -131,14 +132,6 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
     foreach ($entities as $style) {
       // Flush cached media for the deleted style.
       $style->flush();
-      // Check whether field settings need to be updated.
-      // In case no replacement style was specified, all image fields that are
-      // using the deleted style are left in a broken state.
-      if (!$style->isSyncing() && $new_id = $style->getReplacementID()) {
-        // The deleted ID is still set as originalID.
-        $style->setName($new_id);
-        static::replaceImageStyle($style);
-      }
     }
   }

I wonder if the remaining $style->flush() should not go ImageStyleStorage::resetCache()? (the class doesn't exist right now).

I think it is acceptable to move the image replacement style to state. Note I do not think we should remove ImageStyle::getReplacementId() - this should just call state.

Re the getDescription and concatenation of markup... this works because the result is XSS admin filtered...$form['description'] = array('#markup' => $this->getDescription());

I don't think think we should be listing individual components on the form because (a) we already have the config dependency list (b) this adds unnecessary complexity and it not necessary for fixing the bug. In the RC phase we should be looking to do the smallest possible fix and try ensure that every eventuality is tested.

I've discussed this with @WimLeers, @dawehner, @jibran and @gabor and we agreed that this does not meet the definition of critical since the steps to create the error are not typical. However, that does not mean that this issue should not land during RC. Given that it creates a difficult to remove from error and we have missing configuration dependencies (which makes the system less robust and deployable) I'm tagging this with "rc target" which means once it is rtbc it is eligible for commit during RC.

Tracked things back a little, ImageStyle::getReplacementId() is a one-off construct specific to image styles, introduced in #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity) (early 2013) to preserve the one-off construct in D7 image styles that allowed code to "replace" an image style being replaced.

D7 :

function image_style_delete($style, $replacement_style_name = '') {
  (...)
  // Let other modules update as necessary on save.
  $style['old_name'] = $style['name'];
  $style['name'] = $replacement_style_name;
  module_invoke_all('image_style_delete', $style);
}

So getReplacementID() is the remnant of a completely separate and parallel API next to the more general "config dependency" system that D8 gained since then. Sad, but yeah, now is probably not the time to remove it :-/

Using state for this means :
- ImageStyle::getReplacementID() reads from state,
- since we don't have a corresponding setReplacementId() setter, ImageStyle needs to override ::set() to catch writes to 'replacementID' and store it to state.
- in order to clean up, since we use a persistent storage, ::postDelete() probably needs to remove the corresponding state data too.

That means quite a few unnecessary writes to a persistent storage, but it's not triggered too often...
Wondering if we could use an in-memory property in an ImageStyleStorage handler for that, with associated read/write methods there ?
(ImageStyle config entities currently don't have a dedicated storage handler of their own, but they could ?)

I used state and memory for replacement. Curious.

Interdiff is not too relevant.

Thanks @claudiu.cristea. More detailed review soon, for now just :

1. +++ b/core/modules/image/image.install
   @@ -61,3 +61,15 @@ function image_requirements($phase) {
   +function image_update_8001() {
   +  $config_factory = \Drupal::configFactory();
   +  foreach (['form', 'view'] as $context) {
   +    foreach ($config_factory->listAll("core.entity_{$context}_display.") as $name) {
   +      $config_factory->getEditable($name)->save();
   +    }
   +  }
   +}
   

   If this uses APIs, this should be a post_update ?

2. +++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
   @@ -47,9 +67,46 @@ public function form(array $form, FormStateInterface $form_state) {
   +  protected function hasAffectedComponents() {
   +    if (isset($this->affectedComponents)) {
   +      return $this->affectedComponents;
   +    }
   

   Not sure I see the interest of the property, is this really called several times in a request ?

About how we track replacements :

+++ b/core/modules/image/image.services.yml
@@ -13,3 +13,8 @@ services:
+  image.keyvalue.memory:
+    class: Drupal\Core\KeyValueStore\KeyValueMemoryFactory
+  image.state:
+    parent: state
+    arguments: ['@image.keyvalue.memory']

+++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
@@ -47,9 +67,46 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+   \Drupal::service('image.state')
+     ->set("image.style.replacement.{$this->entity->id()}", $replacement);

+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
@@ -226,4 +227,40 @@ public function onDependencyRemoval(array $dependencies) {
+   $state = \Drupal::service('image.state');
+   if ($replacement_id = $state->get("image.style.replacement.$name")) {

Yeah, this ends up being a slightly convoluted way to just to store a string between those two points in the callstack...

If we're adding a new service, we might at well define a class with a simple getter and setter without going through the state API - or simply put those methods in the 'image_style' storage handler ?

Curious to hear what others think :-)

@yched, thank you for review.

#63.1: You're right. Moved but still keeping at config level.

#63.2: Good catch. Initially that was used twice. I didn't check.

#64. First I jumped to drupal_static() but that is in a retire process even it's not marked yet as deprecated. I went with a storage class. I wanted also to remove ImageStyle::postDelete() and move the flush operation in ImageStyleStorage::resetCache(). Unfortunately resetCache() is taking only IDs as arguments, not the entire object and in that stage the image style is already deleted, so I cannot load() it. I don't understand the reason why resetCache() is taking only the IDs as arguments while in the moment when it is called the complete list of objects is available.

Small fixes.

Grrrr. Forgot to add an empty line at the end of ImageStyleStorage.

Thanks @claudiu.cristea, looks way nicer IMO !

As an additional step, I'm wondering whether it would make sense to consolidate the "delete style and specify a replacement" capability direclty in ImageStyle::delete() :

public function delete($replacement = NULL) {
  if ($replacement) {
    $this->entityManager()->getStorage($this->entityTypeId)->setReplacementId($this->id(), $replacement);
  }
  parent::delete();
}

Thoughts ?

As an additional step, I'm wondering whether it would make sense to consolidate the "delete style and specify a replacement" capability direclty in ImageStyle::delete()

I don't see a use-case in core for this. The UI deletion goes to the standard entity delete form, so we don't call ->delete() directly. Other cases should know about this modified interface. BTW, we don't follow the interface, but it works while the parameter is optional.

The UI deletion goes to the standard entity delete form, so we don't call ->delete() directly.

Right, good point. Never mind then.

Last code nitpicks :

1. +++ b/core/modules/image/src/Entity/ImageStyle.php
   @@ -128,17 +122,10 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
      public static function postDelete(EntityStorageInterface $storage, array $entities) {
   

   I think we should still cleanup the replacement mapping in the storage once the style has actually been deleted. Not that it matters much since the mapping is not preserved past the request, but still cleaner :-)

   That might mean adding a removeReplacementId() (maybe overkill), or supporting NULL in get/setReplacementId()

2. +++ b/core/modules/image/src/ImageStyleStorage.php
   @@ -0,0 +1,35 @@
   +  public function getReplacementId($name) {
   +    return array_key_exists($name, $this->replacement) ? $this->replacement[$name] : NULL;
   +  }
   

   Detail : The array_key_exists() here is not really needed, could be a cheaper isset($this->replacement[$name]), since if it's set to NULL we will return NULL anyway...

3. +++ b/core/modules/image/src/Entity/ImageStyle.php
   @@ -380,7 +367,7 @@ public function addImageEffect(array $configuration) {
      public function getReplacementID() {
   -    return $this->get('replacementID');
   +    return NULL;
      }
   

   Well, this is almost as BC-breaking as removing the method from ImageStyleInterface, right ? :-)

   I'm frankly skeptical that anything would actually break if we did remove it, but if we want to be extra conservative and keep it, then this should call storage::getReplacementId() ? (additionally we could at least mark it @deprecated)

4. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
   @@ -226,4 +227,39 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +    /** @var \Drupal\image\ImageStyleInterface $style */
   +    $style_id = $this->getSetting('image_style');
   +    if ($style_id && $style = ImageStyle::load($style_id)) {
   

   The type hint is one line too high :-p

5. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
   @@ -226,4 +227,39 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +    $name = $this->getSetting('image_style');
   

   Minor : var name mismatch, it's called $style_id in the previous method :-)

   Also, those two methods could use a small comment summarizing what is being done, the code is not super obvious to read.

   (and same remarks for the duplicated code in ImageWidget)

6. +++ b/core/modules/image/src/Tests/ImageAdminStylesTest.php
   @@ -446,6 +447,11 @@ function testConfigImport() {
   +    // Break first the entity view display dependency to image style, to avoid
   +    // import dependency validation error.
   

   Grammar is a bit off.

   "Remove the image field from the display, to avoid a dependency error during import" ?
   (just a proposal, not sure if that's what the test is actually doing here)

7. +++ b/core/modules/image/src/Tests/ImageFieldTestBase.php
   @@ -67,8 +67,10 @@ protected function setUp() {
   +   *   A list of formatter settings that will be added to the formatter defaults.
   

   80 chars :-)

Was about to get back to the "UI information duplication" part, but looks like it's been removed in #61. Honestly, that might be shortest path right now :-)

Also : the "preview image style" feature in the Widget is actually optional. So if no replacement it provided, the widget could be kept without a preview, which would be a frendlier fallback than hiding the widget and forbidding file upload altogether.
(of course, it's different for formatters, a formatter without an output style is just useless)

Plus :

+++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
@@ -18,27 +20,38 @@ class ImageStyleDeleteForm extends EntityDeleteForm {
+          '#markup' => $this->t("There are components relying on %style image style. You may select another style to replace it in all components settings. If you don't provide a replacement, those components will be disabled. In this case you'll need to revisit the form and view displays an reconfigure the widgets and formatters.", ['%style' => $this->entity->label()]),

s/an/and ;-)

Thank you @yched for your detailed review.

#72.1: I did that already in a temporary patch (never posted) but it seamed to me kinda over-engeneering. Added back.
#72.2: OK.
#72.3. OK. The deprecated statement was already added in the interface. Just changed a bit.
#72.4, 5: OK. Cleaned a little. Also the formatter has the image style storage already injected, so I used that. Unfortunately the widget misses that.
#72.6, 7: OK.
#73: Excellent catch. In fact disabling that preview is non-invasive but setting the formatter to "original image" seems to me invasive. So the widget remain with no preview, the formatter gets disabled.
#74: OK.

Also working on formatter I discovered #2588553: Image widget summary is lying when the image preview is disabled :)

Thanks @claudiu.cristea. I thought I'd push RTBC, unfortunately I missed that earlier :

+++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
@@ -18,27 +20,38 @@ class ImageStyleDeleteForm extends EntityDeleteForm {
+          '#markup' => $this->t("There are components relying on %style image style and will be disabled. You'll need to revisit the form and view displays and reconfigure the widgets and formatters.", ['%style' => $this->entity->label()]),

That first sentence is not english ;-)

Also, I think we tend to avoid contractions like "we'll, you'll, don't..." in our code comments, so I guess that applies all the more to UI text as well ?

That first sentence is not english ;-)

Well, I cannot help more here, my English is poor.

Yeah, not that easy :-/

Suggestions :

- when there are possible replacement styles :
"The %style image style you are about to delete is used in existing components. You may select another image style to replace it. If no replacement style is selected, the corresponding components will be disabled, and might need manual reconfiguration."

- when there are no possible replacement styles :
"The %style image style you are about to delete is used in existing components. Those components will be disabled, and might need manual reconfiguration."

?

Suggestions / adjustments welcome :-)

Right, sounds better. This is English :)

How about replacing the word "components", which (I think) is a little bit too abstract and used more internally, with "formatters and widgets"? I mean, it seems that "component" is more an API terminology while formatter/widget is more descriptive to a site builder.

...that meaning:

"The %style image style you are about to delete is used in existing formatters and widgets. You may select another image style to replace it. If no replacement style is selected, the corresponding formatters and widgets will be disabled, and might need manual reconfiguration."

and

"The %style image style you are about to delete is used in existing formatters and widgets. Those formatters and widgets will be disabled, and might need manual reconfiguration."

The problem is now, that staring with #75 we are disabling only the widgets :(

Yeah, I was not sure we really expose "formatters / widgets" in the UI, but we do.
I'd say "existing *field* formatters and widgets", just to be a bit clearer.

The problem is now, that staring with #75 we are disabling only the widgets :(

Yep, the sentence about disabling should mention only formatters I guess.

OK.

Ok, let's bring it to the right eyes :-)

Great work, @claudiu.cristea!

Just a couple of things:

1. I wonder whether it would still make sense to have this change to template_preprocess_image_style() from the patch in #4:

+++ b/core/modules/image/image.module
@@ -270,22 +270,22 @@ function image_style_options($include_empty = TRUE) {
  *   - attributes: Associative array of attributes to be placed in the img tag.
  */
 function template_preprocess_image_style(&$variables) {
-  $style = ImageStyle::load($variables['style_name']);
-
   // Determine the dimensions of the styled image.
   $dimensions = array(
     'width' => $variables['width'],
     'height' => $variables['height'],
   );
 
-  $style->transformDimensions($dimensions, $variables['uri']);
+  if ($style = ImageStyle::load($variables['style_name'])) {
+    $style->transformDimensions($dimensions, $variables['uri']);
+  }
 
   $variables['image'] = array(
     '#theme' => 'image',
     '#width' => $dimensions['width'],
     '#height' => $dimensions['height'],
     '#attributes' => $variables['attributes'],
-    '#uri' => $style->buildUrl($variables['uri']),
+    '#uri' => $style ? $style->buildUrl($variables['uri']) : $variables['uri'],
     '#style_name' => $variables['style_name'],
   );
 

and the test too. Contrib/custom may pass '#theme' => 'image_style' arrays outside of formatters/widgets, and if the style id is non existing, we will still get a fatal. Workaround is for contrib/custom to try and load the image style to check its existence before building the render array, but that seems a duplication.

2. Testing manually, I noticed that if you first disable and then renable the image formatter, the visual result differ, with the comments section getting rendered inline with the image. It's not related to this patch though.

Before disabling:

After disabling and re-enabling:

@mondrake,

I wonder whether it would still make sense to have this change to template_preprocess_image_style()

I don't like how this is now in HEAD, neither in patch. I don't think the theme layer should *load* objects. That should have been done before. IMO this is a bad design. I don't know what is the solution here. I feel that the whole image style object should be passed to the theme layer, not only the name. But probably it's to late for that in D8?

EDIT: And yes, the backend (in this case the modules) should be in business of loading, validating the objects.

#85:

I feel that the whole image style object should be passed to the theme layer, not only the name.

Yes that would make a lot of sense.

But probably it's to late for that in D8?

Maybe in 8.1 we can introduce a $variable['image_style'] in parallel to $variable['style_name'] (that should be deprecated). Then $variable['style_name'] could become (in 8.1) a fallback if no image style object is passed. But that's for later...

@mondrake, I opened an issue for the image style loading stuff. I would be very glad to get some feedback there #2589847: Stop loading image_style in the theme layer.

1. +++ b/core/modules/image/image.post_update.php
   @@ -0,0 +1,18 @@
   +function image_post_update_image_style_dependencies() {
   +  $config_factory = \Drupal::configFactory();
   +  foreach (['form', 'view'] as $context) {
   +    foreach ($config_factory->listAll("core.entity_{$context}_display.") as $name) {
   +      $config_factory->getEditable($name)->save();
   +    }
   +  }
   +}
   
   +++ b/core/modules/image/src/Tests/Update/ImageUpdateTest.php
   @@ -0,0 +1,55 @@
   +class ImageUpdateTest extends UpdatePathTestBase {
   

   This is not testing this code. It is testing that system_post_update_recalculate_configuration_entity_dependencies() has run. You need to load and resave the configuration entity for dependency recalculation. I think we need to create a dump for each RC to make testing simpler.

2. +++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
   @@ -18,27 +20,38 @@ class ImageStyleDeleteForm extends EntityDeleteForm {
   +          '#markup' => $this->t("The %style image style you are about to delete is used in existing field formatters and widgets. You may select another image style to replace it. If no replacement style is selected, the corresponding field formatters will be disabled, and might need manual reconfiguration.", ['%style' => $this->entity->label()]),
   

   , and might need<code> no need for the comma. Could this be the <code>#description on <code>$form['replacement']. Also given the way config entities and plugins work together a contrib config entity could depend on image styles and implement the replacement logic. I think limiting this to existing field formatters and widgets is wrong but, if we do that we need to be consistent and the corresponding field formatters will be disabled should be the corresponding field formatters and widgets will be disabled

3. +++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
   @@ -18,27 +20,38 @@ class ImageStyleDeleteForm extends EntityDeleteForm {
   +          '#empty_option' => $this->t('- No replacement (disable widgets/formatters) -'),
   

   Again this might well be more than just widgets and formatters.

4. +++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
   @@ -18,27 +20,38 @@ class ImageStyleDeleteForm extends EntityDeleteForm {
   +          '#markup' => $this->t("The %style image style you are about to delete is used in existing field formatters and widgets. Those formatters will be disabled, and might need manual reconfiguration.", ['%style' => $this->entity->label()]),
   

   Same comma issue as above and also same issue that this might delete or fix other configuration entities.

5. +++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
   @@ -47,9 +60,44 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $displays = array_merge(array_values(EntityViewDisplay::loadMultiple()), array_values(EntityFormDisplay::loadMultiple()));
   

   Oh I see - why only displays? This replacement logic could be used by any configuration entity that depends on image styles to fix itself.

6. +++ b/core/modules/image/src/ImageStyleStorage.php
   @@ -0,0 +1,42 @@
   +  /**
   +   * Image style replacement memory storage.
   +   *
   +   * @var array
   +   */
   +  protected $replacement = [];
   

   I don't think "memory storage" is very meaningful. I think it is worth expanding the comment to save this is not saved to the individual entity but used during deleting to do a replacement. Maybe just an @see to the methods is all the documentation required.

7. +++ b/core/modules/image/src/ImageStyleStorageInterface.php
   @@ -0,0 +1,54 @@
   +   * replacing the deleted style in other configuration entities (widgets,
   +   * formatters) that are depending on the image style being deleted.
   

   Atm it is only widgets and formatters that do this - but it would be possible for any other configuration entity that uses image styles to do this.

8. +++ b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
   @@ -273,4 +274,49 @@ public static function validateRequiredFields($element, FormStateInterface $form
   +        // If there's no replacement or the replacement is invalid, disable the
   +        // image preview.
   +        else {
   +          $this->setSetting('preview_image_style', '');
   +        }
   

   It's good to see that this is tested - nice work.

9. +++ b/core/modules/image/src/Tests/ImageStyleDeleteTest.php
   @@ -0,0 +1,121 @@
   +  /**
   +   * Tests image style deletion messages.
   +   */
   +  public function testDeletionMessages() {
   ...
   +  /**
   +   * Tests the deletion of image styles.
   +   */
   +  public function testDelete() {
   

   Let's combine these tests. Since message testing and doing the deletes are essentially the same activity - why install Drupal twice?

@alexpott :

- #88 2. 3. 4. The issue is that what will happen if no replacement is specified, depends on each config domain logic and what it does with the image style.

E.g image formatters get disabled, but image upload widgets are kept working, just without a preview thumbnail (since that is an optional widget setting anyway - see #73). Other config that use the image style might react in other ways.

- #88 5. "why only entity displays" : I guess expanding that would mean loading *all* config and find which ones currently depend on the image style ? Is that reasonable ?

[edit : hm, I guess that's ConfigManager::findConfigEntityDependentsAsEntities() ? We would then call it twice on this form ?]

Not really clear what to review here, the message?

@Bojhan we need to review how the user selects a replacement image style when deleting an image style.

Also #2592665: Create RC1 database dumps has landed so now we can write a proper upgrade path test without previous upgrades getting in the way.

#88.1: Fixed.

#882, 3, 4, 5: @yched replied in #89.

#88.6, &: Added/fixed docs.

#88.9: OK, there's only one test now.

Ouch! Forgot the patch.

Tested manually, adding some image styles and then deleting the Thumbnail style and replacing it with the newly created styles. Formatters get disabled when style is deleted with no replacement, and widget gets set to not display preview.

+++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
@@ -18,27 +20,36 @@ class ImageStyleDeleteForm extends EntityDeleteForm {
+          '#empty_option' => $this->t('- No replacement (disable widgets/formatters) -'),

I would just say '- No replacement -', there's enough information in the rest of the form description already.

Other than that RTBC from my point of view - I think only @alexpott can give feedback to replies to #88.2-5

EDIT: if we implement onDependencyRemoval in ImageFormatter and ImageWidget, do we still need ImageStyle::replaceImageStyle? Maybe I haven't understood.

@mondrake thank you.

if we implement onDependencyRemoval in ImageFormatter and ImageWidget, do we still need ImageStyle::replaceImageStyle? Maybe I haven't understood.

That is still needed when renaming a style name, see ImageStyle::postSave().

Thank you. This also solves #2631140: After Deleting Thumbnail Image Style, Saving to an Image Field Throws Fatal Error.

Added before/after screenshots.

That is still needed when renaming a style name...

Ahh right. Pity that we do not have a method to react to dependencies updates, then. You have gone a long way to decouple ImageStyle code from formatters and widgets dependencies on delete, but it's still having dedicated code for these entities on update...

1. +++ b/core/modules/image/image.post_update.php
   @@ -0,0 +1,23 @@
   + * Post update functions for Image.
   

   Nit: s/Post update/Post-update/

2. +++ b/core/modules/image/image.post_update.php
   @@ -0,0 +1,23 @@
   + * Stores the image style dependencies into form and view display entities.
   

   "stores into" sounds very strange.

3. +++ b/core/modules/image/image.post_update.php
   @@ -0,0 +1,23 @@
   +  // Merge view and form displays together. Use array_values() to avoid key
   

   "merge together" -> "merge"

4. +++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
   @@ -18,27 +20,36 @@ class ImageStyleDeleteForm extends EntityDeleteForm {
   +    return $this->t("All images that have been generated for this style will be permanently deleted.");
   

   Why change from single to double quotes? Let's keep it the same as before.

5. +++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
   @@ -18,27 +20,36 @@ class ImageStyleDeleteForm extends EntityDeleteForm {
   +    // message and, if case, the image style replacement select.
   

   "and if case"?

6. +++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
   @@ -18,27 +20,36 @@ class ImageStyleDeleteForm extends EntityDeleteForm {
   +      // optionally pickup a replacement.
   

   s/pickup/pick up/

7. +++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
   @@ -47,9 +58,44 @@ public function form(array $form, FormStateInterface $form_state) {
   +    // If a replacement has been selected, save it in image style storage to be
   +    // used later, in the same request, when resolving dependencies.
   

   This sentence sounds kind of strange.

8. +++ b/core/modules/image/src/ImageStyleInterface.php
   @@ -17,8 +17,12 @@
   -   * @return string
   -   *   The name of the image style to use as replacement upon delete.
   +   * @return null
   +   *
   +   * @deprecated in Drupal 8.0.x, will be removed in Drupal 9.0.x. Use
   +   *   \Drupal\image\ImageStyleStorageInterface::getReplacementId() instead.
   

   This does not make sense.

   Returning NULL means no @return is necessary.

   But also, it's just deprecated, so actually it should still return this string.

9. +++ b/core/modules/image/src/ImageStyleStorage.php
   @@ -0,0 +1,48 @@
   +   * @var array
   

   Is this string[]?

@Wim Leers, thank you for review.

#100 seems addressed to me. Back to RTBC.

Sorry, I did not mean to remove the 'rc target' tag. Added back.

@effulgentsia pointed out the issue summary mentions a fatal error, but I don't see that mentioned anywhere else, is that still the case?

#104: updated IS with steps from comment #56 and from #2631140: After Deleting Thumbnail Image Style, Saving to an Image Field Throws Fatal Error.

bot error

+++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
@@ -18,27 +20,36 @@ class ImageStyleDeleteForm extends EntityDeleteForm {
+    if ($this->hasAffectedComponents()) {

Making this check here means that only if there is an affected component (ie an entity display) will you get a replacement image style. This is a change of behaviour that I don't think is correct. I think we should always get a replacement style and then other contrib config entities can use the same system.

@alexpott, OK. Should ::hasAffectedComponents() return TRUE by scanning all dependencies or should it be dropped? If we'll drop it, that might be confusing for some site builders. Because, if there's no config depending on that style on the system, they might think that they are replacing something. But their action would be senseless because the replacement is volatile.

@claudiu.cristea I guess it could check all dependencies... ConfigManager::findConfigEntityDependents() can do that. However the replacement question has always been volatile. So I think the description can be improved to encompass this.

@alexpott, @yched, I need your feedback based on #110 and #112.

Note: I didn't use ConfigManager::findConfigEntityDependents() for 2 reasons:

1. We call ConfigManager::findConfigEntityDependents() twice on the same page, but we already have the list, later in the form building phase.
2. I need only the dependencies that are updating (not deletions) because only those can benefit from image style replacement.

I admit that I don't like the way I'm calling $this->addDependencyListsToForm() earlier nor the way I'm testing for dependencies by checking the #access key. I think we should split ConfigDependencyDeleteFormTrait::addDependencyListsToForm() in 2 parts the existing method that only adds the form elements and a new protected method that computes and statically caches the list of dependencies. Then we call the new protected method from addDependencyListsToForm(). Also we call that directly in ImageStyleDeleteForm::form(). When is called 2nd time in the request to add the form elements, will retrieve the list from the static cache.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyDeleteFormTrait.php
@@ -45,6 +45,11 @@
+    // If dependencies were aleready added to the form, exit here.
+    if (isset($form['entity_updates']) || isset($form['entity_deletes'])) {
+      return;
+    }

I'm not a huge fan of adding this... let's just override buildForm() in ImageStyleDeleteForm instead of form() - then we don't have to do this. BUT actually that's not going to work either. This is because in order to work out that it can fix something it needs the replacement ID so we need the replacement ID before showing the user the list of what can and can not be fixed. To me it looks as though we must be missing some test coverage because I can't see how the latest patch is actually working.

@alexpott,

This is because in order to work out that it can fix something it needs the replacement ID so we need the replacement ID before showing the user the list of what can and can not be fixed. To me it looks as though we must be missing some test coverage because I can't see how the latest patch is actually working.

I think this works as expected because, if there's no replacement, the removal gets resolved at display level, in EntityDisplayBase::onDependencyRemoval(). I did that but now, I'm confused.

Discussed with @alexpott on IRC about providing the image style replacement select only when there are configurations depending on that image style. The conclusion was to not limit the display of the replacement select based on that. In this way we let any contrib module to benefit on the replacement and implement its own logic to update other configurations. So, the behaviour doesn't change. Compared to HEAD, I'm only suppressing the select element when there's no replacement at all on the system. But this makes a lot of sense in terms of UI & UX.

@claudiu.cristea so I understand in #116 we are removing a restriction that was looking only at dependent image widget/formatters during the deletion phase, and with this any config entity that has a dependency on an image style will be able to react to its deletion via its own implementation of ::onDependencyRemoval?

Couple of points below:

+++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
@@ -25,20 +32,28 @@ public function getQuestion() {
   public function getDescription() {
-    return $this->t('If this style is in use on the site, you may select another style to replace it. All images that have been generated for this style will be permanently deleted.');
+    if (count($this->getReplacementOptions()) > 1) {
+      return $this->t('If this style is in use on the site, you may select another style to replace it. All images that have been generated for this style will be permanently deleted. If no replacement style is selected, the dependent configurations might need manual reconfiguration.');
+    }
+    return $this->t('All images that have been generated for this style will be permanently deleted. The dependent configurations might need manual reconfiguration.');
   }

+++ b/core/modules/image/src/ImageStyleInterface.php
@@ -17,8 +17,14 @@
+   * @deprecated in Drupal 8.0.x, will be removed in Drupal 9.0.x. Use

@mondrake, we are not checking anymore for dependencies. There are no more a), b) and c). Regardless if this style has dependencies or not, we are just a) provide the replacement, if there are or b) provide only an explanation and allow the deletion. Also we've discussed to keep as much as we can the UI strings. So the a) string is the same as in HEAD. The other one is a new string.

#119 ah all right then. Tested manually on simplytest.me, everything looks right. Updated the screenshots in the IS to reflect latest changes. RTBCing so to bring to the right eyes :)

I believe the minor change to the deprecation notice suggested in #117 can also be done on commit.

Thanks

Bot problem. Also the screenshots got lost :(, re-adding.

This was a bot failure.

@claudiu.cristea thanks for sticking with this - really nice work. Committed f986c6a and pushed to 8.0.x and 8.1.x. Thanks!

diff --git a/core/modules/image/src/Form/ImageStyleDeleteForm.php b/core/modules/image/src/Form/ImageStyleDeleteForm.php
index 46b6fa2..0638369 100644
--- a/core/modules/image/src/Form/ImageStyleDeleteForm.php
+++ b/core/modules/image/src/Form/ImageStyleDeleteForm.php
@@ -84,4 +84,5 @@ protected function getReplacementOptions() {
     }
     return $this->replacementOptions;
   }
+
 }
diff --git a/core/modules/image/src/ImageStyleInterface.php b/core/modules/image/src/ImageStyleInterface.php
index 35db87d..39bb583 100644
--- a/core/modules/image/src/ImageStyleInterface.php
+++ b/core/modules/image/src/ImageStyleInterface.php
@@ -21,7 +21,7 @@
    *   The replacement image style ID or NULL if no replacement has been
    *   selected.
    *
-   * @deprecated in Drupal 8.0.x, will be removed in Drupal 9.0.x. Use
+   * @deprecated in Drupal 8.0.x, will be removed before Drupal 9.0.x. Use
    *   \Drupal\image\ImageStyleStorageInterface::getReplacementId() instead.
    *
    * @see \Drupal\image\ImageStyleStorageInterface::getReplacementId()
diff --git a/core/modules/image/src/ImageStyleStorage.php b/core/modules/image/src/ImageStyleStorage.php
index 143bde8..606d3ef 100644
--- a/core/modules/image/src/ImageStyleStorage.php
+++ b/core/modules/image/src/ImageStyleStorage.php
@@ -9,6 +9,9 @@
 
 use Drupal\Core\Config\Entity\ConfigEntityStorage;
 
+/**
+ * Storage controller class for "image style" configuration entities.
+ */
 class ImageStyleStorage extends ConfigEntityStorage implements ImageStyleStorageInterface {
 
   /**
diff --git a/core/modules/image/src/ImageStyleStorageInterface.php b/core/modules/image/src/ImageStyleStorageInterface.php
index acfdcbf..014ff7c 100644
--- a/core/modules/image/src/ImageStyleStorageInterface.php
+++ b/core/modules/image/src/ImageStyleStorageInterface.php
@@ -7,6 +7,9 @@
 
 namespace Drupal\image;
 
+/**
+ * Interface for storage controller for "image style" configuration entities.
+ */
 interface ImageStyleStorageInterface {
 
   /**
@@ -35,7 +38,7 @@ public function setReplacementId($name, $replacement);
    * @return string|null
    *   The ID of the image style used as replacement, if there's any, or NULL.
    *
-   * @see \Drupal\image\ImageStyleStorageInterface::setReplacementId().
+   * @see \Drupal\image\ImageStyleStorageInterface::setReplacementId()
    */
   public function getReplacementId($name);
 
@@ -47,7 +50,7 @@ public function getReplacementId($name);
    * @param string $name
    *   The ID of the image style to be replaced.
    *
-   * @see \Drupal\image\ImageStyleStorageInterface::setReplacementId().
+   * @see \Drupal\image\ImageStyleStorageInterface::setReplacementId()
    */
   public function clearReplacementId($name);
 
diff --git a/core/modules/image/src/Tests/ImageStyleDeleteTest.php b/core/modules/image/src/Tests/ImageStyleDeleteTest.php
index 4ab33a9..7bcedb3 100644
--- a/core/modules/image/src/Tests/ImageStyleDeleteTest.php
+++ b/core/modules/image/src/Tests/ImageStyleDeleteTest.php
@@ -9,7 +9,6 @@
 
 use Drupal\Core\Entity\Entity\EntityFormDisplay;
 use Drupal\Core\Entity\Entity\EntityViewDisplay;
-use Drupal\image\Entity\ImageStyle;
 
 /**
  * Tests image style deletion using the UI.
@@ -56,7 +55,6 @@ public function testDelete() {
       $this->assertIdentical($component['settings']['preview_image_style'], 'thumbnail');
     }
 
-
     $this->drupalGet('admin/config/media/image-styles/manage/thumbnail/delete');
     // Checks that the 'replacement' select element is displayed.
     $this->assertFieldByName('replacement');

Some code style cleanup on commit and fixing #117 deprecation message.

The issue has been automatically closed.
However, does the 8.1.6 contains this fix ?
Currently I'm still having this issue with the 8.1.3

---- edited ----

My fault, it was about image style, but not matching this issue.
Sorry

Strange, this was committed well before 8.1.0 - have you run update.php?