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.

Files: 
CommentFileSizeAuthor
#106 drupal-1987712-106.patch35.06 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 57,283 pass(es).
[ View ]
#106 interdiff.txt3.46 KBclaudiu.cristea
#100 drupal-1987712-100.patch35.14 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 57,188 pass(es).
[ View ]
#100 interdiff.txt3.89 KBclaudiu.cristea
#97 drupal-1987712-97.patch34.54 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 56,574 pass(es).
[ View ]
#97 interdiff.txt611 bytesclaudiu.cristea
#96 drupal-1987712-96.patch34.54 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 56,571 pass(es).
[ View ]
#96 interdiff.txt2.65 KBclaudiu.cristea
#92 drupal-1987712-92.patch34.23 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 57,141 pass(es).
[ View ]
#92 interdiff.txt693 bytesclaudiu.cristea
#90 drupal-1987712-90.patch34.15 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 56,970 pass(es).
[ View ]
#90 interdiff.txt2.43 KBclaudiu.cristea
#88 drupal-1987712-88.patch33.68 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 57,070 pass(es).
[ View ]
#88 interdiff.txt1.13 KBclaudiu.cristea
#86 drupal-1987712-86.patch33.81 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 57,107 pass(es).
[ View ]
#86 interdiff.txt686 bytesclaudiu.cristea
#81 drupal-1987712-81.patch33.82 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 57,038 pass(es).
[ View ]
#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
PASSED: [[SimpleTest]]: [MySQL] 56,872 pass(es).
[ View ]
#77 interdiff.txt1.95 KBclaudiu.cristea
#75 drupal-1987712-75.patch31.31 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 56,813 pass(es).
[ View ]
#75 interdiff.txt4.28 KBclaudiu.cristea
#70 drupal-1987712-70.patch31.75 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 56,980 pass(es).
[ View ]
#70 interdiff.txt9.22 KBclaudiu.cristea
#60 drupal-1987712-60.patch30.59 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,785 pass(es), 16 fail(s), and 13 exception(s).
[ View ]
#60 interdiff.txt810 bytesdawehner
#58 drupal-1987712-58.patch30.59 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,920 pass(es), 16 fail(s), and 13 exception(s).
[ View ]
#55 interdiff.txt3.1 KBandypost
#55 file-deliver-1987712-55.patch30.42 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 56,076 pass(es), 16 fail(s), and 13 exception(s).
[ View ]
#52 interdiff.txt3.79 KBandypost
#52 file-deliver-1987712-52.patch30.34 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 56,075 pass(es), 17 fail(s), and 13 exception(s).
[ View ]
#49 drupal-1987712-49.patch30.9 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,380 pass(es).
[ View ]
#49 interdiff.txt1.86 KBdawehner
#41 file_download-1987712-41.patch30.92 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,900 pass(es).
[ View ]
#41 interdiff.txt802 bytesdawehner
#39 drupal-1987712-39.patch30.88 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,200 pass(es), 25 fail(s), and 2 exception(s).
[ View ]
#33 file_download-1987712-33.patch28.93 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,357 pass(es).
[ View ]
#33 interdiff.txt1.6 KBdawehner
#31 file_download-1987712-31.patch28.78 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#29 file_download-1987712-29.patch28.71 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_download-1987712-29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#29 interdiff.txt4.77 KBdawehner
#27 file_download-1987712-27.patch28.34 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,026 pass(es).
[ View ]
#27 interdiff.txt2.45 KBdawehner
#25 file_download_controllers-1987712-25.patch25.89 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,012 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#23 file_download_controllers-1987712-21.patch26.74 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_download_controllers-1987712-21_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#21 file_download_controllers-1987712-21.patch26.74 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,440 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#21 interdiff.txt648 bytesdawehner
#19 file_download_controllers-1987712-19.patch26.76 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,037 pass(es), 20 fail(s), and 0 exception(s).
[ View ]
#16 interdiff.txt4.33 KBdawehner
#16 file_download_controllers-1987712-16.patch21.72 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,662 pass(es), 70 fail(s), and 3 exception(s).
[ View ]
#14 file_download_controllers-1987712-14.patch21.16 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,568 pass(es), 68 fail(s), and 3 exception(s).
[ View ]
#11 drupal-1987712-11.patch9.39 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,042 pass(es), 28 fail(s), and 2 exception(s).
[ View ]
#10 1987712-9.patch7.92 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 57,002 pass(es), 25 fail(s), and 2 exception(s).
[ View ]
#9 1987712-9.patch2.52 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987712-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 interdiff-1987712-9.txt2.52 KBdamiankloip
#5 1987712-4.patch7.35 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 56,817 pass(es), 25 fail(s), and 2 exception(s).
[ View ]
#5 interdiff-1987712-4.txt2.77 KBdamiankloip
#3 1987712-3.patch7.32 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 55,926 pass(es), 27 fail(s), and 2 exception(s).
[ View ]
#3 interdiff-1987712-3.txt3.42 KBdamiankloip
#3 interdiff-1987712-3.1.txt654 bytesdamiankloip
#2 1987712.patch5.76 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 55,791 pass(es), 25 fail(s), and 1 exception(s).
[ View ]

Comments

Assigned:Unassigned» damiankloip

Doing this one.

Status:Active» Needs review
StatusFileSize
new5.76 KB
FAILED: [[SimpleTest]]: [MySQL] 55,791 pass(es), 25 fail(s), and 1 exception(s).
[ View ]

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.

StatusFileSize
new654 bytes
new3.42 KB
new7.32 KB
FAILED: [[SimpleTest]]: [MySQL] 55,926 pass(es), 27 fail(s), and 2 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new2.77 KB
new7.35 KB
FAILED: [[SimpleTest]]: [MySQL] 56,817 pass(es), 25 fail(s), and 2 exception(s).
[ View ]

Hmm, I think this might fix the tests.

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new2.52 KB
new2.52 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987712-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

StatusFileSize
new7.92 KB
FAILED: [[SimpleTest]]: [MySQL] 57,002 pass(es), 25 fail(s), and 2 exception(s).
[ View ]

Sorry, wrong patch uploaded in #9

StatusFileSize
new9.39 KB
FAILED: [[SimpleTest]]: [MySQL] 57,042 pass(es), 28 fail(s), and 2 exception(s).
[ View ]

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.

Assigned:damiankloip» dawehner

Assign to myself.

Assigned:dawehner» Unassigned
Status:Needs work» Needs review
StatusFileSize
new21.16 KB
FAILED: [[SimpleTest]]: [MySQL] 57,568 pass(es), 68 fail(s), and 3 exception(s).
[ View ]

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.

StatusFileSize
new21.72 KB
FAILED: [[SimpleTest]]: [MySQL] 56,662 pass(es), 70 fail(s), and 3 exception(s).
[ View ]
new4.33 KB

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.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new26.76 KB
FAILED: [[SimpleTest]]: [MySQL] 55,037 pass(es), 20 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new648 bytes
new26.74 KB
FAILED: [[SimpleTest]]: [MySQL] 55,440 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Fixing all beside one test failure.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new26.74 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_download_controllers-1987712-21_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new25.89 KB
FAILED: [[SimpleTest]]: [MySQL] 58,012 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Doh, that is the wrong patch.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new2.45 KB
new28.34 KB
PASSED: [[SimpleTest]]: [MySQL] 58,026 pass(es).
[ View ]

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

StatusFileSize
new4.77 KB
new28.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_download-1987712-29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thank you for the review.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new28.78 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

StatusFileSize
new1.6 KB
new28.93 KB
PASSED: [[SimpleTest]]: [MySQL] 58,357 pass(es).
[ View ]

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.

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

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

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Needs work

Status:Needs work» Needs review
StatusFileSize
new30.88 KB
FAILED: [[SimpleTest]]: [MySQL] 58,200 pass(es), 25 fail(s), and 2 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new802 bytes
new30.92 KB
PASSED: [[SimpleTest]]: [MySQL] 57,900 pass(es).
[ View ]

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.

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.

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

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

Status:Needs review» Reviewed & tested by the community

Seems all addressed

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.

Status:Needs work» Needs review
StatusFileSize
new1.86 KB
new30.9 KB
PASSED: [[SimpleTest]]: [MySQL] 56,380 pass(es).
[ View ]

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!

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

Status:Needs work» Needs review
StatusFileSize
new30.34 KB
FAILED: [[SimpleTest]]: [MySQL] 56,075 pass(es), 17 fail(s), and 13 exception(s).
[ View ]
new3.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.

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.

Status:Needs work» Needs review
StatusFileSize
new30.42 KB
FAILED: [[SimpleTest]]: [MySQL] 56,076 pass(es), 16 fail(s), and 13 exception(s).
[ View ]
new3.1 KB

Fixed rename

Status:Needs review» Needs work

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

Status:Needs work» Needs review

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

StatusFileSize
new30.59 KB
FAILED: [[SimpleTest]]: [MySQL] 55,920 pass(es), 16 fail(s), and 13 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new810 bytes
new30.59 KB
FAILED: [[SimpleTest]]: [MySQL] 55,785 pass(es), 16 fail(s), and 13 exception(s).
[ View ]

This should fix the failures.

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

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

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.

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.

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

<?php
$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:

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

Assigned:Unassigned» claudiu.cristea
Status:Needs work» Needs review
StatusFileSize
new9.22 KB
new31.75 KB
PASSED: [[SimpleTest]]: [MySQL] 56,980 pass(es).
[ View ]

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

StatusFileSize
new4.28 KB
new31.31 KB
PASSED: [[SimpleTest]]: [MySQL] 56,813 pass(es).
[ View ]

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.

StatusFileSize
new1.95 KB
new31.28 KB
PASSED: [[SimpleTest]]: [MySQL] 56,872 pass(es).
[ View ]

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

Status:Needs review» Needs work
StatusFileSize
new28.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.

Issue tags:+WSCCI

Status:Needs work» Needs review
StatusFileSize
new6.83 KB
new33.82 KB
PASSED: [[SimpleTest]]: [MySQL] 57,038 pass(es).
[ View ]

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] => 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

StatusFileSize
new686 bytes
new33.81 KB
PASSED: [[SimpleTest]]: [MySQL] 57,107 pass(es).
[ View ]

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

StatusFileSize
new1.13 KB
new33.68 KB
PASSED: [[SimpleTest]]: [MySQL] 57,070 pass(es).
[ View ]

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

StatusFileSize
new2.43 KB
new34.15 KB
PASSED: [[SimpleTest]]: [MySQL] 56,970 pass(es).
[ View ]

OK. Removed also some use statements from ImageStyle.

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

StatusFileSize
new693 bytes
new34.23 KB
PASSED: [[SimpleTest]]: [MySQL] 57,141 pass(es).
[ View ]

Good catch :)

Status:Needs review» Reviewed & tested by the community

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:

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

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new2.65 KB
new34.54 KB
PASSED: [[SimpleTest]]: [MySQL] 56,571 pass(es).
[ View ]

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

StatusFileSize
new611 bytes
new34.54 KB
PASSED: [[SimpleTest]]: [MySQL] 56,574 pass(es).
[ View ]

And a small doc fix.

Status:Needs review» Reviewed & tested by the community

This is cool and tricky!

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.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new3.89 KB
new35.14 KB
PASSED: [[SimpleTest]]: [MySQL] 57,188 pass(es).
[ View ]

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

Status:Needs work» Reviewed & tested by the community

back to RTBC

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

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?

StatusFileSize
new3.46 KB
new35.06 KB
PASSED: [[SimpleTest]]: [MySQL] 57,283 pass(es).
[ View ]

Back

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

Title:Convert file_download() to a new style controllerChange 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...

Title:Change notice: Convert file_download() to a new style controllerConvert 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.

Issue summary:View changes

update