Cleaning up a Method

One thing I would maybe do is move the logic of uploading the cover_image to the model itself. I would also add an observer method to the model for setting the approved attribute.

// Your post model
class Post extends Model
{
    protected $guarded = []; // for mass assigning in the controller

    public function setCoverImage($file)
    {
        // your logic for storing the file

        // then update the model (variables come from your logic)
        $this->update(['cover_image' => $fileName, 'thumb_image' => $thumbnailpic]);
    }

    public static function boot()
    {
        parent::boot();

        static::creating(function ($post) {
            $post->approved = auth()->user()->type == 'admin' ? '1' : '0';
        });
    }
}

With that in your model now, your controller method could be something like this:

public function store(PostRequest $request)
{
    // Create post
    $post = new Post([
        'title' => $request->input('title'),
        'url' => $request->input('url'),
        'slug' => str_slug($request->input('title')),
        'body' => $request->input('body'),
        'user_id' => auth()->id(),
    ]);

    // Set cover image if set
    if ($request->hasFile('cover_image')) {
        $post->setCoverImage($request->file('cover_image'));
    }

    return redirect('/')->with('success', 'Post Created');
}

Wow this definitely looks a lot cleaner. I gave it a shot but came up against an error when I moved the line

$extension = $request->file('cover_image')->getClientOriginalExtension();

over to the post.php file. I am no longer getting the $request variable that I was getting in the Store method.

Thanks Chris

Uploading file code goes to app\Services\FileUploader.php

if(auth()->user()->type == 'admin')
{
    $approved = '1';
}
    else
{
    $approved = '0';
}

Instead of this code you create one method in User model

class User extends Model
{
    public function isAdmin()
    {
        if($this->type == 'admin')
        {
            return true;
        }

        return false;
    }
}

auth()->user()->isAdmin() // this is available now

And code to store the post goes to (Laravel Form Request) app\Request\PostRequest.php @ save or persist or store

And you end up with

class PostRequest
{
    public function save()
    {
        // save the post
    }
}

class FileUploader
{
    public function store($file)
    {
        // store file
    }
}

public function store(PostRequest $request, FileUploader $uploader)
{
    $uploader->store(request('file'));

    $request->save(); // or persist() or store()
    
    return redirect('/')->with('success', 'Post Created');
}

I dont actually have a app\services\ folder or a fileuploader.php file so I am guessing I'll have to create it.

Like before, how do I pass the $request variable to fileuploader.php file?

Thanks!

You can pass the Request in the constructor of the FileUploader class or use request() helper method.

I would only pass the file part of the request. Like:

new FileUploader($request->file)

or

Sergiu17 posted already the way, I would go. He passes the file part of the request to a dedicated FileUploader store method.

public function store(PostRequest $request, FileUploader $uploader)
{
    $uploader->store(request('file'));

@Sergiu17 Your isAdmin function could even be reduced to a one line function

public function isAdmin()
{
    return $this->type == 'admin';
}

@erikverbeek this is awesome, thank you))

Like before, how do I pass the $request variable to fileuploader.php file?

$uploader->store( request('cover_image') ); // you pass variable with request() function

$request->file('cover_image') == request('cover_image')

those two things above are the same

@chrisgrim Reading over your post, you’re uploading an image and then saving a model instance.

The second part can be quickly cleaned up by marking your fillable properties and then calling:

$post = Post::create($request->all());

If only administrators can post approved posts, then I’d be tempted to add an approve() method to your Post model and call that if the user is an administrator:

if ($request->user()->isAdministrator()) {
    $post->approve();
}

As you can see, I’m getting the user from the request instead, and using an isAdministrator() method that you’ll need to add to your User model. It’s better to use named methods than sprinkle conditional checks like type == 'admin' all over your code.

Finally, your upload logic looks similar to the “old” style of uploading in PHP where you’d hand-generate filenames, parse the extension etc. Laravel has a filesystem component, so leverage it.

I’d move the uploading of the cover image to another method on the Post model. Say, setCoverImage($file):

class Post extends Model
{
    public function addCoverImage(UploadedFile $file)
    {
        return $this->update([
            'cover_image' => $file->store(),
        ]);
    }
}

You should also queue a job to generate the thumbnail at this point so the user isn’t waiting around for that to finish before you redirect. You could dispatch an event, that has a listener that will dispatch the job:

public function addCoverImage(UploadedFile $file)
{
    return $this->update([
        'cover_image' => $file->store(),
    ]);
    
    CoverImageUploaded::dispatch($this); // Dispatch event
}

With this, your controller method should be cleaned up to look something like this:

public function store(PostRequest $request)
{
    $post = Post::create($request->all())
                ->setCoverImage($request->file('cover_image'));

    if ($request->user()->isAdministrator()) {
        $post->approve();
    }

    return redirect()
        ->route('post.index')
        ->withSuccess('Post has been created.');
}

@martinbean I looked over your code and I was hoping I could clarify a few things.

ok for my postcontroller@store method I have this code

public function store(PostRequest $request)
    {
        
        $post = Post::create($request->all())
                ->setCoverImage($request->file('cover_image'));
        
        //what about these?
        'slug' => str_slug($request->input('title')),
        'user_id' => auth()->id(),

        // $post->save();
        return redirect()
        ->route('post.index')
        ->withSuccess('Post has been created.');

    }

My first question is about my slug and user_id. Both of these are not submitted with the form. If I just use the Post::create($request->all() it gives me an error for those because 1364 Field 'slug' doesn't have a default value.

When it comes to the media being uploaded, I am a bit more confused. Right now my Post.php file has this

public function setCoverImage($file)
    {
        $title = str_slug(request('title'));
        $extension = $request->file('cover_image')->getClientOriginalExtension();
        $fileNameToStore= $title.'.'.$extension;
        $thumbnailpic= 'thumb'.'-'.$fileNameToStore;
        $path = $request->file('cover_image')->storeAs('public/cover_images', $fileNameToStore);
        $source = storage_path().'/app/public/cover_images/'.$fileNameToStore;
        $target = storage_path().'/app/public/cover_images/' . $thumbnailpic;
        Image::make($source)->fit(540, 340)->save($target);
        // then update the model (variables come from your logic)
        $this->update(['cover_image' => $fileNameToStore, 'thumb_image' => $thumbnailpic]);
    }
    
    public function addCoverImage(UploadedFile $file)
{
    return $this->update([
        'cover_image' => $file->store(),
    ]);
    CoverImageUploaded::dispatch($this); // Dispatch event

It has the setCoverImage() and the addCoverImage() methods. I am confused if I am only supposed to have thereturn $this->update([ 'cover_image' => $file->store(), how do I get the naming how I would like it? For SEO reasons I would like to have the title always be the title of the post etc.

Thanks so much!

This bit is unnecessary:

if(auth()->user()->type == 'admin')
        {
            $approved = '1';
        }
        else
        {
            $approved = '0';
        }

If you don;t have an accessor on your model you could quickly do:

$approved = auth()->user()->type == 'admin' ? 1 : 0;

I'd personally extract the image upload logic to a resusable service and dispatch that to a queue.

In relation to your last question you could do use observers. Alternatively you could simply merge data into the request:

request()->merge([
   'slug' => str_slug($request->input('title')), 
   'user_id' => auth()->id()
]);

$post = Post::create($request->all())
                    ->setCoverImage($request->file('cover_image'));

@Robstar I tried this code

request()->merge([
        'slug' => str_slug($request->input('title')), 
        'user_id' => auth()->id(),
        'approved' => auth()->user()->isAdmin(),
        'cover_image' => $fileNameToStore,
        'thumb_image' => $thumbnailpic,
        ]);

        $post = Post::create($request->all());
                    // ->setCoverImage($request->file('cover_image'));

        return redirect()
        ->route('post.index')
        ->withSuccess('Post has been created.');

but I still get the error General error: 1364 Field 'slug' doesn't have a default value. Am I doing something wrong?