Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#106 | drupal-1987712-106.patch | 35.06 KB | claudiu.cristea |
#106 | interdiff.txt | 3.46 KB | claudiu.cristea |
#100 | drupal-1987712-100.patch | 35.14 KB | claudiu.cristea |
#100 | interdiff.txt | 3.89 KB | claudiu.cristea |
#97 | drupal-1987712-97.patch | 34.54 KB | claudiu.cristea |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedDoing this one.
Comment #2
damiankloip CreditAttribution: damiankloip commentedI 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.
Comment #3
damiankloip CreditAttribution: damiankloip commentedok, 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.
Comment #5
damiankloip CreditAttribution: damiankloip commentedHmm, I think this might fix the tests.
Comment #7
Crell CreditAttribution: Crell commentedamateescu 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.
Comment #8
amateescu CreditAttribution: amateescu commentedThe name is 'file', not 'query'..
Comment #9
damiankloip CreditAttribution: damiankloip commentedYeah, 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.
Comment #10
damiankloip CreditAttribution: damiankloip commentedSorry, wrong patch uploaded in #9
Comment #11
dawehnerAs 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.
Comment #13
dawehnerAssign to myself.
Comment #14
dawehnerI managed to get rid of any kind of rewrite and just do a inbound path processor.
Comment #15
Crell CreditAttribution: Crell commentedWe'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.
It MAY be scope creep, but most of this code belongs in a service or utility object, not in the controller, I think.
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.
Convert this to returning a response object. This is very easy to cleanup while we're here, so let's do so.
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.
Comment #16
dawehnerI 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.
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
Comment #17
Crell CreditAttribution: Crell commentedWithout 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.
Comment #19
dawehnerSome work done, though there are still some failing tests.
Comment #21
dawehnerFixing all beside one test failure.
Comment #23
dawehnerThis was actually easier to fix then expected.
Comment #25
dawehnerDoh, that is the wrong patch.
Comment #27
dawehnerFixed the last failure on ConfigController.
Comment #28
tim.plunkettPlease split this onto multiple lines for easier future additions
This should be typehinted
These should be namespaced
Registers
This is wrapped strangely
Missing the end of the sentence
Missing docblock
Comment #29
dawehnerThank you for the review.
Comment #31
dawehnerAnother rerole.
Comment #32
tim.plunkettThis isn't a string, it's \Drupal\image\ImageStyleInterface
Comment #33
dawehnerMaybe this is the final one.
Comment #35
tim.plunkett#33: file_download-1987712-33.patch queued for re-testing.
Comment #36
tim.plunkettThanks for fixing that, the rest of the patch already looked great.
Comment #37
alexpottThat's pretty cool
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.
I think this should be
if (strpos($path, $directory_path . '/styles/') === 0) {
as this would match styles_for_llamas.pdf :)I think the strpos needs a trailing slash for the same reason above.
As above
Comment #38
alexpottComment #39
dawehnerIt is quite late, so i am not sure whether the documentation is actually fine.
Comment #41
dawehnerThe problem was that the files processor rewrites the url again.
Comment #43
dawehner#41: file_download-1987712-41.patch queued for re-testing.
Comment #45
dawehner#41: file_download-1987712-41.patch queued for re-testing.
Comment #46
andypostSeems all addressed
Comment #47
andypostIssues #2027423: Make image style system flexible depends on this.
Comment #48
alexpottWe 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.
Comment #49
dawehnerI kind of like to use deliver.
Comment #50
andypostLets unify them!
Comment #51
alexpottImageStyleDeliver::deliver() is weird...
I like deliver lets change the class name to ImageStyleFileService
FileController seems to general... maybe FileService as this is for serving files...
change to deliver
I think we should have a FileServiceInterface that has a deliver method...
Comment #52
andypostRe-roll. Renamed files/classes. Also
common.inc
change does not needed anymoreNo reason in interface because number of arguments is different
Comment #54
Crell CreditAttribution: Crell commentedAlex: 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.
Comment #55
andypostFixed rename
Comment #57
Crell CreditAttribution: Crell commentedandypost: Sorry, calling a controller *Service is just begging for confusion. :-(
Comment #58
dawehnerWell, our idea was that this controller serves files. Should we just go with ImageStyleDownloadController->deliver() and FileDownloadController->deliver()?
Comment #60
dawehnerThis should fix the failures.
Comment #62
vijaycs85#60: drupal-1987712-60.patch queued for re-testing.
Comment #64
dawehner#60: drupal-1987712-60.patch queued for re-testing.
Comment #66
claudiu.cristeaA decision need to be taken here. Do we need this controller or it's OK to have the
->deliver()
method only inImageStyleInterface
withImageStyle
implementation (see #2027423: Make image style system flexible).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.
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:
Comment #67
dawehnerWe 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 ...
Well, modules could implement their own path processor to do all what is needed.
Comment #68
claudiu.cristeaI like this! In this case
deliver()
should be something like:Comment #69
dawehnerFrom a personal point of view the image style should not build the response object but just get the image as content.
Comment #70
claudiu.cristeaWe cannot use the same
->deliver()
method name as they take different arguments and we're running into:Here's a new patch.
Comment #71
claudiu.cristeaAs this turned green, here are some toughs on this issue combined with #2027423: Make image style system flexible:
->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()
, formerimage_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.itok
) generation (image_style_path_token()
->ImageStyleDownloadController::getPathToken()
) should be moved inImageStyleDownloadController
as it's controller business. It should be removed from #2027423: Make image style system flexible patch.Please provide some feedback and review.
Comment #72
dawehnerI 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?
Comment #73
claudiu.cristeaJust:
image_style_path_token()
->ImageStyleDownloadController::getPathToken()
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.Comment #74
andypostI 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
Comment #75
claudiu.cristeaWorking 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 inImageStyle
, 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 ofImageStyleInterface
implementation (but not in the interface).ImageStyle
is the implementation ofImageStyleInterface
shipped with Drupal and will define its own security strategy (itok is part of it). OtherImageStyleInterface
may decide other security strategies depending on their implementations.For now I let
image_style_path_token()
as procedural. It will be moved toImageStyle
in #2027423: Make image style system flexible. IMO this patch should be committed first.Comment #76
dawehnerYou could also use something like $valid &= $request->...
So i see, this will be used later in the += array() line.
Comment #77
claudiu.cristeaSmall fixes from #76 and phpdoc.
Comment #78
claudiu.cristeaThere’s is still a confusion on what tasks should be handled by
ImageStyleDownloadController
(specifically by->deliver()
) and what should go inImageStyle
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 defaultImageStyleInterface
implementation shipped with Drupal 8. When shipping Drupal withImageStyle
next assumptions are made (similar to procedural D7):public://styles/[style_name]/public/[file]
orprivate://styles/[style_name]/private/[file]
.Next pieces of code from this patch are implementing these assumptions:
IMO we need to move these pieces of code under
ImageStyle
and let theImageStyleDownloadController::deliver()
only wrap around them after #2027423: Make image style system flexible will get committed. In this way a contrib module can implement his ownImageStyleInterface
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...
Comment #79
claudiu.cristeaThis is just a patch reworked in top of #2027423: Make image style system flexible. Passed the local testing, no testing requires because needs work.
Comment #80
claudiu.cristeaComment #81
claudiu.cristeaRegretfully I wasn't able to link
ImageStyleDownloadController
by annotatingImageStyle
. For me this was important asImageStyle
will serve also as example for future implementations and has to provide a way to access its own controller. This may be a followup (asImageStyle::getPathToken()
should be moved inImageStyleDownloadController
because is not image style related).Comment #82
alexpottWhy does ImageStyleDownloadController extend FileDownloadController? Afaics ImageStyleDownloadController does not use anything it inherits from FileDownloadController.
Comment #83
claudiu.cristea@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.
Comment #84
claudiu.cristeaIn fact there is the
parent::download()
used fromFileDownloadController
Comment #85
claudiu.cristeaFrom IRC:
Comment #86
claudiu.cristeaComment #87
dawehnerThis feels really good now.
Instead of assinging this for outself, just call parent::__construct with the module handler.
Comment #88
claudiu.cristeaVoilà!
Comment #89
andypost+1 RTBC, small nitpick
Please, leave $module_handler first
Comment #90
claudiu.cristeaOK. Removed also some
use
statements fromImageStyle
.Comment #91
dawehnerIn ImageStyleDownloadController there is no use statement for "throw ServiceUnavailableHttpException".
Comment #92
claudiu.cristeaGood catch :)
Comment #93
andypostGreat!
Comment #94
tim.plunkettI 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
Comment #95
claudiu.cristeaYou mean:
Comment #96
claudiu.cristeaYou're right,
FileDownloadController
was hardcoded, now it's injected. Cleaner.Comment #97
claudiu.cristeaAnd a small doc fix.
Comment #98
dawehnerThis is cool and tricky!
Comment #99
alexpottt can be injected now using the translation service.
Comment #100
claudiu.cristeaOK, but let's kick this in! Otherwise will have to collect all ongoing conversions :)
Comment #101
jibranback to RTBC
Comment #102
dawehnerI 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->
Comment #103
claudiu.cristeaWell, being nostalgic to Drupal =< 7 I'm tempted to use
$this->t->
because it reminds me the oldt()
:)But I think
$this->translation
fits better the "new world order".Comment #104
dawehnerAt least the two usages in LocalActionBase and LocalTaskBase also go with ->t, so what about adapting that, if you already prefer it ...
Comment #105
claudiu.cristeaDrupalism :)
Or what about
$this->translator
as it fits more the interface?Comment #106
claudiu.cristeaBack
Comment #107
dawehnerI don't care that much, that is still RTBC.
Comment #108
alexpottCommitted 36d647e and pushed to 8.x. Thanks!
We need a change notice to explain this change...
Comment #109
dawehnerhttps://drupal.org/node/2047751
Comment #110
jibranFixed some docs. Looks fine to me.
Comment #111.0
(not verified) CreditAttribution: commentedupdate