Should a custom find method on a Laravel Model be static?

Question

In the following Laravel 5 Model should the findByIdAndCourseOrFail method be static?

class Section extends Model {

    //should this method be static?
    public function findByIdAndCourseOrFail($id, $courseId)
    {

        $result = $this->where('id', $id)->where('course_id', $courseId)->first();

        if (!is_null($result))
        {
            return $result;
        }

        throw (new ModelNotFoundException())->setModel(Section::class);

    }


    }

With the controller:

class SectionsController extends Controller {

protected $sections;

public function __construct(Section $section)
{

    $this->sections = $section;

}

public function foo($id, $courseId)  //illustration only
{
     $section = $this->sections->findOrFail($id);   

     $section = $this->sections->findByIdAndCourseOrFail($id, $courseId);  
     //would need to be non-static

     $section = Section::findByIdAndCourseOrFail($id, $courseId); 
     //weird when compared with find above
}

On the one hand, we're not acting on a Section instance [See Note]. On the other hand, in a controller with auto-dependency injection through Laravel's service container we'd act on an instance: $sections = $this->sections-> findByIdAndCourseOrFail(7,3); and my IDE (PhpStorm) squawks if Static.

[Note]: This comment may be a misunderstanding of how Laravel Models work. For me, I would expect that find(), findOrFail() to be Class methods and thus Static as opposed to the instance that a find method would return.

Of course, if the method was changes to static, `$this->` would be changed to `self::` — Gazzer, Nov 06 '16 at 08:28
But then `where()` and `first()` need a change as well, and perhaps then the entire `Model` if those methods are defined there. — Xorifelse, Nov 06 '16 at 08:31
Why would where(), and first(0 need to be changed? These methods are defined (or accessible) within the Laravel Eloquent Model Class. — Gazzer, Nov 06 '16 at 14:19

score 1 · Answer 1 · answered Nov 06 '16 at 09:21

1

class Section extends Model {


public static function findByIdAndCourseOrFail($id, $courseId)
{

    $result = self::where('id', $id)->where('course_id', $courseId)->first();

    if (!is_null($result))
    {
        return $result;
    }

    throw (new ModelNotFoundException())->setModel(Section::class);

}


}

answered Nov 06 '16 at 09:21

Sherif

1,477
1
14
21

1

The question isn't about how to make the method static. Rather, whether it should be static or not. – Gazzer Nov 06 '16 at 14:17
well in this case ... it should be static, it makes more sense to me. you call this method staticly so it returns a model for you. – Sherif Nov 06 '16 at 14:46
if you already have the model, or it's instance it wouldn't make sense to call find on it – Sherif Nov 06 '16 at 14:47
That's not the case though with auto-dependency injection through Laravel's service container. Controllers have an instance of the class injected automatically using PHPs reflection mechanism. On these instances you can indeed call `find`. – Gazzer Nov 07 '16 at 05:42
1

this is really hard and I don't think there is one answer, so I'm gonna go with my intuition and say it should be static. – Sherif Nov 07 '16 at 08:33

Pitchinnate · Answer 2 · 2016-11-07T16:06:55.310

1

Personally I would make this a static method, I'm not sure if there is a "correct" answer though as either can be done. The way I kind of separate them in my mind is if I'm doing something to an instance of a model then I make it a normal public function. If I am doing something to the Collection I use a static. For example:

$person = new Person();
$person->setAdmin(true);
$person->save();

// OR

$admins = Person::getAdmins();

In the first example we have a specific instance of a Person and we are manipulating it, all code would be simply manipulating that specific instance. In the second example we are acting on the entire collection of Person and we want a collection of objects to be returned.

In your case you would have to initiate an instance of Section just to be able to use your non-static public method, like this:

$section = new Section();
$foundSection = $section->findByIdAndCourseOrFail(7,3);

So $section becomes a temporary variable that is never really used. On the other hand if you made it a static you could call it without having to do this.

$section = Section::findByIdAndCourseOrFail(7,3);

Hopefully that makes sense.

edited Nov 07 '16 at 16:06

answered Nov 07 '16 at 15:57

Pitchinnate

7,517
1
20
37

Thanks for this reply. Regarding: "In your case you would have to initiate an instance of Section just to be able to use your non-static public method, like this". In fact, this is not the case. In the controller, I use reflection and the instance is automatically injected into the constructor. `public function __construct(Section $sections){ $this->sections = $sections}` and in this case static makes less sense although generally I work along the logic that you do hence the question! – Gazzer Nov 11 '16 at 02:59
1

Regardless I don't think I would make an exception just for this case. What if you need to use that function somewhere else at a different time? I would stick to your standards. – Pitchinnate Nov 11 '16 at 04:00
I don't think this is making an exception. Automatic injection with the service container is used extensively, and the `find()` method itself is not called statically, and making it static would nullify the benefits of the injection as I would be making `find()` calls with the instance but not my `findByCustom()` calls which would look weird! – Gazzer Nov 11 '16 at 04:16
I guess, I'm looking for a more objective answer. Is the `find()` method itself `static` or not? It's hard to tell because it's buried in the `Eloquent\Builder` class, but it appears non-static. I suppose that refactoring to use a repository would make the point moot but this seems like overkill. – Gazzer Nov 11 '16 at 04:16
I've edited the question above to show the controller to better illustrate the dilemma. – Gazzer Nov 11 '16 at 04:23

Paul Spiegel · Accepted Answer · 2016-11-14T12:34:19.957

1

I'm not sure if local scopes are meant to be used like that. But it works for me on laravel 5.2:

public function scopeFindByIdAndCourseOrFail($query, $id, $courseId)
{
    $result = $query->where('id', $id)->where('course_id', $courseId)->first();

    if (!is_null($result))
    {
        return $result;
    }

    throw (new ModelNotFoundException())->setModel(Section::class);
}

In the controller you can use it both ways:

$section = Section::findByIdAndCourseOrFail($id, $courseId);

Or

$model = new Section();
$section = $model->findByIdAndCourseOrFail($id, $courseId);

edited Nov 14 '16 at 12:34

answered Nov 13 '16 at 15:04

Paul Spiegel

30,925
5
44
53

This indeed is the correct approach. Looking deeper into the source code, none of the model methods are actually static. model::find() is syntactic sugar, so using scopes that retain the same qualities is the correct approach. – Gazzer Dec 27 '17 at 14:37

Should a custom find method on a Laravel Model be static?

3 Answers3