Convert file_download() to a new style controller

Doing this one.

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.

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.

Hmm, I think this might fix the tests.

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.

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

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.

Sorry, wrong patch uploaded in #9

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.

Assign to myself.

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

+++ 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.

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

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.

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

Fixing all beside one test failure.

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());

Doh, that is the wrong patch.

Fixed the last failure on ConfigController.

+++ 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

Thank you for the review.

Another rerole.

+++ 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

Maybe this is the final one.

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

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

+++ 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

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

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

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

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

Seems all addressed

Issues #2027423: Make image style system flexible depends on this.

+++ 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.

I kind of like to use deliver.

+++ 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!

+++ 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...

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

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.

Fixed rename

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

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

This should fix the failures.

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

+++ 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);

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.

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);
}

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

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.

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.

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?

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.

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

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.

+++ 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.

Small fixes from #76 and phpdoc.

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...

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.

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).

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

@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.

+++ 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

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 

[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

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.

Voilà!

+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

OK. Removed also some use statements from ImageStyle.

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

Good catch :)

Great!

+++ 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

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;
  }
...
}

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

And a small doc fix.

This is cool and tricky!

+++ 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.

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

back to RTBC

+++ 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->

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".

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

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?

Back

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

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...

https://drupal.org/node/2047751

Fixed some docs. Looks fine to me.