Delete image style only if it exists in database

Slightly expanding the issue here - but also I think features needs to pass a second parameter to image_style_delete. If none is passed then all field image formatters that used the image_style get their image_style instance setting wiped out thanks to image_image_style_delete().

At the moment any features-revert-all which ends up reverting an image_style *after* reverting an image field which used it causes that image field to end up broken in the db, and the old feature Overridden once again.

I've added the second parameter to the patch.

Patch applies and seems to solve the problem.

+++ b/includes/features.image.incundefined
@@ -74,7 +74,16 @@ function image_features_revert($module) {
+        // If image style has an isid, then it exists in database : delete it to revert.

' : ' is not valid English.

+++ b/includes/features.image.incundefined
@@ -74,7 +74,16 @@ function image_features_revert($module) {
+          // Pass 2nd parameter here to prevent image.module
+          // from wiping out all field settings associated with this image style.
+          // Ideally features would have some notion of dependency when reverting
+          // features - reverting an image needs to be followed by reverting
+          // all fields that used that image style.
+          // This fix works for now.

These comments sound like documentation of a hack, not a fix. If there needs to be a @todo, let's add one.

Closing this for lack of activity. I believe this was already fixed, but please re-open this issue if you can reproduce it in the latest version.

I know this is an old thread, but for my setup this issue seems to have reared its head again. I'm using 7.x-2.11 on Drupal 7.65. with the following snippet to revert any overridden features after running my install profile.

  if(module_exists('features')){
    module_load_include('inc', 'features', 'features.admin');
    $features = _features_get_features_list();
    foreach($features as $mod_name => $mod){
      if(module_exists($mod_name)){
        features_revert_module($mod_name);
      }
    }
  }

which is causing the following notice:

Notice: Undefined index: isid in image_default_style_revert() (line 1141 of Drupal/Sites/dev/modules/image/image.module).

Think this issue may need reopening.

I, too, am seeing this issue on all of our sites during initial install. We are running a custom install profile that uses Features.

Looks like the culprit is here in core:

function image_default_style_revert($style) {
  image_style_flush($style);

  // NOTE: No check if $style['isid'] is set
  db_delete('image_effects')->condition('isid', $style['isid'])->execute();
  db_delete('image_styles')->condition('isid', $style['isid'])->execute();

  return TRUE;
}

But since this issue is in the queue for features, perhaps the issue is that Features should not be calling this function if the style doesn't actually exist in the DB yet.

This issue still affects the current 7.x-2.x version. It happened to one of our websites when trying to install a website from scratch based on an install profile which contains features. When this install profile runs it causes a lot of these messages:

Exception Notice     image.module      1141 image_default_style_revert()
    Undefined index: isid

I made a patch which applies to the current 7.x-2.x version. It adds a check to the image_default_style_revert() method call which ensures the isid key exists. If this key doesn't exist it means the style doesn't exist in the database (only in the code) and therefore the revert should not be called as it won't be able to do anything.

See attached patch.

@mpotter, can you please re-open this issue? I can't as "Only maintainers can reopen".

Normally you should just create a new issue rather than re-opening an old issue like this. But will make an exception in this case.

We've added this patch to our distribution; it fixes our problem with PHP notices when enabling a feature that includes an image style.

The patch might cause a regression.

Problem: Do we need to flush generated images?

image_default_style_revert() does two things:

• Delete overrides in the database.
• Flush cached images for the image style, by calling image_style_flush($style).

If the style is not stored in the database, we don't need to delete it from there.
However we might still want to flush the existing generated images.

The typical situation would be like this:

• Initial situation: Image style exists in the database, or not at all.
• Version 1 of feature gets deployed, you run drush fr.
  -> Image style is deleted in the database, only the code version is left.
  -> If the old (database) version and the new (code) version are different, we need to clear generated images.
• Version 2 of feature gets deployed, you run drush fr.
  -> Image style was already not present in the database.
  -> If the old (code) and new (code) version are different, we need to clear the generated images.

Clearing generated images is a costly operation on production, it means that images need to be regenerated afterwards.
It would be great if we could skip this, if the definition has not actually changed.

Proposed solution for this issue

Perhaps in this issue we should aim for a "minimal change" solution that fixes the problem but does not cause any regression.

This would be to call image_style_flush() directly, instead of image_default_style_revert(), if the style has no style id.

            if (isset($style['isid'])) {
              image_default_style_revert($style);
            }
            else {
              image_style_flush($style);
            }

Perhaps the flushing might not be necessary in all cases, but this would be a separate problem that already exists in the current version.

More problems.

IMAGE_STORAGE_DEFAULT vs isset($style['isid'])

The function image_features_revert() already contains a check for $style['storage'] != IMAGE_STORAGE_DEFAULT.

Normally we should assume that if the storage is IMAGE_STORAGE_DEFAULT, then the style, at the time it was loaded (and written to the cache), was NOT stored in the database, but was coming from a module. In that case it does NOT have an 'isid'.
On the other hand, if 'storage' is something other than IMAGE_STORAGE_DEFAULT, then it DOES have an 'isid'.

This means, the check for 'isid' we are adding here is equivalent to the check for IMAGE_STORAGE_DEFAULT which is already there!

Under this assumption, the else branch with its call to image_default_style_revert() should be pointless.
This else branch was added in #2148453: Image styles do not revert properly, commit 6e7c23b.

But so also need to take into account the cache in image_styles().

Can it happen that:

• Cached image style has an 'isid', while...
  • ...the actual database record has since been deleted?
    -> Normally not. When an image style gets deleted from the database via image_default_style_revert(), the cache will be cleared.
  • ...the config in the database has changed ince?
    -> Normally not. When image style is updated in the database, the cache gets cleared.
• Cached image style does not have an 'isid', while...
  • ...there is an actual record in the database?
    -> Normally not. When an image style is written to the database, the cache gets cleared.
  • ...the code has changed since?
    -> Yes! But only if you forgot to run drush cc all after the deployment.
    -> In this case, the else branch of image_features_revert() will kick in on features revert.
  • ...the cached version is up to date with the code, but generated images are outdated?
    -> Yes! If you run drush cc all before drush fr, then none of the two operations will actually clear the generated images.

This is some food for thought for whoever wants to work on this.

All of the above are conclusions I made from looking at the code. I did not actually test any of it.
Whoever wants to work on this should also check if my conclusions are correct!

I added a patch in #3201779: Comparison of style effect in image_features_revert() might never be equal to fix the effect comparison for in code changes in the else condition which appeared to be causing in code image styles to be flushed on every feature revert. However, I agree with @donquixote's last comment, the else is only relevant for in code styles if the image styles cache is not cleared before running the feature revert; I ran a few feature reverts on in code changes with and without drush cc all.