fix: openproject avatar remain stale due to long browser cache#1058
Open
Ashim-Stha wants to merge 1 commit into
Open
fix: openproject avatar remain stale due to long browser cache#1058Ashim-Stha wants to merge 1 commit into
Ashim-Stha wants to merge 1 commit into
Conversation
Ashim-Stha
force-pushed
the
fix-avatar-cache
branch
2 times, most recently
from
e075b26 to
0d713f7
Compare
saw-jan
reviewed
Jun 16, 2026
Ashim-Stha
force-pushed
the
fix-avatar-cache
branch
4 times, most recently
from
d22098b to
32236ec
Compare
saw-jan
reviewed
Jun 16, 2026
Ashim-Stha
force-pushed
the
fix-avatar-cache
branch
10 times, most recently
from
b3af3f3 to
db32125
Compare
saw-jan
reviewed
Jun 16, 2026
Ashim-Stha
force-pushed
the
fix-avatar-cache
branch
3 times, most recently
from
299ef37 to
bbda2c6
Compare
Signed-off-by: Ashim Shrestha <ashimshrestha2384@gmail.com>
Ashim-Stha
force-pushed
the
fix-avatar-cache
branch
from
bbda2c6 to
f1c5c09
Compare
Ashim-Stha
requested review from
Kharonus,
NobodysNightmare,
ba1ash,
dominic-braeunlein,
lindenthal,
nabim777,
psatyal and
wielinde
as code owners
PHP Code CoverageCoverage after merging fix-avatar-cache into release/3.1 will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
nabim777
reviewed
Jun 18, 2026
| 'Cache-Control' => 'no-cache', | ||
| 'ETag' => $etag, | ||
| ]; | ||
| $ifNoneMatch = $this->request->getHeader('If-None-Match'); |
Collaborator
There was a problem hiding this comment.
Suggested change
| $ifNoneMatch = $this->request->getHeader('If-None-Match'); | |
| $ifNoneMatch = $this->request->getHeader('If-None-Match') ?? ''; |
Collaborator
There was a problem hiding this comment.
request->getHeader will return empty string if not found
| ); | ||
| public function getOpenProjectAvatarDataProvider(): array { | ||
| return [ | ||
| 'oauth - 200 OK' => [ |
Collaborator
There was a problem hiding this comment.
Suggested change
| 'oauth - 200 OK' => [ | |
| 'OAuth: returns 200 OK when If-None-Match differs from ETag' => [ |
| 'contentType' => 'image/jpeg', | ||
| 'expectedStatusCode' => Http::STATUS_OK, | ||
| ], | ||
| 'oauth - 304 Not Modified' => [ |
Collaborator
There was a problem hiding this comment.
Suggested change
| 'oauth - 304 Not Modified' => [ | |
| 'OAuth: returns 304 Not Modified when If-None-Match matches ETag' => [ |
| 'ifNoneMatch' => '"some etag"', | ||
| 'contentType' => 'image/jpeg', | ||
| 'expectedStatusCode' => Http::STATUS_NOT_MODIFIED, | ||
| ] |
Collaborator
There was a problem hiding this comment.
I think it's good to check ifNoneMatch if it's empty.
nabim777
reviewed
Jun 18, 2026
Comment on lines
+402
to
+407
| $avatarFile = $avatar->getFile(64); | ||
| return [ | ||
| 'avatar' => $avatarFile->getContent(), | ||
| 'type' => $avatarFile->getMimeType(), | ||
| 'etag' => md5($avatarFile->getContent()), | ||
| ]; |
Collaborator
There was a problem hiding this comment.
Suggested change
| $avatarFile = $avatar->getFile(64); | |
| return [ | |
| 'avatar' => $avatarFile->getContent(), | |
| 'type' => $avatarFile->getMimeType(), | |
| 'etag' => md5($avatarFile->getContent()), | |
| ]; | |
| $avatarFile = $avatar->getFile(64); | |
| $avatarContent = $avatarFile->getContent(); | |
| return [ | |
| 'avatar' => $avatarContent, | |
| 'type' => $avatarFile->getMimeType(), | |
| 'etag' => md5($avatarContent), | |
| ]; |
nabim777
reviewed
Jun 18, 2026
| $etag = '"' . $etag . '"'; | ||
| $this->assertSame( | ||
| "no-cache", | ||
| $response->getHeaders()["Cache-Control"] |
Collaborator
There was a problem hiding this comment.
getHeaders() is repeated. You can make simply
$headers = $response->getHeaders()and use this in multiple places.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
max-age=86400withCache-Control: no-cacheso the browser always revalidates the avatar on each requestETagsupport based onmd5of theavatar contentso the browser can skip downloading the body if the avatar hasn't changed (304 Not Modified)References: see ref1, ref2
Related Issue or Workpackage
Screenshots (if appropriate):
Types of changes
Checklist:
CHANGELOG.mdfile