Problem/Motivation

Image styles register itself for the file directory path so the same image url can be used to either serve the image style
directly via the webserver (once the image is generated) or via the drupal route system.

As the new route system in Drupal 8 does not support arbitrary length of paths anymore the internal drupal URL need a query parameter
to serve the image.

The same problem exists for normal file downloads.

Proposed resolution

The conversion happens in an inbound path alter event subscriber.

Remaining tasks

Fixing broken tests.

CommentFileSizeAuthor
#106 drupal-1987712-106.patch35.06 KBclaudiu.cristea
#106 interdiff.txt3.46 KBclaudiu.cristea
#100 drupal-1987712-100.patch35.14 KBclaudiu.cristea
#100 interdiff.txt3.89 KBclaudiu.cristea
#97 drupal-1987712-97.patch34.54 KBclaudiu.cristea
#97 interdiff.txt611 bytesclaudiu.cristea
#96 drupal-1987712-96.patch34.54 KBclaudiu.cristea
#96 interdiff.txt2.65 KBclaudiu.cristea
#92 drupal-1987712-92.patch34.23 KBclaudiu.cristea
#92 interdiff.txt693 bytesclaudiu.cristea
#90 drupal-1987712-90.patch34.15 KBclaudiu.cristea
#90 interdiff.txt2.43 KBclaudiu.cristea
#88 drupal-1987712-88.patch33.68 KBclaudiu.cristea
#88 interdiff.txt1.13 KBclaudiu.cristea
#86 drupal-1987712-86.patch33.81 KBclaudiu.cristea
#86 interdiff.txt686 bytesclaudiu.cristea
#81 drupal-1987712-81.patch33.82 KBclaudiu.cristea
#81 interdiff.txt6.83 KBclaudiu.cristea
#79 drupal-1987712-79-do-not-test.patch28.47 KBclaudiu.cristea
#77 drupal-1987712-77.patch31.28 KBclaudiu.cristea
#77 interdiff.txt1.95 KBclaudiu.cristea
#75 drupal-1987712-75.patch31.31 KBclaudiu.cristea
#75 interdiff.txt4.28 KBclaudiu.cristea
#70 drupal-1987712-70.patch31.75 KBclaudiu.cristea
#70 interdiff.txt9.22 KBclaudiu.cristea
#60 drupal-1987712-60.patch30.59 KBdawehner
#60 interdiff.txt810 bytesdawehner
#58 drupal-1987712-58.patch30.59 KBdawehner
#55 interdiff.txt3.1 KBandypost
#55 file-deliver-1987712-55.patch30.42 KBandypost
#52 interdiff.txt3.79 KBandypost
#52 file-deliver-1987712-52.patch30.34 KBandypost
#49 drupal-1987712-49.patch30.9 KBdawehner
#49 interdiff.txt1.86 KBdawehner
#41 file_download-1987712-41.patch30.92 KBdawehner
#41 interdiff.txt802 bytesdawehner
#39 drupal-1987712-39.patch30.88 KBdawehner
#33 file_download-1987712-33.patch28.93 KBdawehner
#33 interdiff.txt1.6 KBdawehner
#31 file_download-1987712-31.patch28.78 KBdawehner
#29 file_download-1987712-29.patch28.71 KBdawehner
#29 interdiff.txt4.77 KBdawehner
#27 file_download-1987712-27.patch28.34 KBdawehner
#27 interdiff.txt2.45 KBdawehner
#25 file_download_controllers-1987712-25.patch25.89 KBdawehner
#23 file_download_controllers-1987712-21.patch26.74 KBdawehner
#21 file_download_controllers-1987712-21.patch26.74 KBdawehner
#21 interdiff.txt648 bytesdawehner
#19 file_download_controllers-1987712-19.patch26.76 KBdawehner
#16 interdiff.txt4.33 KBdawehner
#16 file_download_controllers-1987712-16.patch21.72 KBdawehner
#14 file_download_controllers-1987712-14.patch21.16 KBdawehner
#11 drupal-1987712-11.patch9.39 KBdawehner
#10 1987712-9.patch7.92 KBdamiankloip
#9 1987712-9.patch2.52 KBdamiankloip
#9 interdiff-1987712-9.txt2.52 KBdamiankloip
#5 1987712-4.patch7.35 KBdamiankloip
#5 interdiff-1987712-4.txt2.77 KBdamiankloip
#3 1987712-3.patch7.32 KBdamiankloip
#3 interdiff-1987712-3.txt3.42 KBdamiankloip
#3 interdiff-1987712-3.1.txt654 bytesdamiankloip
#2 1987712.patch5.76 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

Assigned: Unassigned » damiankloip

Doing this one.

damiankloip’s picture

Status: Active » Needs review
FileSize
5.76 KB

I have made a start on this and converted the callback to a controller/method and added a route definition. However, how do we manage dynamic routes like this? Not dynamic in the sense of routesubscriber dynamic (still defined set of routes) but request dynamic. So we use the additional request arguments from the path.

damiankloip’s picture

ok, I spoke to amateescu, I guess we will have to use a query string to pass on the private file url. Which isn't ideal imo but seems a worthwhile tradeoff of the new routing system.

Status: Needs review » Needs work

The last submitted patch, 1987712-3.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.77 KB
7.35 KB

Hmm, I think this might fix the tests.

Status: Needs review » Needs work

The last submitted patch, 1987712-4.patch, failed testing.

Crell’s picture

amateescu asked me to comment here. Moving the file name to a query parameter seems fine, since this should be an opaque string. My only question is if that becomes a data leakage issue causing a security issue, but since even if you knew the direct URI to a private file you shouldn't be able to access it anyway (it's out of the webroot if you're doing it right), I don't think it's a problem.

I might suggest a name other than query for it, but that's a nitpick.

amateescu’s picture

The name is 'file', not 'query'..

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.52 KB
2.52 KB

Yeah, where did that come from?! :) This logic is going to remain unchanged, just using the file path in the query string instead. So the actual usage of private files will remain unchanged.

So E.g.

http://localhost/D8/system/files/styles/style_foo/private/image-test.png will become http://localhost/D8/system/files?file=styles/style_foo/private/image-tes...

I guess we should add the scheme as a slug in the pattern too, so it mimics the previous behaviour of getting the stream type from an arg.

Private files are working ok, the tests are failing because of private files and imagecache urls. Haven't worked out wtf is going on yet.

damiankloip’s picture

FileSize
7.92 KB

Sorry, wrong patch uploaded in #9

dawehner’s picture

FileSize
9.39 KB

As discussed AFK we realized that we have to change the way private image styles work.

As both private image styles and public ones go through the same route #1987722: Convert image_style_deliver() to a new style controller is somehow a requirement to make it working.
This patch just includes everything which is related to rewrite the url to make it possible later.

Status: Needs review » Needs work

The last submitted patch, drupal-1987712-11.patch, failed testing.

dawehner’s picture

Assigned: damiankloip » dawehner

Assign to myself.

dawehner’s picture

Assigned: dawehner » Unassigned
Status: Needs work » Needs review
FileSize
21.16 KB

I managed to get rid of any kind of rewrite and just do a inbound path processor.

Crell’s picture

+++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleController.php
@@ -0,0 +1,169 @@
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static($container->get('config.factory'), $container->get('module_handler'), $container->get('lock'));
+  }

We've usually been multi-lining the new static() call if it has multiple parameters. Not a commit blocker, but something to tweak if another reroll is needed.

+++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleController.php
@@ -0,0 +1,169 @@
+  public function deliver(Request $request, $scheme, $image_style) {

It MAY be scope creep, but most of this code belongs in a service or utility object, not in the controller, I think.

+++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleController.php
@@ -0,0 +1,169 @@
+        if (count($headers)) {
+          foreach ($headers as $name => $value) {
+            drupal_add_http_header($name, $value);
+          }

We're trying to remove drupal_add_http_header() anyway, so let's go ahead and eliminate it outright here.

Instead, the headers should be saved and set on the response object.

+++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleController.php
@@ -0,0 +1,169 @@
+      if (!$lock_acquired) {
+        // Tell client to retry again in 3 seconds. Currently no browsers are known
+        // to support Retry-After.
+        drupal_add_http_header('Status', '503 Service Unavailable');
+        drupal_add_http_header('Retry-After', 3);
+        print t('Image generation in progress. Try again shortly.');
+        drupal_exit();

Convert this to returning a response object. This is very easy to cleanup while we're here, so let's do so.

+++ b/core/modules/system/lib/Drupal/system/PathProcessor/PathProcessorPrivateFiles.php
@@ -0,0 +1,47 @@
+      $query = drupal_get_query_array($file_path);

This function call makes this class impure. Can we not just inline its logic somewhere? Does PHP have no native equivalent? Does the request object?

It's small enough I'm fine with just duplicating the code to a protected method on this object if nothing else.

dawehner’s picture

It MAY be scope creep, but most of this code belongs in a service or utility object, not in the controller, I think.

I totally agree, especially FileController::fileDownload though the logic is so full of response vs. actual logic that it feels hard to untackle. Maybe you have an idea.

This function call makes this class impure. Can we not just inline its logic somewhere? Does PHP have no native equivalent? Does the request object?

We have drupal_get_query_array and drupal_http_build_query. On the other side is parse_str and http_build_query. http_build_query does not work ("This differs from http_build_query() as we need to rawurlencode() (instead of urlencode()) all query parameters.") I would go with a HttpQuery component, see #2010024: Move url related functions to a new Url component

Crell’s picture

Without a complete rewrite of that function, I guess not. Urf. At least the fugly is contained here, so I guess that can be a follow-up.

Commented on the linked issue from #16. Otherwise this is looking good.

Status: Needs review » Needs work

The last submitted patch, file_download_controllers-1987712-16.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
26.76 KB

Some work done, though there are still some failing tests.

Status: Needs review » Needs work

The last submitted patch, file_download_controllers-1987712-19.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
648 bytes
26.74 KB

Fixing all beside one test failure.

Status: Needs review » Needs work

The last submitted patch, file_download_controllers-1987712-21.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
26.74 KB

This was actually easier to fix then expected.

      $image = image_load($derivative_uri);
      $uri = $image->source;
      $response->headers->set('Content-Type', $image->info['mime_type']);
      $response->headers->set('Content-Length', $image->info['file_size']);
      return new BinaryFileResponse($uri, 200, $response->headers->all());

Status: Needs review » Needs work

The last submitted patch, file_download_controllers-1987712-21.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
25.89 KB

Doh, that is the wrong patch.

Status: Needs review » Needs work

The last submitted patch, file_download_controllers-1987712-25.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.45 KB
28.34 KB

Fixed the last failure on ConfigController.

tim.plunkett’s picture

+++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.phpundefined
@@ -34,10 +37,17 @@ class ConfigController implements ControllerInterface {
+    return new static($container->get('config.storage'), $container->get('config.storage.staging'), $container->get('module_handler'));

+++ b/core/modules/system/lib/Drupal/system/FileController.phpundefined
@@ -0,0 +1,98 @@
+    return new static($container->get('module_handler'));

Please split this onto multiple lines for easier future additions

+++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleController.phpundefined
@@ -0,0 +1,178 @@
+   * @param $image_style
+   *   The image style to deliver.
...
+  public function deliver(Request $request, $scheme, $image_style) {

This should be typehinted

+++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleController.phpundefined
@@ -0,0 +1,178 @@
+   * @return BinaryFileResponse|Response

These should be namespaced

+++ b/core/modules/image/lib/Drupal/image/EventSubscriber/RouteSubscriber.phpundefined
@@ -0,0 +1,53 @@
+   * Register dynamic routes for image styles.

Registers

+++ b/core/modules/image/lib/Drupal/image/EventSubscriber/RouteSubscriber.phpundefined
@@ -0,0 +1,53 @@
+   * Generate image derivatives of publicly available files.
+   * If clean URLs are disabled, image derivatives will always be served
+   * through the menu system.
+   * If clean URLs are enabled and the image derivative already exists,
+   * PHP will be bypassed.

This is wrapped strangely

+++ b/core/modules/system/lib/Drupal/system/FileController.phpundefined
@@ -0,0 +1,98 @@
+   *   Thrown when the

Missing the end of the sentence

+++ b/core/modules/system/lib/Drupal/system/PathProcessor/PathProcessorFiles.phpundefined
@@ -0,0 +1,67 @@
+class PathProcessorFiles implements InboundPathProcessorInterface {

Missing docblock

dawehner’s picture

Thank you for the review.

Status: Needs review » Needs work

The last submitted patch, file_download-1987712-29.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
28.78 KB

Another rerole.

tim.plunkett’s picture

+++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleController.phpundefined
@@ -0,0 +1,179 @@
+   * @param string $image_style
...
+  public function deliver(Request $request, $scheme, $image_style) {
...
+      $valid = $valid && $request->query->get(IMAGE_DERIVATIVE_TOKEN) === image_style_path_token($image_style->name, $scheme . '://' . $target);
...
+    $derivative_uri = image_style_path($image_style->id(), $image_uri);

This isn't a string, it's \Drupal\image\ImageStyleInterface

dawehner’s picture

Maybe this is the final one.

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

The last submitted patch, file_download-1987712-33.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion

#33: file_download-1987712-33.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for fixing that, the rest of the patch already looked great.

alexpott’s picture

+++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.phpundefined
@@ -64,7 +81,10 @@ public function downloadExport() {
+    $file_controller = new FileController($this->moduleHandler);
+    $request = new Request(array('file' => 'config.tar.gz'));
+    return $file_controller->fileDownload($request, 'temporary');

That's pretty cool

+++ b/core/modules/system/lib/Drupal/system/PathProcessor/PathProcessorFiles.phpundefined
@@ -0,0 +1,70 @@
+/**
+ * Defines a path processor to rewrite file URLs.
+ */

Could do with some more documentation to explain what is going on here...

Also I think I think this should be split into 2 as the image stuff is only required if the image module is enabled.

+++ b/core/modules/system/lib/Drupal/system/PathProcessor/PathProcessorFiles.phpundefined
@@ -0,0 +1,70 @@
+    if (strpos($path, $directory_path . '/styles') === 0) {
+      $rest = str_replace($directory_path .'/styles/', '', $path);

I think this should be if (strpos($path, $directory_path . '/styles/') === 0) { as this would match styles_for_llamas.pdf :)

+++ b/core/modules/system/lib/Drupal/system/PathProcessor/PathProcessorFiles.phpundefined
@@ -0,0 +1,70 @@
+    elseif (strpos($path, 'system/files/styles') === 0) {
+      $rest = str_replace('system/files/styles/', '', $path);

I think the strpos needs a trailing slash for the same reason above.

+++ b/core/modules/system/lib/Drupal/system/PathProcessor/PathProcessorFiles.phpundefined
@@ -0,0 +1,70 @@
+    elseif (strpos($path, 'system/files') === 0) {
+      $file_path = str_replace('system/files/', '', $path);

As above

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
dawehner’s picture

Status: Needs work » Needs review
FileSize
30.88 KB

It is quite late, so i am not sure whether the documentation is actually fine.

Status: Needs review » Needs work

The last submitted patch, drupal-1987712-39.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
802 bytes
30.92 KB

The problem was that the files processor rewrites the url again.

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

The last submitted patch, file_download-1987712-41.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#41: file_download-1987712-41.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, file_download-1987712-41.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion

#41: file_download-1987712-41.patch queued for re-testing.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Seems all addressed

andypost’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleController.phpundefined
@@ -0,0 +1,181 @@
+class ImageStyleController extends FileController implements ControllerInterface {

We can't call this ImageStyleController because it clashes with all the over possible controllers... eg. list.

Perhaps ImageStyleFileService might be good as this class is responsible for serving files generated for image styles.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.86 KB
30.9 KB

I kind of like to use deliver.

andypost’s picture

+++ b/core/modules/image/image.routing.ymlundefined
@@ -11,3 +11,11 @@ image_effect_delete:
+    _controller:  '\Drupal\image\Controller\ImageStyleDeliver::deliver'

+++ b/core/modules/system/system.routing.ymlundefined
@@ -144,6 +144,14 @@ system_admin_index:
+    _controller: 'Drupal\system\FileController::fileDownload'

Lets unify them!

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleDeliver.phpundefined
@@ -0,0 +1,181 @@
+class ImageStyleDeliver extends FileController implements ControllerInterface {

ImageStyleDeliver::deliver() is weird...

I like deliver lets change the class name to ImageStyleFileService

+++ b/core/modules/system/lib/Drupal/system/FileController.phpundefined
@@ -0,0 +1,100 @@
+class FileController implements ControllerInterface {

FileController seems to general... maybe FileService as this is for serving files...

+++ b/core/modules/system/lib/Drupal/system/FileController.phpundefined
@@ -0,0 +1,100 @@
+  public function fileDownload(Request $request, $scheme = 'private') {

change to deliver

I think we should have a FileServiceInterface that has a deliver method...

andypost’s picture

Status: Needs work » Needs review
FileSize
30.34 KB
3.79 KB

Re-roll. Renamed files/classes. Also common.inc change does not needed anymore

+++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleDeliver.phpundefined
@@ -0,0 +1,181 @@
+  public function deliver(Request $request, $scheme, ImageStyleInterface $image_style) {

+++ b/core/modules/system/lib/Drupal/system/FileController.phpundefined
@@ -0,0 +1,100 @@
+  public function fileDownload(Request $request, $scheme = 'private') {

No reason in interface because number of arguments is different

Status: Needs review » Needs work

The last submitted patch, file-deliver-1987712-52.patch, failed testing.

Crell’s picture

Alex: Almost every other controller we have is called *Controller. Suffixing it with service is wrong, because it's not a service. It's not a read-only utility library available via the container.

If the confusion here is with entity controllers, well, that's why we've been trying to rename those for something like 10 months. :-) Those are misnamed. There's no collision here because namespaces.

andypost’s picture

Status: Needs work » Needs review
FileSize
30.42 KB
3.1 KB

Fixed rename

Status: Needs review » Needs work

The last submitted patch, file-deliver-1987712-55.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review

andypost: Sorry, calling a controller *Service is just begging for confusion. :-(

dawehner’s picture

FileSize
30.59 KB

Well, our idea was that this controller serves files. Should we just go with ImageStyleDownloadController->deliver() and FileDownloadController->deliver()?

Status: Needs review » Needs work

The last submitted patch, drupal-1987712-58.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
810 bytes
30.59 KB

This should fix the failures.

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

The last submitted patch, drupal-1987712-60.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review

#60: drupal-1987712-60.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-1987712-60.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#60: drupal-1987712-60.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion

The last submitted patch, drupal-1987712-60.patch, failed testing.

claudiu.cristea’s picture

+++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleDownloadController.phpundefined
@@ -0,0 +1,181 @@
+  public function deliver(Request $request, $scheme, ImageStyleInterface $image_style) {

A decision need to be taken here. Do we need this controller or it's OK to have the ->deliver() method only in ImageStyleInterface with ImageStyle implementation (see #2027423: Make image style system flexible).

+++ b/core/modules/image/lib/Drupal/image/PathProcessor/PathProcessorImageStyles.phpundefined
@@ -0,0 +1,77 @@
+class PathProcessorImageStyles implements InboundPathProcessorInterface {

Are modules able to overwrite this? The idea in #2027423: Make image style system flexible was to allow modules to define arbitrary stream wrappers, paths, and permissions for different derivatives of the same image. I'm OK if this can be easily overwritten.

+++ b/core/modules/image/lib/Drupal/image/PathProcessor/PathProcessorImageStyles.phpundefined
@@ -0,0 +1,77 @@
+      $rest = str_replace($directory_path .'/styles/', '', $path);

This will break the hypothetical path sites/default/files/styles/medium/public/sites/default/files/styles/image.png :)

Maybe next replace code would be safer:

$rest = preg_replace("|^$directory_path/styles/|", '', $path);
dawehner’s picture

A decision need to be taken here. Do we need this controller or it's OK to have the ->deliver() method only in ImageStyleInterface with ImageStyle implementation (see #2027423: Make image style system flexible).

We need something on the controller in order to render it at all through the route system.

I think this should then call the deliver method on the image style ...

Are modules able to overwrite this? The idea in #2027423: Make image style system flexible was to allow modules to define arbitrary stream wrappers, paths, and permissions for different derivatives of the same image. I'm OK if this can be easily overwritten.

Well, modules could implement their own path processor to do all what is needed.

claudiu.cristea’s picture

We need something on the controller in order to render it at all through the route system.

I like this! In this case deliver() should be something like:

public function deliver(Request $request, $scheme, ImageStyleInterface $image_style) {
  $target = $request->query->get('file');
  return $image_style->deliver($request, $scheme, $target);
}
dawehner’s picture

From a personal point of view the image style should not build the response object but just get the image as content.

claudiu.cristea’s picture

Assigned: Unassigned » claudiu.cristea
Status: Needs work » Needs review
FileSize
9.22 KB
31.75 KB

Should we just go with ImageStyleDownloadController->deliver() and FileDownloadController->deliver()?

We cannot use the same ->deliver() method name as they take different arguments and we're running into:

Strict warning: Declaration of Drupal\image\Controller\ImageStyleDownloadController::deliver() should be compatible with Drupal\system\FileDownloadController::deliver(Symfony\Component\HttpFoundation\Request $request, $scheme = 'private') in require() (line 26 of core/modules/image/lib/Drupal/image/Controller/ImageStyleDownloadController.php).

Here's a new patch.

claudiu.cristea’s picture

As this turned green, here are some toughs on this issue combined with #2027423: Make image style system flexible:

  • Based on #69 I agree that ->deliver() should belong to controller (ImageStyleDownloadController) and not to image style (ImageStyleInterface, ImageStyle). ImageStyleDownloadController should be made responsible for request handling and response building but derivative creation (ImageStyleInterface::createDerivative(), former image_style_create_derivative()), along with derivative URI & URL building, should be provided by the image style object (already injected). ->deliver() to be removed from #2027423: Make image style system flexible.
  • Security token (itok) generation (image_style_path_token() -> ImageStyleDownloadController::getPathToken()) should be moved in ImageStyleDownloadController as it's controller business. It should be removed from #2027423: Make image style system flexible patch.
  • This patch should be committed before #2027423: Make image style system flexible.
  • This patch still needs cleanup.

Please provide some feedback and review.

dawehner’s picture

I agree that we should clearly separate the concerns of file download/image style download controller,
so put the itok into the controller as well as the actual response.

What kind of stuff do you have in mind which needs cleanup in this patch?

claudiu.cristea’s picture

Just:

  1. Move image_style_path_token() -> ImageStyleDownloadController::getPathToken()
  2. phpDoc cleanup and use statements order.

Also can you take a look at deliver() from #2027423: Make image style system flexible. There are some differences on how headers are collected. Also on error throwing.

andypost’s picture

I see this as:
- FileDeliveryInterface that provides token handling and responce generation with access taken in to account.
- Base imlmentation in the class
- FileController in system module for base route
- ImageDerivativeController implementation for additional routing in #2033669: Image file objects should be classed
- ImageStyle->buildDerivative() for transformation via effect plugin #1821854: Convert image effects into plugins

claudiu.cristea’s picture

FileSize
4.28 KB
31.31 KB

Working on patch, based on #71 and #72 I found that image_style_path_token() is used also in image style URL generation -- image_style_url(). That procedural function will be moved as method in ImageStyle, because image style class should be made responsible on how they build derivatives URI and URL (that is the idea from #2027423: Make image style system flexible). So, it make no sense to have the token generation in the controller while it seems logical to be part of ImageStyleInterface implementation (but not in the interface). ImageStyle is the implementation of ImageStyleInterface shipped with Drupal and will define its own security strategy (itok is part of it). Other ImageStyleInterface may decide other security strategies depending on their implementations.

For now I let image_style_path_token() as procedural. It will be moved to ImageStyle in #2027423: Make image style system flexible. IMO this patch should be committed first.

dawehner’s picture

+++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleDownloadController.phpundefined
@@ -102,16 +103,14 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
+      $valid = $valid && $request->query->get(IMAGE_DERIVATIVE_TOKEN) === image_style_path_token($image_style->id(), $image_uri);

You could also use something like $valid &= $request->...

+++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleDownloadController.phpundefined
@@ -124,20 +123,13 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
-        if (count($headers)) {
-          foreach ($headers as $name => $value) {
-            $response->headers->set($name, $value);
-          }
-        }

So i see, this will be used later in the += array() line.

claudiu.cristea’s picture

FileSize
1.95 KB
31.28 KB

Small fixes from #76 and phpdoc.

claudiu.cristea’s picture

There’s is still a confusion on what tasks should be handled by ImageStyleDownloadController (specifically by ->deliver()) and what should go in ImageStyle itself (ImageStyleInterface implementation). See also #2027423: Make image style system flexible.

I just want to let you know my point of view:

ImageStyle is the default ImageStyleInterface implementation shipped with Drupal 8. When shipping Drupal with ImageStyle next assumptions are made (similar to procedural D7):

  • Derivatives are inheriting the original stream wrapper
  • Derivatives are placed under a fixed location: public://styles/[style_name]/public/[file] or private://styles/[style_name]/private/[file].
  • Derivatives (all of them) are inheriting the original access permissions.

Next pieces of code from this patch are implementing these assumptions:

+  public function deliver(Request $request, $scheme, ImageStyleInterface $image_style) {
...
+    if (!$this->configFactory->get('image.settings')->get('allow_insecure_derivatives')) {
+      $valid &= $request->query->get(IMAGE_DERIVATIVE_TOKEN) === image_style_path_token($image_style->id(), $image_uri);
+    }
+    if (!$valid) {
+      throw new AccessDeniedHttpException();
+    }
...

+    if ($scheme == 'private') {
...
+    }
...
+  }

IMO we need to move these pieces of code under ImageStyle and let the ImageStyleDownloadController::deliver() only wrap around them after #2027423: Make image style system flexible will get committed. In this way a contrib module can implement his own ImageStyleInterface class and can handle permissions for example in a different way. But that should not be contained in controller as much as this is possible.

Let me know your thoughts...

claudiu.cristea’s picture

Status: Needs review » Needs work
FileSize
28.47 KB

This is just a patch reworked in top of #2027423: Make image style system flexible. Passed the local testing, no testing requires because needs work.

claudiu.cristea’s picture

Issue tags: +WSCCI
claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
6.83 KB
33.82 KB

Regretfully I wasn't able to link ImageStyleDownloadController by annotating ImageStyle. For me this was important as ImageStyle will serve also as example for future implementations and has to provide a way to access its own controller. This may be a followup (as ImageStyle::getPathToken() should be moved in ImageStyleDownloadController because is not image style related).

alexpott’s picture

Why does ImageStyleDownloadController extend FileDownloadController? Afaics ImageStyleDownloadController does not use anything it inherits from FileDownloadController.

claudiu.cristea’s picture

@alexpott, good question! That was something that I inherited from early patches.

There was an attempt to use the same deliver/download method but this cannot be achieved because they take different arguments. See #70.

claudiu.cristea’s picture

+++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleDownloadController.phpundefined
@@ -0,0 +1,170 @@
+      if (file_exists($derivative_uri)) {
+        return parent::download($request, $scheme);
+      }

In fact there is the parent::download() used from FileDownloadController

claudiu.cristea’s picture

From IRC:

[5:02pm] claudiu_cristea: alexpott: when you have time let's make a decision on https://drupal.org/node/1987712#comment-7632171. But removing FileDownloadController as ancestor means that we have to inject it somehow in the deliver() method. Even if the class it doesn't inherit anything from FileDownloadController it still make sense — both controllers are doing the same: downloading files
[5:02pm] Druplicon: https://drupal.org/node/1987712 => Convert file_download() to a new style controller #1987712: Convert file_download() to a new style controller => Drupal core, file system, normal, needs review, 84 comments, 24 IRC mentions
[5:03pm] alexpott: claudiu_cristea: I'm fine with extends… I just missed the call to parent 
claudiu.cristea’s picture

FileSize
686 bytes
33.81 KB
[9:47pm] dawehner_: claudiu_cristea: do you know why $scheme can be an int?
[9:48pm] claudiu_cristea: dawehner_: no
[9:48pm] dawehner_: claudiu_cristea: lets change this then
[9:48pm] claudiu_cristea: dawehner_: what?
[9:48pm] dawehner_: claudiu_cristea: see FileDownloadController::download
[9:48pm] dawehner_: @param int|string $scheme
...
[9:51pm] claudiu_cristea: dawehner_: looking in .routing.yml it seems that {scheme} is always "private", right?
[9:51pm] dawehner_: claudiu_cristea: well, we should support any kind of string but yeah
[9:53pm] claudiu_cristea: dawehner_: and we check inside download() for wrong (disabled) scheme
[9:53pm] claudiu_cristea: dawehner_: I see no reason for $scheme being an int
[9:53pm] dawehner_: claudiu_cristea: me neither
dawehner’s picture

This feels really good now.

+++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleDownloadController.phpundefined
@@ -0,0 +1,170 @@
+  /**
+   * The module handler.
+   *
+   * @var \Drupal\Core\Extension\ModuleHandlerInterface
+   */
+  protected $moduleHandler;
...
+    $this->moduleHandler = $module_handler;

+++ b/core/modules/system/lib/Drupal/system/FileDownloadController.phpundefined
@@ -0,0 +1,99 @@
+  /**
+   * The module handler.
+   *
+   * @var \Drupal\Core\Extension\ModuleHandlerInterface
+   */
+  protected $moduleHandler;
+
+  /**
+   * Constructs a FileDownloadController object.
+   *
+   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
+   *   The module handler.
+   */
+  public function __construct(ModuleHandlerInterface $module_handler) {
+    $this->moduleHandler = $module_handler;
+  }

Instead of assinging this for outself, just call parent::__construct with the module handler.

claudiu.cristea’s picture

FileSize
1.13 KB
33.68 KB

Voilà!

andypost’s picture

+1 RTBC, small nitpick

+++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleDownloadController.phpundefined
@@ -0,0 +1,163 @@
+  public function __construct(ConfigFactory $config_factory, ModuleHandlerInterface $module_handler, LockBackendInterface $lock) {
+    parent::__construct($module_handler);

+++ b/core/modules/system/lib/Drupal/system/FileDownloadController.phpundefined
@@ -0,0 +1,99 @@
+  public function __construct(ModuleHandlerInterface $module_handler) {
+    $this->moduleHandler = $module_handler;

Please, leave $module_handler first

claudiu.cristea’s picture

FileSize
2.43 KB
34.15 KB

OK. Removed also some use statements from ImageStyle.

dawehner’s picture

In ImageStyleDownloadController there is no use statement for "throw ServiceUnavailableHttpException".

claudiu.cristea’s picture

FileSize
693 bytes
34.23 KB

Good catch :)

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Great!

tim.plunkett’s picture

+++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.phpundefined
@@ -34,10 +35,21 @@ class ConfigController implements ControllerInterface {
+      $container->get('module_handler')

@@ -64,7 +79,10 @@ public function downloadExport() {
+    $file_controller = new FileDownloadController($this->moduleHandler);

I personally would have had FileDownloadController::create($container) in ConfigController::create(), to keep the factory stuff all in one place...
Because the ConfigController doesn't need the module handler, it just needs the FileDownloadController

claudiu.cristea’s picture

You mean:

class ConfigController implements ControllerInterface {
...
  /**
   * The file download controller.
   *
   * @var \Drupal\system\FileDownloadController;
   */
  protected $fileDownloadController;
...
  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('config.storage'),
      $container->get('config.storage.staging'),
      FileDownloadController::create($container)
    );
  }
...
  public function __construct(StorageInterface $target_storage, StorageInterface $source_storage, ControllerInterface $file_download_controller) {
    $this->targetStorage = $target_storage;
    $this->sourceStorage = $source_storage;
    $this->fileDownloadController = $file_download_controller;
  }
...
}
claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.65 KB
34.54 KB

You're right, FileDownloadController was hardcoded, now it's injected. Cleaner.

claudiu.cristea’s picture

FileSize
611 bytes
34.54 KB

And a small doc fix.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This is cool and tricky!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleDownloadController.phpundefined
@@ -0,0 +1,164 @@
+      return new Response(t('Error generating image, missing source file.'), 404);
...
+      return new Response(t('Error generating image.'), 500);

t can be injected now using the translation service.

claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.89 KB
35.14 KB

OK, but let's kick this in! Otherwise will have to collect all ongoing conversions :)

jibran’s picture

Status: Needs work » Reviewed & tested by the community

back to RTBC

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleDownloadController.phpundefined
@@ -49,11 +57,14 @@ class ImageStyleDownloadController extends FileDownloadController implements Con
+  public function __construct(ModuleHandlerInterface $module_handler, ConfigFactory $config_factory, LockBackendInterface $lock, TranslatorInterface $translation_manager) {

I would love to not call it translation manager, because the interface is not talking about the manager, as this is just an implementation detail. What about $this->translation or even $this->t->

claudiu.cristea’s picture

Well, being nostalgic to Drupal =< 7 I'm tempted to use $this->t-> because it reminds me the old t() :)

But I think $this->translation fits better the "new world order".

dawehner’s picture

At least the two usages in LocalActionBase and LocalTaskBase also go with ->t, so what about adapting that, if you already prefer it ...

claudiu.cristea’s picture

At least the two usages in LocalActionBase and LocalTaskBase also go with ->t, so what about adapting that, if you already prefer it ...

Drupalism :)

Or what about $this->translator as it fits more the interface?

claudiu.cristea’s picture

FileSize
3.46 KB
35.06 KB

Back

dawehner’s picture

I don't care that much, that is still RTBC.

alexpott’s picture

Title: Convert file_download() to a new style controller » Change notice: Convert file_download() to a new style controller
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 36d647e and pushed to 8.x. Thanks!

+++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.phpundefined
@@ -64,7 +79,9 @@ public function downloadExport() {
-    return file_download('temporary', 'config.tar.gz');
+
+    $request = new Request(array('file' => 'config.tar.gz'));
+    return $this->fileDownloadController->download($request, 'temporary');

We need a change notice to explain this change...

dawehner’s picture

jibran’s picture

Title: Change notice: Convert file_download() to a new style controller » Convert file_download() to a new style controller
Assigned: claudiu.cristea » Unassigned
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record

Fixed some docs. Looks fine to me.

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

Anonymous’s picture

Issue summary: View changes

update