-
Notifications
You must be signed in to change notification settings - Fork 920
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
[4.1][Improvement] Image field - allow multiple image uploaders + generate variations on save #124
Conversation
Have refactored a small amount of code to allow multiple uploaders on the page and enabled better user feedback with loaders and hiding of buttons
This is an offshoot of a previous PR which I’ve closed. This uses the new image uploader frontend, however allows handling in a similar fashion as `uploadFileToDisk` by using `uploadImageToDisk` ```php $this->crud->addField([ 'name' => 'image', 'label' => 'Company Photo', 'type' => 'image', 'crop' => true, 'ratio' => '1:2', 'disk' => 'uploads' ],'both'); ``` ```php public function setImageAttribute($value) { $attribute_name = "image"; $disk = "uploads"; $destination_path = "folder_1/subfolder_3"; $this->uploadImageToDisk($value, $attribute_name, $disk, $destination_path, ['medium' => [500,500]]); return $this->attribute[$attribute_name]; } ```
[ci skip] [skip ci]
[ci skip] [skip ci]
Just noticed an odd permissions issue with Intervention, so looking into it before marking it as ready again |
[ci skip] [skip ci]
@tabacitu just an update, there was a previous Issue added that somebody was asking for a way to fix the issue this PR solves, maybe when you get a minute we could look at getting it integrated before too much other stuff changes :P |
# Conflicts: # src/resources/views/fields/image.blade.php
[ci skip] [skip ci]
@tabacitu I've made the discussed changes and tweaked a few bits, now blockui is also included should be easier for you to test? an example usage would be $this->crud->addField([
'name' => 'photo',
'label' => 'Profile Photo',
'type' => 'image',
'crop' => true,
'aspect_ratio' => '1:1',
'disk' => 'uploads'
]); public function setPhotoAttribute($value)
{
return $value = $this->uploadImageToDisk($value, $attributeName, $disk, $uploadFolder, $variations = [
'small' => [100,100],
'medium' => [500, 500]
]);
} |
@tabacitu - this PR still solves several bugs with the image field, I know it's old now and maybe seem like hard one to merge, but I think it will definitely be worth it! P.s. Currently you can only have 1 image uploaded on the form, this also fixes that amongst other fixes |
public function uploadImageToDisk($value, $attribute_name, $disk, $destination_path, $variations = null) | ||
{ | ||
if (! $variations || ! is_array($variations)) { | ||
$variations = ['original' => null, 'thumb' => [150, 150]]; |
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.
That this defaults to 150x150 probably should be stated somewhere.
(less important: where did `150x150 come from?)
$variations = ['original' => null, 'thumb' => [150, 150]]; | ||
} | ||
|
||
//Needed for the original image |
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.
That comment's confusing - it's needed but it's null? Does that mean the value should be set to the null value maybe?
|
||
//Needed for admin thumbnails | ||
if (! array_key_exists('thumb', $variations)) { | ||
$variations['thumb'] = [150, 150]; |
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.
150x150?
$disk_root = $disk_config['root']; | ||
|
||
//if the disk is public, we need to know the public path | ||
if ($disk_config['visibility'] == 'public') { |
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.
What if there is no 'visibility' key (out of morbid interest)?
Is this better as
if (config("filesystems.disks.$disk.visbility") === 'public')) { /* stuff */ }
...maybe?
if (($request->hasFile($attribute_name) || starts_with($value, 'data:image')) && $this->{$attribute_name}) { | ||
foreach ($variations as $variant => $dimensions) { | ||
$variant_name = str_replace('-original', '-'.$variant, $this->{$attribute_name}); | ||
\Storage::disk($disk)->delete($variant_name); |
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.
Just out of morbid interest, can that ever fail? (the answer is yes, but what does it do when it does?)
I'm more smoking out "errors that never get logged/cause exceptions".
} | ||
|
||
/** | ||
* Handles the retrieval of an image by variant:. |
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.
Period at the end?
/** | ||
* Handles the retrieval of an image by variant:. | ||
* | ||
* @param [type] $attribute Name of the attribute within the model that contains the json |
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.
We end in periods (.) or we don't but they're mixed for some reason.
* | ||
* @param [type] $attribute Name of the attribute within the model that contains the json | ||
* @param [type] $variant Name of the variant you want to extract | ||
* @param [type] $disk Filesystem disk used to store files. |
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.
We do return...but what?
$disk_config = config('filesystems.disks.'.$disk); | ||
$disk_root = $disk_config['root']; | ||
|
||
//if the disk is public, we need to know the public path |
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.
That's surely a DRY (it must be up there too!).
} | ||
} | ||
|
||
return $url; |
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.
This can be 'null' but can Storage::url($foo)
ever also be null? I don't think it can in which case it's ok but if there is a difference between:
- $image was not set for some reason
Storage::url()
returned something funny
...we probably should allow the end user to detect that somehow. Or throw an exception.
@OwenMelbz & @lloy0076 - I really appreciate your work on this. But... since I've let 4 years pass, this PR is no longer in a state where it can be merged. So I'm going to have to close it. Thanks again for all the help you guys have provided - I really appreciate it. But it's time to put this PR to sleep 😄 Cheers! |
I wonder if this is possible to do now since all the updates!? 😂 |
😆 A PR from 2016 merged in 2020? I'm pretty sure if we try... we'll create a black hole. Or at least a nuclear explosion... |
Have refactored a small amount of code to allow multiple uploaders on
the page and enabled better user feedback with loaders and hiding of
buttons