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.

Sign up for GitHub

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

Jump to bottom

feat: prepare AWS S3 upload for avatars #1747

Merged
merged 14 commits into from
Sep 2, 2018
Merged

feat: prepare AWS S3 upload for avatars #1747

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ea8d946
Prepare s3 support
asbiin Aug 26, 2018
d27ea90
Apply fixes from StyleCI
asbiin Aug 26, 2018
3be774b
Upload file with public visibility
asbiin Aug 27, 2018
c7af31b
Add new command
asbiin Aug 27, 2018
c13517d
Apply fixes from StyleCI
asbiin Aug 27, 2018
5bbf1d1
Fixes
asbiin Sep 1, 2018
4016759
Merge remote-tracking branch 'origin/master' into feat/prepare-s3
asbiin Sep 1, 2018
1bbeffc
Fix MoveAvatars command
asbiin Sep 1, 2018
4177c65
Apply fixes from StyleCI
asbiin Sep 1, 2018
2a72c69
Fix
asbiin Sep 1, 2018
21257b1
Add --dryrun option
asbiin Sep 1, 2018
a27eca6
Merge branch 'master' into feat/prepare-s3
asbiin Sep 2, 2018
680fd4e
Fix psalm
asbiin Sep 2, 2018
53fdda8
Apply fixes from StyleCI
asbiin Sep 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion app/Http/Controllers/ContactsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use App\Helpers\SearchHelper;
use App\Models\Contact\Contact;
use Illuminate\Support\Facades\Auth;
use Illuminate\Support\Facades\Storage;
use App\Models\Relationship\Relationship;
use Barryvdh\Debugbar\Facade as Debugbar;
use Illuminate\Support\Facades\Validator;
Expand Down Expand Up @@ -275,9 +276,13 @@ public function update(Request $request, Contact $contact)
$contact->nickname = $request->input('nickname', null);

if ($request->file('avatar') != '') {
if ($contact->has_avatar) {
$contact->deleteAvatars();
}

$contact->has_avatar = true;
$contact->avatar_location = config('filesystems.default');
$contact->avatar_file_name = $request->avatar->store('avatars', config('filesystems.default'));
$contact->avatar_file_name = $request->avatar->storePublicly('avatars', $contact->avatar_location);
}

// Is the person deceased?
Expand Down
49 changes: 28 additions & 21 deletions app/Jobs/ResizeAvatars.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,27 +35,34 @@ public function __construct(Contact $contact)
*/
public function handle()
{
if ($this->contact->has_avatar) {
try {
$avatar_file = Storage::disk($this->contact->avatar_location)->get($this->contact->avatar_file_name);
$avatar_path = Storage::disk($this->contact->avatar_location)->url($this->contact->avatar_file_name);
$avatar_filename_without_extension = pathinfo($avatar_path, PATHINFO_FILENAME);
$avatar_extension = pathinfo($avatar_path, PATHINFO_EXTENSION);
} catch (FileNotFoundException $e) {
return;
}

$size = 110;
$avatar_cropped_path = 'avatars/'.$avatar_filename_without_extension.'_'.$size.'.'.$avatar_extension;
$avatar = Image::make($avatar_file);
$avatar->fit($size);
Storage::disk($this->contact->avatar_location)->put($avatar_cropped_path, $avatar->stream()->__toString());

$size = 174;
$avatar_cropped_path = 'avatars/'.$avatar_filename_without_extension.'_'.$size.'.'.$avatar_extension;
$avatar = Image::make($avatar_file);
$avatar->fit($size);
Storage::disk($this->contact->avatar_location)->put($avatar_cropped_path, $avatar->stream()->__toString());
if (! $this->contact->has_avatar) {
return;
}

$storage = Storage::disk($this->contact->avatar_location);
if (! $storage->exists($this->contact->avatar_file_name)) {
return;
}

try {
$avatar_file = $storage->get($this->contact->avatar_file_name);
$filename = pathinfo($this->contact->avatar_file_name, PATHINFO_FILENAME);
$extension = pathinfo($this->contact->avatar_file_name, PATHINFO_EXTENSION);
} catch (FileNotFoundException $e) {
return;
}

$this->resize($avatar_file, $filename, $extension, $storage, 110);
$this->resize($avatar_file, $filename, $extension, $storage, 174);
}

private function resize($avatar_file, $filename, $extension, $storage, $size)
{
$avatar_file_name = 'avatars/'.$filename.'_'.$size.'.'.$extension;

$avatar = Image::make($avatar_file);
$avatar->fit($size);

$storage->put($avatar_file_name, (string) $avatar->stream(), 'public');
}
}
39 changes: 38 additions & 1 deletion app/Models/Contact/Contact.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
use Illuminate\Database\Eloquent\Relations\HasMany;
use Illuminate\Database\Eloquent\Relations\BelongsTo;
use Illuminate\Database\Eloquent\ModelNotFoundException;
use Illuminate\Contracts\Filesystem\FileNotFoundException;
use App\Http\Resources\Address\AddressShort as AddressShortResource;
use App\Http\Resources\Contact\ContactShort as ContactShortResource;
use App\Http\Resources\ContactField\ContactField as ContactFieldResource;
Expand Down Expand Up @@ -937,14 +938,50 @@ public function getAvatarURL($size = 110)
return $this->avatar_external_url;
}

$original_avatar_url = Storage::disk($this->avatar_location)->url($this->avatar_file_name);
$original_avatar_url = $this->avatar_file_name;
Copy link
Member

Choose a reason for hiding this comment

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

PSR4

$avatar_filename = pathinfo($original_avatar_url, PATHINFO_FILENAME);
$avatar_extension = pathinfo($original_avatar_url, PATHINFO_EXTENSION);
$resized_avatar = 'avatars/'.$avatar_filename.'_'.$size.'.'.$avatar_extension;

return asset(Storage::disk($this->avatar_location)->url($resized_avatar));
}

/**
* Delete avatars files.
*/
public function deleteAvatars()
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why we need to create a function in Contact object just for this, especially if this process will only happen once for this migration. Is it really useful? Can we move this function elsewhere ?

Copy link
Member Author

Choose a reason for hiding this comment

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

When a contact already has an avatar (not gravatar), if you update the image, today the old image stays in the store forever. I'm not sure it's a voluntary behavior. I've created this function to remove the old image, but we can imagine an other behavior...

Copy link
Member Author

Choose a reason for hiding this comment

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

And today a user can flood us by uploading as much as images he can, and they will stay in the store...

Copy link
Member

Choose a reason for hiding this comment

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

This is true. Thanks for the explanation.
I wonder if we should create a service for this method instead, like what I've been started to do here: https://github.com/monicahq/monica/pull/1629/files#diff-b3c40f0913df027e06af970085f889ed

The idea: have a class called, for instance, deleteAvatarForContact. This is explained here.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

After a bit reflexion: this does not apply here, even I think Services are great, but it's used for Controller, to handle request the same way for htpp and api. In this case, I already have the $contact object, and I just pass it to the Service: there is no interest.

{
if ($this->avatar_location == 'external') {
return;
}

$this->deleteAvatarSize();
$this->deleteAvatarSize(110);
$this->deleteAvatarSize(174);
}

/**
* Delete avatar file for one size.
*/
private function deleteAvatarSize($size = null)
{
$avatar_file_name = $this->avatar_file_name;
if (! is_null($size)) {
$filename = pathinfo($avatar_file_name, PATHINFO_FILENAME);
$extension = pathinfo($avatar_file_name, PATHINFO_EXTENSION);
$avatar_file_name = 'avatars/'.$filename.'_'.$size.'.'.$extension;
}

try {
$storage = Storage::disk($this->avatar_location);
if ($storage->exists($avatar_file_name)) {
$storage->delete($avatar_file_name);
}
} catch (FileNotFoundException $e) {
return;
}
}

/**
* Returns the source of the avatar, or null if avatar is undefined.
*/
Expand Down
1 change: 1 addition & 0 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
"laravel/socialite": "^3.0",
"laravel/tinker": "^1.0",
"league/flysystem-aws-s3-v3": "~1.0",
"league/flysystem-cached-adapter": "^1.0",
"mariuzzo/laravel-js-localization": "^1.4",
"matriphe/iso-639": "^1.0",
"moneyphp/money": "^3.1",
Expand Down
110 changes: 107 additions & 3 deletions composer.lock
View file
Open in desktop

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

11 changes: 9 additions & 2 deletions config/filesystems.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
|
*/

'cloud' => 's3',
'cloud' => env('FILESYSTEM_CLOUD', 's3'),

/*
|--------------------------------------------------------------------------
Expand Down Expand Up @@ -60,7 +60,14 @@
'secret' => env('AWS_SECRET'),
'region' => env('AWS_REGION'),
'bucket' => env('AWS_BUCKET'),
'endpoint' => 'https://'.env('AWS_SERVER'),

/* TODO
'cache' => [
'store' => env('AWS_CACHE_STORE', 'memcached'),
'expire' => env('AWS_CACHE_EXPIRE', 600),
'prefix' => env('AWS_CACHE_PREFIX'),
],
*/
],

],
Expand Down