Skip to content
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

Closed
wants to merge 14 commits into from
211 changes: 211 additions & 0 deletions src/CrudTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -191,4 +191,215 @@ public function uploadMultipleFilesToDisk($value, $attribute_name, $disk, $desti

$this->attributes[$attribute_name] = json_encode($attribute_value);
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a general note this function strikes me as being too long; personally I'd split it into a few protected (or public?) methods with easy to understand names...

* Handle image upload and DB storage for a image:
* - on CREATE
* - stores the image at the destination path
* - generates a name
* - creates image variations
* - stores json object into database with variations and paths
* - on UPDATE
* - if the value is null, deletes the file and sets null in the DB
* - if the value is different, stores the different file and updates DB value.
*
* @param [type] $value Value for that column sent from the input.
* @param [type] $attribute_name Model attribute name (and column in the db).
* @param [type] $disk Filesystem disk used to store files.
* @param [type] $destination_path Path in disk where to store the files.
* @param [type] $variations Array of variations and their dimensions
*/
public function uploadImageToDisk($value, $attribute_name, $disk, $destination_path, $variations = null)
{
if (! $variations || ! is_array($variations)) {
$variations = ['original' => null, 'thumb' => [150, 150]];
Copy link
Contributor

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?)

}

//Needed for the original image
Copy link
Contributor

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?

if (! array_key_exists('original', $variations)) {
$variations['original'] = null;
}

//Needed for admin thumbnails
if (! array_key_exists('thumb', $variations)) {
$variations['thumb'] = [150, 150];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

150x150?

}

$request = \Request::instance();

//We need to setup the disk paths as they're handled differently
//depending if you need a public path or internal storage
$disk_config = config('filesystems.disks.'.$disk);
$disk_root = $disk_config['root'];

//if the disk is public, we need to know the public path
if ($disk_config['visibility'] == 'public') {
Copy link
Contributor

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?

$public_path = str_replace(public_path(), '', $disk_root);
} else {
$public_path = $disk_root;
}

// if a new file is uploaded, delete the file from the disk
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);
Copy link
Contributor

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".

}
$this->attributes[$attribute_name] = null;
}

// if the file input is empty, delete the file from the disk
if (empty($value)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why wouldn't this be caught by the above? And if it's not, what's the value of $this->{$attribute} which unless I'm going crazy would be either not set or null (i.e. fail a *&& test).

foreach ($variations as $variant => $dimensions) {
$variant_name = str_replace('-original', '-'.$variant, $this->{$attribute_name});
\Storage::disk($disk)->delete($variant_name);
}

return $this->attributes[$attribute_name] = null;
}

// if a new file is uploaded, store it on disk and its filename in the database
if ($request->hasFile($attribute_name) && $request->file($attribute_name)->isValid()) {

// 1. Generate a new file name
$file = $request->file($attribute_name);
$new_file_name = md5($file->getClientOriginalName().time());
$new_file = $new_file_name.'.'.$file->getClientOriginalExtension();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll be documented somewhere (in the README.io place) how the thing is named won't it?


// 2. Move the new file to the correct path
$file_path = $file->storeAs($destination_path, $new_file, $disk);
$image_variations = [];

// 3. but only if they have the ability to crop/handle images
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that "but" in the comment implies that step 2. shouldn't have occurred. But it did occur...

  1. The comment/s is/are confusing
  2. The comment/s is/are wrong
  3. The code is wrong

...but I'm not sure which of the above :(

if (class_exists('\Intervention\Image\ImageManagerStatic')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if this class doesn't exist, the variations don't get made; that's ok in a sense.

BUT if that's the case and someone expects the variations TO BE MADE and they're not we land in the world of confusion.

Is it even possible for this condition to sensibly return a falsey value?

foreach ($variations as $variant => $dimensions) {
$img = \Intervention\Image\ImageManagerStatic::make($file);

$variant_name = $new_file_name.'-'.$variant.'.'.$file->getClientOriginalExtension();
$variant_file = $destination_path.'/'.$variant_name;

if ($dimensions) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works but isset($dimensions) is probably more correct? Or even a check to see what the dimensions are - couldn't someone include a truthy dimensions that are just unexpected?

$width = $dimensions[0];
$height = $dimensions[1];

if ($img->width() > $width || $img->height() > $height) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if they're less?

$img->resize($width, $height, function ($constraint) {
$constraint->aspectRatio();
})
->save($disk_root.'/'.$variant_file);
} else {
$img->save($disk_root.'/'.$variant_file);
}

$image_variations[$variant] = $public_path.'/'.$variant_file;
} else {
$image_variations['original'] = $public_path.'/'.$file_path;
}
}
} else {
$image_variations['original'] = $public_path.'/'.$file_path;
$image_variations['thumb'] = $public_path.'/'.$file_path;
}

// 3. Save the complete path to the database
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1, 2, , 3, 3 👿

No, this is number 4!

$this->attributes[$attribute_name] = $image_variations['original'];
} elseif (starts_with($value, 'data:image')) {
$new_file_name = md5($value.time());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

microtime(true)? For the paranoid?


if (! \Illuminate\Support\Facades\File::exists($disk_root.'/'.trim($destination_path, '/'))) {
\Illuminate\Support\Facades\File::makeDirectory($disk_root.'/'.trim($destination_path, '/'), 0775, true);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Else?


foreach ($variations as $variant => $dimensions) {
$img = \Intervention\Image\ImageManagerStatic::make($value);

switch ($img->mime()) {
case 'image/bmp':
case 'image/ief':
case 'image/jpeg':
case 'image/pipeg':
case 'image/tiff':
case 'image/x-jps':
$extension = '.jpg';
break;
case 'image/gif':
$extension = '.gif';
break;
case 'image/x-icon':
case 'image/png':
$extension = '.png';
break;
default:
$extension = '.jpg';
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just convert them all to jpg/png/gif/pick your poison?


$variant_name = $new_file_name.'-'.$variant.$extension;
$variant_file = $destination_path.'/'.$variant_name;

if ($dimensions) {
$width = $dimensions[0];
$height = $dimensions[1];

if ($img->width() > $width || $img->height() > $height) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure I saw this same clause up there (see up there). Plus it's a possible candidate for DRY.

$img->resize($width, $height, function ($constraint) {
$constraint->aspectRatio();
})
->save($disk_root.'/'.$variant_file);
} else {
$img->save($disk_root.'/'.$variant_file);
}

$image_variations[$variant] = $public_path.'/'.$variant_file;
} else {
$img->save($disk_root.'/'.$variant_file);
$image_variations['original'] = $variant_file;
}
}

return $this->attributes[$attribute_name] = $image_variations['original'];
}
}

/**
* Handles the retrieval of an image by variant:.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Period at the end?

*
* @param [type] $attribute Name of the attribute within the model that contains the json
Copy link
Contributor

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] $variant Name of the variant you want to extract
* @param [type] $disk Filesystem disk used to store files.
Copy link
Contributor

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?

*/
public function getUploadedImageFromDisk($attribute, $variant = 'original', $disk = null)
{
$image = $this->attributes[$attribute];
$url = null;
if (! empty($image)) {
$image_variant = str_replace('-original', '-'.$variant, $image);

if ($disk) {
$disk_config = config('filesystems.disks.'.$disk);
$disk_root = $disk_config['root'];

//if the disk is public, we need to know the public path
Copy link
Contributor

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!).

if ($disk_config['visibility'] == 'public') {
$public_path = str_replace(public_path(), '', $disk_root);
} else {
$public_path = $disk_root;
}

if (\Storage::disk($disk)->exists($image_variant)) {
$url = asset($public_path.'/'.$image_variant);
} else {
$url = asset($public_path.'/'.trim($image, '/'));
}
} else {
if (\Storage::exists($image_variant)) {
$url = \Storage::url(trim($image_variant, '/'));
} else {
$url = url($image);
}
}
}

return $url;
Copy link
Contributor

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:

  1. $image was not set for some reason
  2. Storage::url() returned something funny

...we probably should allow the end user to detect that somehow. Or throw an exception.

}
}
82 changes: 82 additions & 0 deletions src/public/owenmelbz/blockui.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/*csslint box-sizing: false*/

@-webkit-keyframes block-ui-spin {
0% {
-webkit-transform: rotate(0deg);
transform: rotate(0deg);
}
100% {
-webkit-transform: rotate(360deg);
transform: rotate(360deg);
}
}

@-moz-keyframes block-ui-spin {
0% {
-moz-transform: rotate(0deg);
transform: rotate(0deg);
}
100% {
-moz-transform: rotate(360deg);
transform: rotate(360deg);
}
}

@-o-keyframes block-ui-spin {
0% {
-o-transform: rotate(0deg);
transform: rotate(0deg);
}
100% {
-o-transform: rotate(360deg);
transform: rotate(360deg);
}
}

@keyframes block-ui-spin {
0% {
transform: rotate(0deg);
}
100% {
transform: rotate(360deg);
}
}

.block-ui-target {
position: relative;
pointer-events: none;
}

.block-ui-block::after,
.block-ui-block::before {
display: block;
content: "";
width: 100%;
height: 100%;
position: absolute;
top: 0;
left: 0;
pointer-events: none;
}

.block-ui-block::before {
z-index: 2000;
top: 50%;
left: 50%;
margin-left: -7px;
margin-top: -7px;
width: 14px;
height: 14px;
box-sizing: border-box;
border: solid 2px #000000;
border-top-color: #605ca8;
border-left-color: #605ca8;
border-radius: 10px;
animation: block-ui-spin 400ms linear infinite;
}

.block-ui-block::after {
background: #FFF;
opacity: .7;
z-index: 1999;
}
18 changes: 18 additions & 0 deletions src/public/owenmelbz/blockui.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*jslint browser: true*/

//Block UI JS
(function (jQuery) {

'use strict';

jQuery.fn.blockui = jQuery.fn.blockUI = function () {
this.addClass('block-ui-target block-ui-block');
return this;
};

jQuery.fn.unblockui = jQuery.fn.unblockUI = function () {
this.removeClass('block-ui-target block-ui-block');
return this;
};

}(window.jQuery));
1 change: 1 addition & 0 deletions src/public/owenmelbz/blockui.min.css

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions src/public/owenmelbz/blockui.min.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions src/resources/views/columns/image.blade.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{{-- regular object attribute --}}
<td>
@if ( isset($entry->{$column['name']}) && !empty($entry->{$column['name']}) )
<img src="{{ $entry->getUploadedImageFromDisk($column['name'], 'thumb', (isset($column['disk']) && $column['disk'] ? $column['disk'] : null)) }}" alt="entry image preview" style="height: 25px;" />
@else
n/a
@endif
</td>
Loading