0

Edit function:

public function editCheck($id, LanguagesRequest $request)
{
    try{
        $language = language::select()->find($id);
        $language::update($request->except('_token'));
        return redirect()->route('admin.languages')->with(['sucess' => 'edit done by sucsses']);
    } catch(Exception $ex) {
        return redirect()->route('admin.addlanguages');
    }
}

and model or select function

public function scopeselect()
{   
    return  DB::table('languages')->select('id', 'name', 'abbr', 'direction', 'locale', 'active')->get();
}
lagbox
  • 48,571
  • 8
  • 72
  • 83
abdurhman
  • 1
  • 1
  • 2
    ideally a scope should be building a query not returning a result ... why are you using Query Builder directly inside of your model? and regular collection's don't have a `find` method, only Eloquent Collections do, but you are using query builder there so it wouldn't return an Eloquent Collection – lagbox Dec 21 '21 at 00:13
  • also there is a `select` method of Query Builder already that is being blocked from you being able to use now when building queries with this Model, so not the best idea .... this isn't a scope you have written it is just a method that should be a real static method – lagbox Dec 21 '21 at 00:28

2 Answers2

2

This code is very inefficient, you're selecting every record in the table, then filtering it to find your ID. This will be slow, and is entirely unnecessary. Neither are you using any of the Laravel features specifically designed to make this kind of thing easy.

Assuming you have a model named Language, if you use route model binding, thing are much simpler:

  1. Make sure your route uses the word language as the placeholder, eg maybe your route for this method looks like:

    Route::post('/languages/check/{language}', 'LanguagesController@editCheck');
    
  2. Type hint the language as a parameter in the method:

    public function editCheck(Language $language, LanguagesRequest $request) {
    
  3. Done - $language is now the single model you were afer, you can use it without any selecting, filtering, finding - Laravel has done it all for you.

    public function editCheck(Language $language, LanguagesRequest $request) {
        // $language is now your model, ready to work with
        $language::update($request->except('_token'));
        // ... etc
    

If you can't use route model binding, or don't want to, you can still make this much simpler and more efficient. Again assuming you have a Language model:

public function editCheck($id, LanguagesRequest $request) {
    $language = Language::find($id);
    $language::update($request->except('_token'));
    // ... etc

Delete the scopeselect() method, you should never be selecting every record in your table. Additionally the word select is surely a reserved word, trying to use a function named that is bound to cause problems.

Don't Panic
  • 13,965
  • 5
  • 32
  • 51
1

scopeselect() is returning a Collection, which you're then trying to filter with ->find() which is a method on QueryBuilders.

You can instead filter with ->filter() or ->first() as suggested in this answer

$language = language::select()->first(function($item) use ($id) {
    return $item->id == $id;
});

That being said, you should really find a different way to do all of this entirely. You should be using $id with Eloquent to get the object you're after in the first instance.

Joundill
  • 6,828
  • 12
  • 36
  • 50
  • 1
    An additional note: there is a `find()` method on the `Illuminate\Database\Eloquent\Collection` object. However, the OP's scope isn't using Eloquent, so it is returning a plain `Illuminate\Support\Collection` object, which doesn't have a `find()` method. – patricus Dec 21 '21 at 04:54