Support delivering local image styles until remote upload is complete

I've got this most of the way locally - TODOs are registering and cleaning up temp files, and copying derivatives from temp to S3 instead of generating the image again.

I filed #2685905: Refactor ImageStyleDownloadController so derivatives can be generated by contrib modules which significantly simplifies our code, as we don't have to re-implement image generation in it's entirety. This patch copies that class with a small addition to add a method added in 8.1.x.

Here's an update that:

* Saves temporary files to {file_managed} so cron can clean them up.
* Adds the routes and path processor for temp-then-redirect images.
* Refactors image generation to the controller.
* Adds a bunch of docblock comments.

Still a few TODOs on this, so not quite ready yet.

This patch:

• Copies the generated image from temporary:// instead of recreating it for the adapter
• Splits the shutdown function code to a method
• Handles temporary files being removed from under Drupal, such as in a server reboot
• Prevents stampedes with multiple requests that need a temporary file
• Prevents duplicate uploads of the same image to the adapter in high traffic scenarios
• Moves the path processor to a new class

I think this is ready for an initial review, though it needs tests. I'm not particularly happy with how large the ImageStyleRedirectController class is, but we're kind of stuck with it if we want to use the upstream controller class for lock and image handling. Any ideas?

Oh, and I forgot to mention, this removes the generateImageStyle() method in favour of a generateImageUrl() method. Generating images in serial as URLs are generated is a significant performance issue, and any adapters implementing it should probably be refactored to delay image generation until an inbound request is received.

1. +++ b/src/Controller/ImageStyleRedirectController.php
   @@ -21,72 +29,102 @@ use Symfony\Component\HttpFoundation\Response;
   +      throw new UploadException(sprintf('Unable to copy %image to %destination', $temporary_uri, $derivative_uri));
   

   May be not a good idea - but is that possible to have a queue here?

2. +++ b/src/Controller/ImageStyleRedirectController.php
   @@ -105,23 +143,132 @@ class ImageStyleRedirectController extends ImageStyleDownloadController {
   +    ob_end_flush();
   

   Does the sequence of calling the two method matter?

3. +++ b/src/Controller/ImageStyleRedirectController.php
   @@ -105,23 +143,132 @@ class ImageStyleRedirectController extends ImageStyleDownloadController {
   +      $t = $this;
   

   Why have to do this?

4. +++ b/src/PathProcessor/FlysystemImageStyleRedirectProcessor.php
   @@ -0,0 +1,47 @@
   +    if (strpos($path, '/_flysystem-style-redirect/styles') !== 0) {
   

   I saw this string duplicated several times in this method. Assign it to a var may make the code look cleaner.

Each image upload is a separate HTTP thread, so I don't think a queue will save us anything. We could queue the actual S3 uploads off to a drush background process, but in high traffic scenarios I think it's best to make sure that S3 is populated as soon as possible. I recently discovered imageinfo_cache which is 7.x only but lets you select specific styles to pre-generate. That might be worth investigation (but not as a part of this patch).

The flush call sequence does matter - you have to clear the output buffer before calling flush().

This patch removes the $this assignment for the anonymous function. You need to do that in javascript as the scope of $this changes. PHP 5.4+ allows access to $this so the $t variable isn't needed. However, phpcs is throwing an inspection error on it, but I think that's just a bug in the inspection.

Good call on the string constant - this update adds one.

Also, this fixes not creating directories for image derivatives.

This fixes an extra slash in a regex and a missing slash in the path processor, breaking image style routes.

I was all excited about using the terminate event, but I've found two issues:

• The HTTP kernel only actually flushes when you use fastcgi, and not when you use mod_php or any other method for PHP.
• It looks like there's an open bug where binary or streamed files are sent after the terminate event.

Any ideas? The existing method is working for me with mod_php and fast cgi on Apache (under an Acquia environment), but we're having trouble with an nginx setup (that I'm testing a fix for now).

After testing this out with mod_php, it looks like responses actually do get flushed with mod_php, even though the docs say otherwise. I filed an issue up at
https://github.com/symfony/symfony-docs/issues/6520

This patch:

1. Brings back generateImageStyle() and marks it for deprecation.
2. Makes the route conditional on the image module.
3. Adds a dedicated class and service for copying images, instead of keeping that code in the controller.
4. Fixes a strict warning with reset() calls on a function return on PHP 5.5.

Is this needed [symfony/filesystem]?

Ah! There's a FileNotFoundException both in HttpFoundation and in Filesystem. That explains a good bit of confusion I had. I think since it's all thrown in the controller I'll move it all to the HttpFoundation exception.

throw new \Exception('The temporary image could not be generated');

Agreed, fixing.

Also cleaned up the try / catch block to a single line in the try.

+ protected static $copyTasks = [];

Good call - fixed in this patch.

This gets skipped for local images, correct? Because of path processing order.

Yes, that was intentional to not expose this code to local images. I didn't see any value for the local adapter where a redirect would lead back to yourself. Is there something else here?

+++ b/src/Controller/ImageStyleDownloadController.php
@@ -0,0 +1,297 @@
+ * Contains \Drupal\image\Controller\ImageStyleDownloadController.

flysystem, not image. However, @file docblocks are not needed any more in classes, interfaces and traits.

re-roll

addresses #16

Removed unrelated @file removals from the patch.

Do we have a testing strategy planned?

I've been struggling with this one too. Mocking all of the HTTP calls is going to be pretty complex. While this is really for remote adapters, realistically it should work for the local adapter too. Perhaps that's how we can do functional tests for this?

For now, I'm just disabling caching on the redirect.

Oy! That is nasty. I'm OK with disabling the cache for now, and fixing that as a followup if the performance side matters.

I've removed the temp file saving. I'm not sure what that's for. If we're worried about stale files, wouldn't it be easier to just remove them on cron? Also, what about deleting the temp files after the copy?

I don't think we want to delete files after the copy, because other threads could be in progress caught between the file_exists() call and actually returning the file. We could manually add a cron worker to clean up old files, but isn't that the point of registering a file as temporary anyways?

(Closed #2805037: Stale image styles served from temporary:// under some circumstances and moved the info here)

We're using the patch from #25 and having an issue that seems to be related with it. Basically, it seems like stale image styles can be served from temporary:// under some circumstances.

To reproduce:

0. Use S3 (not sure if this is necessary to reproduce or not)
1. Update an image style
2. View an image of that image style
3. Change the image style again
4. View the image again - you will see the stale image from #2 if you did it quickly

Looks like the issue is that editing an image style DOES correctly flush those images from S3, but it DOES NOT clear them out from temporary://, which is where Flysystem put them before they got sent to S3 in the first place. So what happens is that in ImageStyleRedirectController.php, it sees that the image doesn't exist on S3, so it goes to deliveryTemporary() which itself goes to generateTemporaryImage(), which finally calls generate().

In that function, you see this line:

$success = file_exists($derivative_uri) || $image_style->createDerivative($image_uri, $derivative_uri);

Note that $derivative_url here is the URL for the file in temporary://, which does still exist if you did it too quickly for the OS to clear out your /tmp directory. Thus, you get a stale image. I've worked around this locally like so:

function MODULENAME_file_update(Drupal\Core\Entity\EntityInterface $entity) {
  if ($entity->bundle() !== 'image') {
    return;
  }

  $filename = $entity->getFilename();
  $styles = ImageStyle::loadMultiple();
  foreach ($styles as $style) {
    $temp_path = 'temporary://styles/' . $style->id(). '/temporary/flysystem/s3/' . $filename;
    file_unmanaged_delete($temp_path);
  }
}

Random crazy idea - how about creating items in the queue and then add an AJAX callback to trigger the queue on the next page request? That way the items go into a queue so e.g. an S3 outage / slowdown wouldn't break the save process, but the callback would trigger the file(s) to be pushed up so there wouldn't be this delay until the next cron cycle.

I've been having an issue with this patch with an S3 private bucket. The image derivatives display once correctly with the redirect path, and then once the derivative is saved to S3, when it attempts to generate a URL, I keep getting the 'itok' query string appended to the URL, and Amazon rejects the signed URL because of the additional query string data in the URL. If I remove the itok query, then the generated URL works. I wonder if we should just always route through the redirect path instead. I wonder if other providers would have the need to strip the itok value from the URL.

Thought I'd chime in on this, as I've been going round and round on the issue of successfully applying image styles and storing derivative images in object storage successfully (we are using GCS for object storage, and using Flysystem and Flysystem_s3 to interact with GCS).

To get a solution that successfully writes images to the object store via the streamwrappers provided by the above two modules AND also write the derivative images (thumbnails and other images generated with image styles), we finally had to resort to patching a dependent library.

There is an outstanding PR on github for twistor/stream-util (https://github.com/twistor/stream-util) which is a dependency for this module. By patching this library for the outstanding PR on the project (https://github.com/twistor/stream-util/pull/1), we were able to resolve the issues we were having generating and writing derivative images to the remote object storage, and subsequently retrieving and displaying those images with Drupal.

Because the patch is explicit for version v1.0.1 of twistor/stream-util, I also explicitely declared that version as a dependency in compser.json, pinning the project to this partcular version. Hopefully the outstanding PR will get merged soon, in the meantime this did resolve the issue I was having on two different projects with derivative images.

Patch is attached, please note this is NOT for this module, but for the twistor/stream-util library.

Adding an updated patch to replace the one from #37

Added patch for 2.x beta version.

I created a MR based on the last patch. I had to fix a bunch of stuff to get it working for me:

• Update deprecated/removed core services and functions
• Fix some small issues that came up
• Fix the temporary image generation failing if an image style changes the extension of the generated derivative

I also opened an MR in the flysystem_gcs project, integrating the new code here in an actual adapter: #3285371: Saving and loading content takes way too long when multiple image styles are used.

Now both MR's are in a working state, this one still needs tests.

@dieterholvoet Would you be kind enough to change your MR to apply against the 2.1.x-dev branch? I'd like to see if the automated tests will pass, it appears that your MR does apply to 2.1.x as well as 8.x-1.x.

I rebased the branch against 2.1.x, there weren't any conflicts. Changing back to Needs work because there aren't any tests yet - unless you decide that's not necessary.

https://www.drupal.org/pift-ci-job/2730529

Existing test coverage appears to be adequate, however this patch does fail one existing tests. In looking at the test it appears that perhaps the test needs to be updated.

Can you take a look at the failing test, determine if the failing assertion on `src/Unit/Routing/FlysystemRoutesTest.php:161` needs to be revised, and submit a revision as part of this MR?

If the assertion is still valid, note that here as well, and we'll need to work on this a bit more.