michaelnguyen547's avatar

code review

I have several models that upload to S3 , in all of them, I have 3 main functions

    public function retrieveFileUrl()
    {
        return ($this->file) ? Storage::disk('s3')->temporaryUrl($this->file, now()->addMinutes(5)) : '';
    }


    public function uploadToS3($newFile, $path)
    {
        $this->deleteFromS3();

        $this->file = Storage::disk('s3')->putFile($path, $newFile);

        $this->save();
    }

    private function deleteFromS3()
    {
        if ($this->file) {
            Storage::disk('s3')->delete($this->file);
        }
    }

I figure that I can create a class S3Uploader to handler all uploading to S3. I need

  • $model // to tell which table I am working on, can be user, document
  • $attribute // which column I am working on, $user->photo, $document->file
class S3Uploader
{
    public static function upload($model, $attribute, $request, $path)
    {
        rnew self($model, $attribute, $request, $path)->uploadToS3();
    }

    public function __construct(Model $model, $attribute, $request, $path)
    {
        $this->model = $model;
        $this->attribute = $attribute;
        $this->path = $path;
        $this->request = $request;
    }

    public uploadToS3() {

        if ($this->request->hasFile($this->attribute)) {
            $this->deleteFromS3();
            $this->model->{$this->attribute} = Storage::disk('s3')->putFile($this->path, $this->request->file($this->attribute));
            $this->model->save();
        }
    }

    public retrieveFromS3() {
        return ($this->model->{$this->attribute}) ? Storage::disk('s3')->temporaryUrl($this->model->$attribute, now()->addMinutes(5)) : asset('placeholder.jpg');
    }

    private deleteFromS3($attribute) {
        if ($this->model->{$this->attribute}) {
            Storage::disk('s3')->delete($this->model->{$this->attribute});
        }
    }
}

I think it does what I need, however, this $this->model->{$this->attribute} bothers me. Any suggestions for S3Uploader overall?

0 likes
2 replies
D9705996's avatar
D9705996
7 years ago
Best Answer
Level 51

This would make more sense as a Trait rather than a Class as you can then get access to the model through $this

If you add all your existing code to a new Trait e.g. App\Traits\Uploadable.php

<?php 

namespace App\Traits;

trait Uploadable
{
    public function retrieveFileUrl()
    {
        return ($this->file) ? Storage::disk('s3')->temporaryUrl($this->file, now()->addMinutes(5)) : '';
    }


    public function uploadToS3($newFile, $path)
    {
        $this->deleteFromS3();

        $this->file = Storage::disk('s3')->putFile($path, $newFile);

        $this->save();
    }

    private function deleteFromS3()
    {
        if ($this->file) {
            Storage::disk('s3')->delete($this->file);
        }
    }
}

You can the use this is you classes that need it

use App\Traits\Uploadable;

class 
{
    use Uploadable;

    $this->uploadToS3(...)
}

If you need to customise any specific you could add a default getter function in the trait and if you need to override you can do so in the class using the trait or look at adding a getter that returns a property e.g.

// On the trait

public function getUploadedAttribute() {
    return $this->uploadedAttribute;
}

// On your model

protected $uploadedAttribute = 'file';

Please or to participate in this conversation.