Where would you move this code?

Grab from Github and study: https://laravel.com/docs/5.2/quickstart-intermediate

https://github.com/laravel/quickstart-intermediate

Hi;

I know how to do it. Several ways. I am asking different user's opinions.

Jeff Teaches several ways but he doesn't make it clear which one is prefered and why.

That's why I wanna hear other's opinions.

Tanx

I'd put the validation in a FormRequest object: https://laravel.com/docs/5.7/validation#form-request-validation

The rest is fine in the controller, however I like to just use

//$post = new Post;
//$post->title = $request->title;
//$post->body = $request->body;
//$post->user_id = \Auth::user()->id;
//$post->save();

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

and set title, body and user_id in the $fillable array on the model: https://laravel.com/docs/5.7/eloquent#mass-assignment

So all of that could be reduced to just

    public function store(PostRequest $request)
    {
        Post::create($request->all());

        return redirect('/posts');
    }

I'm still a little older style so I would put the validation part in a validation class like @cronix mentioned then I would create a repository for the store method but you would still need to call that store method from the controller.

I would not store it all in the controller because the store method could grow and might eventually meet other conditions from other controllers.

My models are very lean and basically just set the fill areas and then relationships. It has been years since I used a model like a repository.

Why is bascially to keep things lean and dry.

I often put the model save into the form request class so my controller might look like;

    public function store(PostRequest $request)
    {
        $request->persist();

        return redirect('/posts');
    }

and the form request

<?php

namespace App\Http\Requests;

use Illuminate\Foundation\Http\FormRequest;

class PostRequest extends FormRequest
{
    /**
     * Determine if the user is authorized to make this request.
     *
     * @return bool
     */
    public function authorize()
    {
        return true;
    }

    /**
     * Get the validation rules that apply to the request.
     *
     * @return array
     */
    public function rules()
    {
        return [
            'title' => 'required|unique:posts|max:255',
            'body' => 'required',
            ];
    }

    public function messages()
    {
        return [
            'title.required' => 'Hey buddy, you forgot a title',
        ];
    }

    public function persist()
    {
        return $this->user()->post()->create([
            'title' => $this->title,
            'body' => $this->body,
        ]);
    }

}

        $post = new Post;
        $post->title = $request->title;
        $post->body = $request->body;
        $post->user_id = \Auth::user()->id;
        $post->save();

Is controller code.

Cronix's idea seems like the cleanest one.

@Spporoguy why would you add a repository when you could just go like Cronix mentioned?

I don't understand why it's a good practice to add repositories.

Thanks

@cronix... using your suggestion, how do you add user_id?

$post->user_id = \Auth::user()->id; 

Maybe something like this?

 public function store(StoreBlogPost $request)
    {
        $request['user_id'] = \Auth::user()->id;
        Post::create($request->all());
        return redirect('/posts');
    }

<?php

namespace App;

use Illuminate\Database\Eloquent\Model;

class Post extends Model
{
    public $fillable = ['title', 'body', 'user_id'];
    public function comments()
        {
            return $this->hasMany(Comment::class);
        }

   public function user()
        {
            return $this->belongsTo(User::class);
        }       

}

I am sure there is a better way.

I would set a default user_id in the model itself if not yet set.

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

    static::creating(function($model)) {
      if (!$model->user_id) {
        $model->user_id = \Auth::id();
      } 
    }
  }
}

You should not need to know that there is a user_id column in the Post table.

Use a relationship and create the related post model (as I illustrated)

@Snapey

Hey man. I read your method and I get it.

I know you're an expert forum member. No doubt you have a point.

However, I don't understand why you're doing the "database business" in FormRequest.

I was under the impression that FormRequest is for form validation and we use model, controller or repository for database-business.

What's the deal man?

Thanks

I think this is more of an example provided by @Snapey

I could see instead of doing exactly what he showed, using a repo for this. Maybe in his example there is only database busisness... so adding a repo would just be an added layer of abstraction that has no value. Or I am wrong. That could also be it...

I use form request for dealing with forms. I know it wont be popular with some. Is a bloated controller better?

Anyway the last comment was about assigning user_id with post

to show Im not alone, see this from 9:00 minutes

https://laracasts.com/series/lets-build-a-forum-with-laravel/episodes/55

@behnampmdg3

I had some trouble with repositories when I first learned about them. I did not learn them in school but here in the trenches so please consider this when you read my ideas about them.

When they were the hip thing to do on Laracasts it was all about, " I might change my database from mysql to Maria " and " they also let me set up interfaces where I can see a quick list of all my functions "! Laravel 5.0.0 was really into Repositories!

Jeffery a few months back decided that it was cool to keep things simple so some people jumped on that concept. I was like, " huh?!? After all this talk about using this and that pattern, I can go back to being plain simple to get it done!?! " ... lots of pain.

For me, I like repositories because your function(s) could grow.

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

Which just grabs all data and slams it into the database.

But what happens if you want to Capitalize First letters, check to see if a user is an adult so you could also add them to a mailing list, or any other type of data changes.

You could just do this in the Controller. If this were just a simple controller and one section needed all kinds of data changing and checking, then I might just do it in the controller but then my controller wouldn't be "pretty'.

Another way to look at a repository file is like I said to keep code DRY. Maybe you might have a few controllers that need to do something with users (it's always the users! lol) so you call the user repository when needed and you can keep your code organized.

What is best practice for this? Honestly, wait another 6 months and I have a feeling some other pattern will be the best practice.

If your code is easy to read, the logic doesn't require a map and you keep consistent across the whole project, then do what you are doing now. I would just focus on DRY and trying to reuse as much code as possible because it will keep you on your toes about function creep.

Hi, @SapporoGuy

Thanks, it makes sense. I get the same feeling

This guy @Snapey seems to be one of the best in the forum.

I am going with his approach even though it doesn't make 100% sense to do CRUD in FOrRequest thingy.

@SapporoGuy , @behnampmdg3 Everybody has his/her preference and that could be based on many things and there is no real "right" or "wrong" way of doing it (within boundaries of course). It is an endless (interesting) discussion and you probably will change your mind every month or so when you start developing. But the most important part in all of this is: consistency. If you are going to save the models in the FormRequest model, stick with that option and do that for all of your models. If you are going to save it in the controllers, do it the same way in all of your controllers.

@click

Exactly! I noticed this with Jeffery too. His early videos and later ones give different opinions.

Consistency and DRY are 2 things that I really think help an application even if a design decision is not that great, at least, you have code that can read at a later date and don't have to deal with multiple styles.

And do not forget my favorite method that (in my opinion trumps all... even DRY).

KISS. Keep. It. Simple.

We as dev's always have this big picture mentality, while very viable, sometimes is own downfall. You can not, nor will you ever, think of every single scenario, so why try? Do what is required and nothing more.

I watched an interesting video that talked about doing the min amount of code to make things work, this is a good way to look at things.

You can have a cart, and you can do an insane number of different things to the data. But what value do most provide? Very small amounts, are you really ever going to have to deal with a person coming to your shop and ordering 15,000,000 of your auto-biographies? I doubt it, so why build logic to handle that ridiculous notion?

@cmdbueno makes a good point about coding to a minimal requirement. Starting off as hobbyist I often wanted to cover lots of cool functionality but in the real world this will cause problems for you at your job. If the requirements want ABC; only do ABC. I used to think this was slacking off but in reality most clients don't care about your cool tweaks or they might start expecting that you can deliver ABCD and expect to be only charged for ABC. For personal projects this is a reason why you have so many unfinished projects - you never get a round to implementing ABC and DEFGHI and J!

Here is a hint about corporate. If they say they want ABC say AB is doable in the time frame and that C might not make it. Of course, don't do that for easy things that won't have many bugs or small changes. Corporate is not a small agency but about budgets and making sure the budget scales out through the year.

@snapey

How is it possible that 'PostRequest ' can access $this->user()?

@behnampmdg3 because the form request is an instance of Request object which provides the current user via ->user()

@snapey

I am changed your code to UpdateOrCreate, should technically work but I get

"The title has already been taken."

public function persist()
    {
        return $this->user()->posts()->updateOrCreate([
            'title' => $this->title,
            'body' => $this->body,
            'youtube_video_id' => $this->youtube_video_id,
            'category_id' => $this->category_id,
        ]);
    }

What is the best practise?

Thank you

This is a totally different question!

You just are not using updateOrCreate correctly..

There should be two parameters. The first are required for a 'where' statement, the second parameter is an array of new field values.

However, your error message 'The title has already been taken' is from a unique validation rule - not the code you show. Your unique rule says that the title cannot be repeated

@snapey

Thanks.

2 things.

1 - For update, is there a better way to access id in form request?

public function persist()
    {
        $id=request()->route()->parameter('post')->id;
        return $this->user()->posts()->updateOrCreate(
            ['id' => $id],[
            'title' => $this->title,
            'body' => $this->body
        ]);
    }

2 - For create, this doesn't work obviously, it looks for id.

Or do you think it would be a better practice to just do this

public function persist()
    {
        return $this->user()->posts()->Create(
            [
            'title' => $this->title,
            'body' => $this->body
        ]);
    }

    public function update()
    {
        $id=request()->route()->parameter('post')->id;
        return $this->user()->posts()->updateOrCreate(
            ['id' => $id],[
            'title' => $this->title,
            'body' => $this->body
        ]);
    }

@behnampmdg3

pass the object back from the controller?

controller

public function update(ProfileRequest $request, User $user)
{
    
    $request->persist($user);

ProfileRequest

public function update(User $user)

// now work with the existing model

regarding your first point, the route parameters should be available in the request object depending on what you called them in the route.

eg $this->id or $this->slug etc

For resource controllers $this->photo for instance, depending on the name of the resource

If you php artisan route:list you will see the name of the parameter