[API][Administrator] Implement managing avatar features #11255
Conversation
GSadee
commented
Mar 19, 2020
GSadee
commented
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Related tickets | part of #11250 |
| License | MIT |
| */ | ||
| public function iUploadTheImageAsMyAvatar(string $avatar, AdminUserInterface $administrator): void | ||
| { | ||
| $response = $this->client->upload( |
There was a problem hiding this comment.
Shouldn't we also use avatarImageClient for the avatar uploading?
There was a problem hiding this comment.
To be honest, there is no difference as I pass entire url here. Do you suggest to refactor upload method?
There was a problem hiding this comment.
It's an option, but for me, the main problem is confusion, that we upload an avatar with $client, but remove it with the avatarImageClient 🐎
There was a problem hiding this comment.
I would vote for doing it the same way with the rest of our resources. Right now upload function is behaving differently than other client methods, so +1 for refactor
| $response = $this->client->upload( | ||
| '/new-api/avatar-images', | ||
| ['owner' => $this->iriConverter->getIriFromItem($administrator)], | ||
| ['file' => new UploadedFile($this->minkParameters['files_path'] . $avatar, basename($avatar))] |
There was a problem hiding this comment.
I'm wondering, is it a proper approach 🤔 It's not resolving the issue like this one when one wants to just use the API (not call the endpoint in the code). Maybe we should just pass the URL to the image which is temporarily somewhere else? I don' know, just thinking out loud 😄
There was a problem hiding this comment.
Probably I don't understand you in 100%, the case is to upload an image, not to pass a URL to the image... I have no other idea how to approach this.
There was a problem hiding this comment.
If I understood correctly, there is an ongoing problem in Sylius documentation on how to handle files upload through API since the Admin API. Problem is that what you have used here (and is used in the whole Sylius codebase) works because we are operating inside of a project. One could not simply use UploadedFile in js.
| $response = $this->client->upload( | ||
| '/new-api/avatar-images', | ||
| ['owner' => $this->iriConverter->getIriFromItem($administrator)], | ||
| ['file' => new UploadedFile($this->minkParameters['files_path'] . $avatar, basename($avatar))] |
There was a problem hiding this comment.
If I understood correctly, there is an ongoing problem in Sylius documentation on how to handle files upload through API since the Admin API. Problem is that what you have used here (and is used in the whole Sylius codebase) works because we are operating inside of a project. One could not simply use UploadedFile in js.
|
|
||
| $oldImage = $owner->getImage(); | ||
| if ($oldImage !== null) { | ||
| $this->avatarImageRepository->remove($oldImage); |
There was a problem hiding this comment.
I'm wondering if it will remove a file from disk as well
There was a problem hiding this comment.
Yup, to be 100% sure, I've checked that and ImagesRemoveListener will remove the file
| */ | ||
| public function iUploadTheImageAsMyAvatar(string $avatar, AdminUserInterface $administrator): void | ||
| { | ||
| $response = $this->client->upload( |
There was a problem hiding this comment.
I would vote for doing it the same way with the rest of our resources. Right now upload function is behaving differently than other client methods, so +1 for refactor
|
Thank you, Grzegorz! 🎉 |
…ient + minor fixes (GSadee) This PR was merged into the 1.8-dev branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | after #11255 | License | MIT Commits ------- 27405f5 [API][Administrator] Refactor upload action in api client + minor fixes
