Follow-up from #1057656: Image styles don't flush for images stored non-default schemes.

Updated: Comment #0

Problem/Motivation

When editing image styles, the watchdog gets crowded with messages like

The file private://styles/bingo_bongo was not deleted because it does not exist.

This is due to the fact that upon image style flushing, file_unmanaged_delete_recursive() logs a message if the file to be deleted is non-existent, which is quite the case (for the non-default wrappers) when you are making changes to the style, editing its effects. While this is an expected behavior during a recursive delete, it seems to me unnecessary when it is the root file to be deleted.

Proposed resolution

Introduce a file existence check before invoking file_unmanaged_delete_recursive().

Remaining tasks

Approve patch.

User interface changes

None.

API changes

None.

None.

Files: 
CommentFileSizeAuthor
#10 2079315-image_style_flush-10.patch688 bytesmondrake
PASSED: [[SimpleTest]]: [MySQL] 40,447 pass(es). View
#3 2079315-image_style_flush-3.patch825 bytesmondrake
PASSED: [[SimpleTest]]: [MySQL] 58,819 pass(es). View
#3 interdiff_1-3.txt836 bytesmondrake
#1 2079315-image_style_flush-1.patch839 bytesmondrake
PASSED: [[SimpleTest]]: [MySQL] 58,333 pass(es). View

Comments

mondrake’s picture

Status: Active » Needs review
FileSize
839 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,333 pass(es). View
mondrake’s picture

Issue summary: View changes

more info

claudiu.cristea’s picture

Status: Needs review » Needs work
+++ b/core/modules/image/lib/Drupal/image/Entity/ImageStyle.php
@@ -249,7 +249,9 @@ public function flush($path = NULL) {
+      if (file_exists($wrapper . '://styles/' . $this->id())) {

Use a variable as you process twice the same file uri.

Otherwise looks good.

mondrake’s picture

Status: Needs work » Needs review
FileSize
836 bytes
825 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,819 pass(es). View

Sure. Thanks!

claudiu.cristea’s picture

Just one more comment: what is the reason for using file_exists() and not is_dir()? I didn't have any clue what would be the best.

mondrake’s picture

Valid question. See #1057656: Image styles don't flush for images stored non-default schemes, comment #19.

Nice clean up. Another minor improvement might be to remove is_dir(), as file_unmanaged_delete_recursive() also does an is_dir(). The difference might be that if it is a file (absolutely not expected and not wanted here), it is removed allowing to create it as a directory the next time a image derivative is created.

We might end up having an unintended file with exactly the file name we are expecting to be a directory. So we just remove that in that case, or else the derivatives won't be generated.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

Great catch!

claudiu.cristea’s picture

REMOVED! (erroneously duplicated last comment).

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 00f461a and pushed to 8.x. Thanks!

mondrake’s picture

Status: Fixed » Patch (to be ported)
mondrake’s picture

Version: 8.x-dev » 7.x-dev
Status: Patch (to be ported) » Needs review
FileSize
688 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,447 pass(es). View

Patch for Drupal 7.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

Tests passed in D7 and I see no difference in the code comparing to D8. RTBC. Thank you.

claudiu.cristea’s picture

Issue summary: View changes

c

David_Rothstein’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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