Make the response cacheable

Created on 28 November 2023, about 1 year ago

Problem/Motivation

The file download response from the media route is currently not cached.

This impacts two parts:

  • The response is not cached in Drupal.
  • No cache tags and cache info are sent with the response, so it won't be cached in Varnish. Or worse, if it is cached due to the Varnish config, then it won't be properly purged.

Steps to reproduce

Without Varnish:
Create media, upload pdf.
Visit the media as anonymous.
Put a debug break point or something to detect a controller hit.
Refresh the page.

Expected: Controller is not called on repeated request, cached response is served.
Actual: Controller is called again on repeated request.

With Varnish:
Configure Varnish to cache all anonymous requests with TTL e.g. 8h.
(you can argue whether this is a good or bad idea)
Use a purge module like dropsolid_purge. (the one we use does not purge urls, maybe this is the problem)
Create media, upload pdf.
Visit the media as anonymous.
Update the media, upload a different pdf.
Visit again the media as anonymous.

Expected: Anonymous sees the updated pdf.
Actual: Anonymous sees the old pdf, because it was not purged.

Proposed resolution

Cache the response.
Set cache metadata in the response, which can be picked up by purge module to add the respective headers.

Remaining tasks

User interface changes

API changes

Data model changes

πŸ“Œ Task
Status

Active

Version

2.0

Component

Miscellaneous

Created by

πŸ‡©πŸ‡ͺGermany donquixote

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @donquixote
  • πŸ‡©πŸ‡ͺGermany donquixote

    I am not sure if the response contains enough cache data.
    Maybe we need to add cache contexts?

  • πŸ‡©πŸ‡ͺGermany donquixote

    The phpcs is wrong: The override of __construct() is not useless, because it adds the protected property $uri.

  • πŸ‡©πŸ‡ͺGermany donquixote
        if (!$response->headers->has('Content-Type')) {
          $response->headers->set('Content-Type', $file->getMimeType() ?: 'application/octet-stream');
        }
    

    This part is weird.
    If we created the response object explicitly in this method, then the check should always return the same result.
    Or not?

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    So do you think we should remove that snippet?

  • πŸ‡©πŸ‡ͺGermany donquixote

    So do you think we should remove that snippet?

    Actually I am no longer so sure it is pointless.
    The constructor of BinaryFileResponse has the uri as a parameter.
    Based on this, it could dynamically set specific headers or not.
    Maybe it does set the 'Content-Type' header? I don't see it though.

    But you added this code, so you probably had a reason, no?

  • πŸ‡©πŸ‡ͺGermany donquixote

    Either way it would be out of scope for this issue.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Wonder if this is ready for review?

  • πŸ‡ΊπŸ‡ΈUnited States amanire

    This issue is a blocker for me. I would also like to know if this is ready for testing.

  • Status changed to Needs review 7 months ago
  • πŸ‡©πŸ‡ͺGermany donquixote

    Wonder if this is ready for review?

    I don't remember all the details, but at least setting it to review increases the chance of progress :)

  • Status changed to Fixed 6 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    So warning caching is my worst.

    But applied the change locally and functionality is still there.

    Tested on a work site that's using redis and also still working.

    Since 2.1.x is still in a beta I'm going to go ahead and include it.

  • Automatically closed - issue fixed for 2 weeks with no activity.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    So wonder if

    $response->getCacheableMetadata()->addCacheContexts([
    'url.query_args:edit-media',
    'user.permissions',
    'url.query_args:dl',
    'url.query_args:download',
    ]);

    Is needed, it's causing failures in 11.1.0 now

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Drupal\Tests\media_alias_display\Functional\MediaAliasDisplayControllerTest::testDisplayController
    with data set "private media without alias" (null, true)
    Exception: User warning: Trying to overwrite a cache redirect with one that
    has nothing in common, old one at address "user.permissions,
    url.query_args:dl, url.query_args:download, route, request_format" was
    pointing to "user.roles:authenticated, languages:language_interface, theme,
    url.site, timezone, url.query_args:_wrapper_format", new one points to
    "url.query_args:edit-media".

Production build 0.71.5 2024