diff --git a/src/Controller/ImageStyleRedirectController.php b/src/Controller/ImageStyleRedirectController.php index c857631..e03ddf8 100644 --- a/src/Controller/ImageStyleRedirectController.php +++ b/src/Controller/ImageStyleRedirectController.php @@ -45,7 +45,7 @@ class ImageStyleRedirectController extends ImageStyleDownloadController { protected $imageStyleCopier; /** - * Constructs a ImageStyleDownloadController object. + * Constructs an ImageStyleRedirectController. * * @param \Drupal\Core\Lock\LockBackendInterface $lock * The lock backend. @@ -126,19 +126,6 @@ class ImageStyleRedirectController extends ImageStyleDownloadController { $image_uri = "$scheme://$source_path"; $destination_temp = $image_style->buildUri("temporary://flysystem/$scheme/$source_path"); - // Save the temporary image so cron will eventually clean it up. - $source_file = $this->fileStorage->loadByProperties(['uri' => $destination_temp]); - $temporary_image = reset($source_file); - // The temporary file entity could exist, but the file on disk could have - // been removed by a server reboot or a system administrator. - if (!$temporary_image) { - /** @var File $temporary_image */ - $temporary_image = File::create([ - 'uid' => User::getAnonymousUser()->id(), - 'uri' => $destination_temp, - ]); - } - // Try to generate the temporary image, watching for other threads that may // also be trying to generate the temporary image. try { @@ -146,19 +133,20 @@ class ImageStyleRedirectController extends ImageStyleDownloadController { if (!$success) { throw new \RuntimeException('The temporary image could not be generated'); } - $temporary_image->save(); } catch (ServiceUnavailableHttpException $e) { // This exception is only thrown if the lock could not be acquired. $tries = 0; - $source_file = $this->fileStorage->loadByProperties(['uri' => $destination_temp]); - while ($tries < 4 && (!file_exists($destination_temp) || !$temporary_image = reset($source_file))) { - // The file still doesn't exist or it exists but the other thread hasn't - // saved the entity yet. + + do { + if (file_exists($destination_temp)) { + break; + } + + // The file still doesn't exist. usleep(250000); - $source_file = $this->fileStorage->loadByProperties(['uri' => $destination_temp]); $tries++; - } + } while ($tries < 4); // We waited for more than 1 second for the temporary image to appear. // Since local image generation should be fast, fail out here to try to @@ -168,11 +156,11 @@ class ImageStyleRedirectController extends ImageStyleDownloadController { } } - return $temporary_image; + return $destination_temp; } /** - * Flush the output buffer and copy the temporary image to the adapter. + * Flushes the output buffer and copies the temporary images to the adapter. */ protected function flushCopy() { // We have to call both of these to actually flush the image. @@ -182,7 +170,7 @@ class ImageStyleRedirectController extends ImageStyleDownloadController { } /** - * Redirect to to an adapter hosted image, if it exists. + * Redirects to to an adapter hosted image, if it exists. * * @param string $source_uri * The URI to the source image. @@ -197,15 +185,24 @@ class ImageStyleRedirectController extends ImageStyleDownloadController { */ protected function redirectAdapterImage($source_uri, ImageStyleInterface $image_style) { $derivative_uri = $image_style->buildUri($source_uri); + if (file_exists($derivative_uri)) { // We can't just return TrustedRedirectResponse because core throws an // exception about missing cache metadata. // https://www.drupal.org/node/2638686 // https://www.drupal.org/node/2630808 // http://drupal.stackexchange.com/questions/187086/trustedresponseredirect-failing-how-to-prevent-cache-metadata - $url = Url::fromUri($image_style->buildUrl($source_uri))->toString(TRUE); - $response = new TrustedRedirectResponse($url->getGeneratedUrl()); - $response->addCacheableDependency($url); + + // @todo Figure out why caching this response leads to stale images being + // served. + + // $url = Url::fromUri($image_style->buildUrl($source_uri))->toString(TRUE); + // $response = new TrustedRedirectResponse($url->getGeneratedUrl()); + // $response->addCacheableDependency($url); + + $response = new TrustedRedirectResponse($image_style->buildUrl($source_uri)); + $response->addCacheableDependency(0); + return $response; } @@ -213,7 +210,7 @@ class ImageStyleRedirectController extends ImageStyleDownloadController { } /** - * Deliver a generate an image, deliver it, and upload it to the adapter. + * Delivers a generate an image, deliver it, and upload it to the adapter. * * @param string $scheme * The scheme of the source image. @@ -229,7 +226,7 @@ class ImageStyleRedirectController extends ImageStyleDownloadController { $source_uri = $scheme . '://' . $source_path; $derivative_uri = $image_style->buildUri($source_uri); try { - $temporary_image = $this->generateTemporaryImage($scheme, $source_path, $image_style); + $temporary_uri = $this->generateTemporaryImage($scheme, $source_path, $image_style); } catch (\RuntimeException $e) { $this->logger->notice('Unable to generate the derived image located at %path.', ['%path' => $derivative_uri]); @@ -237,7 +234,7 @@ class ImageStyleRedirectController extends ImageStyleDownloadController { } // Register a copy task with the kernel terminate handler. - $this->imageStyleCopier->addCopyTask($temporary_image->getFileUri(), $source_uri, $image_style); + $this->imageStyleCopier->addCopyTask($temporary_uri, $source_uri, $image_style); // Symfony's kernel terminate handler is documented to only executes after // flushing with fastcgi, and not with mod_php or regular CGI. However, @@ -252,7 +249,7 @@ class ImageStyleRedirectController extends ImageStyleDownloadController { }); } - return $this->send($scheme, $temporary_image->getFileUri()); + return $this->send($scheme, $temporary_uri); } } diff --git a/src/ImageStyleCopier.php b/src/ImageStyleCopier.php index fbf7290..5b913df 100644 --- a/src/ImageStyleCopier.php +++ b/src/ImageStyleCopier.php @@ -10,7 +10,6 @@ use Drupal\image\ImageStyleInterface; use Psr\Log\LoggerInterface; use Symfony\Component\DependencyInjection\ContainerInterface; use Symfony\Component\EventDispatcher\EventSubscriberInterface; -use Symfony\Component\HttpFoundation\File\Exception\UploadException; use Symfony\Component\HttpKernel\KernelEvents; /** @@ -18,8 +17,6 @@ use Symfony\Component\HttpKernel\KernelEvents; * * This class is registered to run on the kernel's terminate event so it doesn't * block image delivery. - * - * @class ImageStyleCopier */ class ImageStyleCopier implements EventSubscriberInterface, ContainerInjectionInterface { @@ -52,7 +49,7 @@ class ImageStyleCopier implements EventSubscriberInterface, ContainerInjectionIn protected $copyTasks = []; /** - * Construct ImageStyleCopier. + * Constructs an ImageStyleCopier. * * @param \Drupal\Core\Lock\LockBackendInterface $lock * The lock backend. @@ -84,6 +81,7 @@ class ImageStyleCopier implements EventSubscriberInterface, ContainerInjectionIn public static function getSubscribedEvents() { $events = []; $events[KernelEvents::TERMINATE] = 'processCopyTasks'; + return $events; } @@ -93,7 +91,7 @@ class ImageStyleCopier implements EventSubscriberInterface, ContainerInjectionIn * @param string $temporary_uri * The URI of the temporary image to copy from. * @param string $source_uri - * The final destination of the image derivative. + * The URI of the source image. * @param \Drupal\image\ImageStyleInterface $image_style * The image style being copied. */ @@ -114,7 +112,7 @@ class ImageStyleCopier implements EventSubscriberInterface, ContainerInjectionIn } /** - * Generate an image with the remote stream wrapper. + * Generates an image with the remote stream wrapper. * * @param string $temporary_uri * The temporary file URI to copy to the adapter. @@ -133,7 +131,8 @@ class ImageStyleCopier implements EventSubscriberInterface, ContainerInjectionIn $lock_name = 'flysystem_copy_to_adapter:' . $image_style->id() . ':' . Crypt::hashBase64($source_uri); if (!$this->lock->acquire($lock_name)) { - throw new UploadException('Another copy of %image to %destination is in progress', $temporary_uri, $derivative_uri); + $this->logger->info('Another copy of %image to %destination is in progress', ['%image' => $temporary_uri, '%destination' => $derivative_uri]); + return; } try { diff --git a/src/PathProcessor/FlysystemImageStyleRedirectProcessor.php b/src/PathProcessor/FlysystemImageStyleRedirectProcessor.php index 3b777e8..735256d 100644 --- a/src/PathProcessor/FlysystemImageStyleRedirectProcessor.php +++ b/src/PathProcessor/FlysystemImageStyleRedirectProcessor.php @@ -16,14 +16,14 @@ class FlysystemImageStyleRedirectProcessor implements InboundPathProcessorInterf /** * The base menu path for style redirects. */ - const STYLES_PATH = '/_flysystem-style-redirect/styles'; + const STYLES_PATH = '/_flysystem-style-redirect'; /** * {@inheritdoc} */ public function processInbound($path, Request $request) { // Quick exit. - if (strpos($path, $this::STYLES_PATH) !== 0) { + if (strpos($path, static::STYLES_PATH) !== 0) { return $path; } diff --git a/src/Plugin/ImageStyleGenerationTrait.php b/src/Plugin/ImageStyleGenerationTrait.php index 3c7ed4a..66f88b1 100644 --- a/src/Plugin/ImageStyleGenerationTrait.php +++ b/src/Plugin/ImageStyleGenerationTrait.php @@ -81,7 +81,13 @@ trait ImageStyleGenerationTrait { */ protected function generateImageUrl($target) { list(, $style, $scheme, $file) = explode('/', $target, 4); - return \Drupal::urlGenerator()->generate("flysystem.$scheme.style_redirect", ['image_style' => $style], UrlGeneratorInterface::ABSOLUTE_URL) . "/$file"; + $args = [ + 'image_style' => $style, + 'scheme' => $scheme, + 'filepath' => $file, + ]; + + return \Drupal::urlGenerator()->generate('flysystem.image_stye_redirect.serve', $args, UrlGeneratorInterface::ABSOLUTE_URL); } } diff --git a/src/Routing/FlysystemRoutes.php b/src/Routing/FlysystemRoutes.php index 936654c..686e0e4 100644 --- a/src/Routing/FlysystemRoutes.php +++ b/src/Routing/FlysystemRoutes.php @@ -78,21 +78,6 @@ class FlysystemRoutes implements ContainerInjectionInterface { foreach ($this->factory->getSchemes() as $scheme) { $settings = $all_settings[$scheme]; - // Public image route that serves initially from Drupal, and then - // redirects to a remote URL when it's ready. - if ($this->moduleHandler->moduleExists('image')) { - $routes['flysystem.' . $scheme . '.style_redirect'] = new Route( - "/_flysystem-style-redirect/styles/{image_style}/$scheme", - [ - '_controller' => 'Drupal\flysystem\Controller\ImageStyleRedirectController::deliver', - 'scheme' => $scheme, - ], - [ - '_access' => 'TRUE', - ] - ); - } - if ($settings['driver'] !== 'local' || empty($settings['config']['public'])) { continue; } @@ -141,6 +126,31 @@ class FlysystemRoutes implements ContainerInjectionInterface { 'scheme' => '^[a-zA-Z0-9+.-]+$', ] ); + + // Public image route that serves initially from Drupal, and then + // redirects to a remote URL when it's ready. + $routes['flysystem.image_stye_redirect'] = new Route( + "/_flysystem-style-redirect/{image_style}/{scheme}", + [ + '_controller' => 'Drupal\flysystem\Controller\ImageStyleRedirectController::deliver', + ], + [ + '_access' => 'TRUE', + 'scheme' => '^[a-zA-Z0-9+.-]+$', + ] + ); + + $routes['flysystem.image_stye_redirect.serve'] = new Route( + "/_flysystem-style-redirect/{image_style}/{scheme}/{filepath}", + [ + '_controller' => 'Drupal\flysystem\Controller\ImageStyleRedirectController::deliver', + ], + [ + '_access' => 'TRUE', + 'scheme' => '^[a-zA-Z0-9+.-]+$', + 'filepath' => '.+', + ] + ); } return $routes;