Add write-only image_source field for asset create/edit API endpoint
#6146
Conversation
| */ | ||
| public function image(Request $request, $asset_id) | ||
| { | ||
| $this->authorize('image', Asset::class); |
There was a problem hiding this comment.
We don't have a permission mask for image, do we?
|
Wouldn't this make more sense in the edit method? Do we need a separate endpoint for image uploads? Also, if we were going to do that, it should be files in general, not just images. Image is only relevant in the edit/create process, everything else is a file upload, which could be images or other types of files. |
mskrip
changed the title
image_source field for asset create/edit API endpoint
|
Moved it to edit/create method as you suggested. But didn't want to create inconsistencies so I made write-only field |
| @@ -506,6 +520,13 @@ public function update(Request $request, $id) | |||
| ($request->filled('company_id')) ? | |||
| $asset->company_id = Company::getIdForCurrentUser($request->get('company_id')) : ''; | |||
|
|
|||
| if ($request->has('image_source')) { | |||
| try { | |||
There was a problem hiding this comment.
Hi there - what's the reason for a try/catch here?
There was a problem hiding this comment.
Hi, added a comment on that. Basically the called function can throw an error if source is not in supported format or somehow malformed.
There was a problem hiding this comment.
Hi - What's the error it can throw? It looks like you're just setting the $asset->image value, no?
$asset->image = $this->image($request->input('image_source'));
There was a problem hiding this comment.
$this->image() can throw the exception
There was a problem hiding this comment.
Oh, sorry... I had no idea it works like that. As you might imagine I'm new to php. Removed both try/catches.
| * @return string path to uploaded image | ||
| * @throws \Exception if image source is not in correct format | ||
| */ | ||
| private function image(String $image_data) |
There was a problem hiding this comment.
Sorry for the delay in finishing this review. We should also probably rename this method, since it actually performs an action, and return false if something goes wrong. Something like processUploadedImage, or something that's a bit more specific as to what it's doing. In this massive codebase, I'm not sure I'll ever remember that ->image() actually processes things, and could be easily confused with the image property of many of our models.
There was a problem hiding this comment.
Also should this maybe be a helper, trait or model method? Lots of our models have images. If there's a way to break it out so that it could be re-used, that would probably be more useful.
There was a problem hiding this comment.
Hi, I've moved the method to helpers and renamed it according to your suggestion.
There was a problem hiding this comment.
The method now returns false if something goes wrong. So it's much more general while the endpoint still retains the possibility to wipe the image from hardware if image_source field is set but empty.
|
Hi @snipe, should I do something with the failing check? Or is it out of my control completely? |
|
ping |
|
@snipe Ping again, any update on this? It would be very useful |
`image_source` should contain base64 encoded image data with mime-type.
Tested with
Content-Type: application/x-www-form-urlencodedand fileattached as base64 encoded string after mime type in field
image_source.