- 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 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 5:52pm 28 May 2024 - π©πͺ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 :)
-
smustgrave β
committed 13495c22 on 2.1.x
Issue #3404512 by donquixote: Make the response cacheable
-
smustgrave β
committed 13495c22 on 2.1.x
- Status changed to Fixed
6 months ago 6:20pm 17 June 2024 - πΊπΈ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".