-
Notifications
You must be signed in to change notification settings - Fork 89
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
Fix image serving in direct editing #2059
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mh,
$userId
seems to be needed in each public function of the controller. It gets injected into the constructor by DI, but in all functions but this one (getImage()
), it's fetched again using$session->getUiserId()
. So does this mean that$userId
as injected by DI is not reliable? Then probably$this->userId
should be removed from the class altogether?Maybe a function like the following could work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the same in all methods of
ImageController
. Whether the user is authenticated or not, we always choose to rely on the edition session rather than the "classic" authentication. This way it works with the mobile clients which are making unauthenticated requests but our UI passes the edition session ID and token.This was suggested by @juliushaertl in #1900 (comment)
The unused
$userId
attribute has been removed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now 😊 You could still move the logic to get
$userId
into a dedicated private function to lower code-duplication, but that's really just nitpicking 😉