When using the 'Flush external images' function of this module, it calls file_unmanaged_delete_recursive() on the 'externals' directory, which attempts to delete everything in one request. On a large site with a highly populated directory (approx 20000 files), this times out for us on Pantheon with set limits. It looks like the D7 version of the module has a threshold to clean this directory up during cron runs, but we have a need to clear it manually after some other custom functionality happens in the site.

I am proposing that it use Drupal's Batch API to split the file deletion process into chunks so that the process does not time out. I will post a D8 patch to this issue, as well as a D7 version. Thanks!

CommentFileSizeAuthor
#9 d7-use_batch_api-imagecache_external-3017032-9.patch5.13 KBvinmassaro
#9 Screenshot 2019-01-18 11.53.45.jpg16.84 KBvinmassaro
#3 d8-use_batch_api-imagecache_external-3017032-3.patch5.95 KBvinmassaro

Comments

vinmassaro created an issue. See original summary.

vinmassaro’s picture

vinmassaro commented
Issue summary: View changes
vinmassaro’s picture

vinmassaro commented
Status: Active » Needs review
StatusFileSize
new d8-use_batch_api-imagecache_external-3017032-3.patch5.95 KB

Here is a patch against 8.x-1.x.

BarisW’s picture

BarisW commented
Status: Needs review » Reviewed & tested by the community

Thanks for this great patch, really happy with it. I tested it and it works great. I found two small issues, which I will fix while committing the patch. 

  1. +++ b/imagecache_external.module
@@ -267,26 +267,122 @@ function imagecache_external_file_download($uri) {
+          $chunk, 'details' => t('(Deleting file batch @chunk  of @count)',

    Double space 

  2. +++ b/imagecache_external.module
@@ -267,26 +267,122 @@ function imagecache_external_file_download($uri) {
+    drupal_set_message($message, 'error');

    drupal_set_message is deprecated, should now be \Drupal::messenger()->addMessage

  • BarisW committed 1baf5dd on 8.x-1.x authored by vinmassaro 
    Issue #3017032 by vinmassaro, BarisW: Use Batch API when flushing...
BarisW’s picture

BarisW commented
Status: Reviewed & tested by the community » Fixed

Thanks again!

vinmassaro’s picture

vinmassaro commented

Thanks! I think that double space may be required. I thought it was odd also. I originally needed this for D7 so I will put that backported patch here in a few days.

vinmassaro’s picture

vinmassaro commented
Issue tags: +Needs backport to D7
vinmassaro’s picture

vinmassaro commented
Status: Fixed » Needs review
StatusFileSize
new Screenshot 2019-01-18 11.53.45.jpg16.84 KB
new d7-use_batch_api-imagecache_external-3017032-9.patch5.13 KB

Here is the D7 patch as promised. This includes the double space you mentioned removing in #4 - here is a screenshot of what the output looks like with only a single space, but I'm not sure why this happens, so maybe it should be added back to the D8 version:

The patch also adds the file count that I have posted a D8 patch for in #3026953: Add file count to 'flush external images' page. Thanks!

BarisW’s picture

BarisW commented
Version: 8.x-1.x-dev » 7.x-2.x-dev
Issue tags: -Needs backport to D7
vinmassaro’s picture

vinmassaro commented

@BarisW: were you able to test my D7 patch? It is similar to the D8 version. Would love to get this committed. Thanks.