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

Status:Active» Needs review
StatusFileSize
new839 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,333 pass(es).
[ View ]

Issue summary:View changes

more info

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.

Status:Needs work» Needs review
StatusFileSize
new836 bytes
new825 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,819 pass(es).
[ View ]

Sure. Thanks!

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.

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.

Status:Needs review» Reviewed & tested by the community

Great catch!

REMOVED! (erroneously duplicated last comment).

Status:Reviewed & tested by the community» Fixed

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

Status:Fixed» Patch (to be ported)

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

Patch for Drupal 7.

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.

Issue summary:View changes

c

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.