Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RTM] Deferred image resizing #354

Open
wants to merge 44 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ausi
Copy link
Member

commented Feb 19, 2019

As discussed in contao/core-bundle#994

@ausi ausi force-pushed the ausi:feature/deferred-image-resizing branch from 2c47264 to 784a33b Feb 19, 2019

@leofeyer leofeyer added this to the 4.8.0 milestone Feb 19, 2019

@leofeyer

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

Depends on contao/image#50.

@ausi ausi force-pushed the ausi:feature/deferred-image-resizing branch from 2cb51ba to a891b50 Feb 19, 2019

@ausi ausi force-pushed the ausi:feature/deferred-image-resizing branch 2 times, most recently from 8f49ef9 to a71339e Mar 15, 2019

@ausi ausi marked this pull request as ready for review Mar 18, 2019

@ausi ausi changed the title Deferred image resizing [RFC] Deferred image resizing Mar 18, 2019

Show resolved Hide resolved core-bundle/src/Command/DeferredImagesCommand.php Outdated
Show resolved Hide resolved core-bundle/src/Command/DeferredImagesCommand.php Outdated
Show resolved Hide resolved core-bundle/src/Command/DeferredImagesCommand.php Outdated
Show resolved Hide resolved core-bundle/src/Routing/ImagesLoader.php Outdated

@ausi ausi force-pushed the ausi:feature/deferred-image-resizing branch from 802c3e1 to 480f709 Mar 21, 2019

@Toflar
Copy link
Member

left a comment

Just a minor nitpick. Apart from that and the missing unit tests for the command this PR is in absolutely excellent shape! Outstanding work @ausi 👏

Show resolved Hide resolved core-bundle/src/Command/ResizeImagesCommand.php Outdated

@leofeyer leofeyer force-pushed the contao:master branch from 804d2ed to c0fc631 Mar 27, 2019

@leofeyer leofeyer force-pushed the contao:master branch 2 times, most recently from 6c52109 to 03f6899 Jun 5, 2019

@leofeyer leofeyer force-pushed the contao:master branch from 67bdc5c to d42ccf4 Jun 13, 2019

throw new NotFoundHttpException($exception->getMessage(), $exception);
}
return new BinaryFileResponse($image->getPath());

This comment has been minimized.

Copy link
@Toflar

Toflar Jun 13, 2019

Member

I think we should add caching headers here ;) We should cache it for a year. I also thought about adding an ETag so that after this year we can save bandwidth if the image still hasn't changed. However, I think that there's no real benefit. Calculating the hash for every image is too expensive, I prefer a re-download after a year. Also, the BinaryFileResponse is public by default which would then fill our reverse proxy. I don't think that makes a lot of sense because the images are created and thus cached anyway. So imho this should look like this:

return new BinaryFileResponse($image->getPath(), 200, ['Cache-Control' => 'private, max-age=31536000'], false);

This comment has been minimized.

Copy link
@ausi

ausi Jun 16, 2019

Author Member

I’m not sure about the private part. If someone doesn’t use our reverse proxy, public caching might be desired (e.g. on a CDN).

How about configuring our reverse proxy to never cache image/* responses?

This comment has been minimized.

Copy link
@ausi

ausi Jun 16, 2019

Author Member

Setting it to private would also differ from the image responses that are handled by the web server directly:

Header set Cache-Control "max-age=31536000" env=CONTAO_ASSETS

This comment has been minimized.

Copy link
@ausi

ausi Jun 16, 2019

Author Member

Cache headers added in 12761e4

This comment has been minimized.

Copy link
@Toflar

Toflar Jun 17, 2019

Member

Hmm, I don't think it would be different than our existing .htaccess. We don't add any public directive so I think it's treated as being private. But in any case, I agree we need to find the best solution here. So we have the following 2 use cases:

  1. No special proxy setup = Symfony HttpCache. If we make the response public, it would - as of today - duplicate the images on the server. I think we can agree that there's no point in doing so. Now either we configure our HttpCache instance to ignore Content-Type: image/* or /assets/images responses (I'd prefer the path based one because it's more specific to only our thumbnails) or we make sure we do not send the public cache-control directive. The latter would be easier I think (we could add a configuration like contao.image.controller_cache_control: max-age=1234, private with the default value for the Symfony proxy). Having HttpCache ignoring certain responses will be tricky because we'd have to override the fetch() method because ignoring certain responses is not part of the design (yet): https://github.com/symfony/symfony/blob/4.3/src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php#L452

  2. Special proxy setup = e.g. Varnish: It might make sense to also store the generated images in Varnish or some CDN for performance reasons. However, we now also have some maintenance task which is called "clear image and scripts assets". Executing this would have no effect as it would clear files on the server but not in the proxy. If we wanted to support that, consequently we should add cache tags to those files (which would now work for the thumbnails but not for css and js files...). So that needs some special knowledge anyway. And for Varnish I could easily force the /assets/images requests to be public if I wanted to store them.

So not sure here really. I think it's better to make them private by default because that's what we need for our HttpCache solution aka the 99% use case. For everybody else they can maybe either force the caching by using proxy configuration (possible in Varnish and probably also in some CDNs I suppose) or we may introduce a configuration parameter but I'm not sure if this is worth it?

This comment has been minimized.

Copy link
@ausi

ausi Jun 17, 2019

Author Member

I agree. Private by default makes sense then.

we may introduce a configuration parameter but I'm not sure if this is worth it?

If a project uses a different proxy they could use a listener to modify the cache headers very easily I think. So a configuration parameter is not needed IMO.

This comment has been minimized.

Copy link
@ausi

ausi Jun 17, 2019

Author Member

Done in ecfaff2.

This comment has been minimized.

Copy link
@Toflar

Toflar Jun 17, 2019

Member

Yeah, we may just provide that configuration because it's easy to do and then we make it a "supported use case". Also, it would allow to adjust the caching duration to a month or so in case somebody needs that. Imho it would just be a fairly simple thing to introduce and potentially make somebody happy in the future :)

This comment has been minimized.

Copy link
@ausi

This comment has been minimized.

Copy link
@leofeyer

leofeyer Jun 18, 2019

Member

I don't know if this really has to be configurable.

@ausi ausi force-pushed the ausi:feature/deferred-image-resizing branch from 433526d to 12761e4 Jun 16, 2019

@ausi

This comment has been minimized.

Copy link
Member Author

commented Jun 16, 2019

The code coverage decrease is because of the untested ResizeImagesCommand::executeConcurrent().
I’m not sure how to test this method and if it’s worth the effort?

@ausi ausi changed the title [RFC] Deferred image resizing Deferred image resizing Jun 16, 2019

@ausi

This comment has been minimized.

Copy link
Member Author

commented Jun 16, 2019

Apart from the open question about the private cache in #354 (comment) this PR should be ready to merge now I think.

Show resolved Hide resolved composer.json
Show resolved Hide resolved core-bundle/src/Command/ResizeImagesCommand.php Outdated
Show resolved Hide resolved core-bundle/src/Command/ResizeImagesCommand.php
Show resolved Hide resolved core-bundle/src/Command/ResizeImagesCommand.php Outdated
Show resolved Hide resolved core-bundle/src/Controller/ImagesController.php
Show resolved Hide resolved core-bundle/src/Resources/contao/classes/DataContainer.php
Show resolved Hide resolved core-bundle/src/Resources/contao/dca/tl_files.php Outdated
Show resolved Hide resolved core-bundle/src/Resources/contao/dca/tl_files.php
Show resolved Hide resolved core-bundle/src/Resources/contao/library/Contao/Image.php Outdated
Show resolved Hide resolved core-bundle/src/Routing/ImagesLoader.php

ausi added some commits Jun 17, 2019

@ausi ausi requested a review from aschempp Jun 17, 2019

ausi added some commits Jun 18, 2019

@ausi ausi changed the title Deferred image resizing [RTM] Deferred image resizing Jun 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.