allow caching avatars for 7 days, allow overwriting default headers, remove IE8 hacks #26589
Conversation
|
@butonic, thanks for your PR! By analyzing the history of the files in this pull request, we identified @PVince81, @rullzer and @LukasReschke to be potential reviewers. |
…remove IE8 handling
butonic
force-pushed
the
avatarcontroller-caching-and-cleanup
branch
from
5745101
to
264fcf5
Compare
butonic
changed the title
|
maybe @felixheidecke has opinion ho long to cache the avatar? |
| @@ -120,22 +120,21 @@ public function getAvatar($userId, $size) { | |||
| ['Content-Type' => $avatar->getMimeType()]); | |||
| $resp->setETag($avatar->getEtag()); | |||
| } catch (NotFoundException $e) { | |||
| $user = $this->userManager->get($userId); | |||
| $resp = new DataResponse([ | |||
| $user = $this->userManager->get($userId); | |||
| @@ -134,8 +134,7 @@ public function getAvatar($userId, $size) { | |||
| ]); | |||
| } | |||
|
|
|||
| $resp->addHeader('Pragma', 'public'); | |||
| $resp->cacheFor(0); | |||
| $resp->cacheFor(604800); // allow caching for a week | |||
There was a problem hiding this comment.
Can we go with 1 or 2 hours here? 7 days seems too long for me.
There was a problem hiding this comment.
Consider the use case. How often do I change my Avatar. Every hour? Every day? Everey Week? Probably never again!
As far as I'm concerned a week is just fine.
|
During my local testing the request still hits the server. Is this intended? |
|
@DeepDiver1975 The initial request, yes. Also, don't use F5 but just hit enter in the location bar. @PhilippSchaffrath and I also came up with a patch for disabling js triggered requests to preview.png. PR pending. |
|
@DeepDiver1975 can you make sure that you don't hit F5 during your test, this would re-load the avatar no matter what the header is. (like @butonic said, click in the address-bar and hit return) The patch mentioned by @butonic is not really related to this issue. |
|
How to move forward ? Maybe 1 day is better. My worry is mostly about invalidating. If someone changed their avatar, maybe it's fine if other users still see the old avatar for a day or so. But if the user who changed their avatar still sees the old avatar for a day, they'll think there's a bug and that the change didn't work. |
|
finish or close ? |
|
PR too old, there doesn't seem to be any interest in finishing it. Closing |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description
Cache-Control: max-age=604800header. Will change for the user immedeately after changing his avatar. Can still be refreshed by pressing F5Related Issue
#26585
Motivation and Context
Performance, technical debt
How Has This Been Tested?
Firefox and Edge work correctly, Chromium works as expected via http but not via https. I expect that to be a bug in chromium.
note that you might want to set
session.cache_limiter =to an empty value in your php.ini to prevent PHP from sending deprecatedPragma: no-cacheheaders that may interfere with IE (Firefox seems to ignore them)Screenshots (if appropriate):
Types of changes
Checklist: