search

bolt / thumbs

Thumbnail handler for Bolt Images
12 stars 16 forks source link

image aliases added #3703 #29

Closed ToBe998 closed 8 years ago

ToBe998 commented 8 years ago

This is a first try of doing it as much as the current code base does it while not changing to much of it. Feel free to suggest changes...

There will be a complimenting bolt/bolt PR with the new config values and a sample alias inside the theme.yml The sample theme.yml will look like this:

# Aliases can be set to restrict images to a specific set of available
# resolutions or to decouple image sizes from template files.
image_aliases:
    myimageformat:
        size: [400,300]
        cropping: crop

Note The twig filters for image and thumbnail are not done yet. They are probably inside bolt and not bolt-thumbs. Will add them asap!

ToBe998 commented 8 years ago

Short note on why so much code in the controller: I actually dont like that at all, but there is no good way to just force the Responder to return an error image without replicating some login here. The logic for rendering images and also for failing that is completely done inside the various sub classes and not accessible from outside. Since the controller needs to abort requests with wrong alias or when not allowed, i need to serve error images there too. The current mechanism is the best i came up with, without completely changing the error image mechanics. I also didnt want to do to much refactoring and distribute the code over to many classes, but open to suggestions. Just dont overengeneer ;)

CarsonF commented 8 years ago

I actually dont like that at all, but there is no good way to just force the Responder to return an error image without replicating some login here.

Sure there is. You can throw an exception, or return a thumbnail of an error image. In Responder::respond the transction's src image and error image are set. Do your restricted check, and set the src image to the error image on failure (and other props if needed).

ToBe998 commented 8 years ago

Got to try that. Didnt analyse the exception handling there to much. My problem was that the controller can check the routing but only the responder does the exception handling and only when an image wasnt found. I didnt see a way to trigger the Responders error behaviour from inside the Controller.

ToBe998 commented 8 years ago

Didnt check the tests to much, but it appears they didnt work before my changes?

GwendolenLynch commented 8 years ago

I just pulled master and they passed … didn't look into your code though, but either a but in the PR or th e tests need expectations updated. Yell out if you need a hand.

ToBe998 commented 8 years ago

The unittests are working now.

I did not refactor the Controller and Responder though. I don't think this would be a good solution for these reasons:

1) Currently the controller does all the logic needed to parse the URL and create proper commands from it. The new feature introduces aliases which can be resolved to create exactly same commands from it. IMHO still logic associated with parsing the url and creating a command. Changing this would need refactoring for not only the new feature but also the old ones, which is out of scope and imho not in line with separation of concerns.

2) The change would need to introduce dependencies to configuration parsing and restriction evaluation to the responder object. The current responder object does not have any such dependencies and is only concerned with calling the thumbnail creator and returning the output. It does not check if a route was allowed or not (and should not imho)

I know this might be opinion based, but let's face it. We are talking about a very small addition to the code. The feature itself was 20 minutes to implement and add's about 5 lines. Is that feature really worth reevaluating code mechanisms for days that are already in place and refactoring them? If you think this is necessary, please go ahead.