Framework: Calling `$model->save()` does not work if the `$model` object does not have the `id` attribute

[AlexandreBonneau]

• Laravel Version: 5.5

Description:

If you retrieve a Model object (in my example User) using ::where() instead of ::find() without specifying that you want to retrieve the id field, any subsequent $user->save() call will fail silently without showing any error.
For newcomers (and perhaps even more advanced users) this is a huge time wasting problem since it relies on the implicit knowledge that id should be retrieve beforehand.

Steps To Reproduce:

Using that code:

// Get the user
$user = User::where([
    ['username', '=', $dataJSON['username']],
])->first(['api_token']);

// Write the new API Token
$apiToken = str_random(60);
$user->api_token = $apiToken;
$user->save(); // This fails silently, no errors shown

To fix that, you just need to change:

$user = User::where([
    ['username', '=', $dataJSON['username']],
])->first(['api_token']);

// to:
$user = User::where([
    ['username', '=', $dataJSON['username']],
])->first(['api_token', 'id']); // Added `'id'` here

The ->save() method should either:

• complain that it has no way of finding that specific user to update, or
• fail since it tries to find a record with a primary key of null, or
• the $user object should keep a link to the data somehow so that retrieving 'id' is not necessary.

Lastly, the documentation about updates could show an example where ::where() is used to retrieve, modify then save the model.
Or at least it could say that id is needed in order for save() to work correctly.

[srmklive]

This is not an error with the framework. You need to define a custom primary key for your model like this:

 protected $primaryKey = 'my_custom_key';

[devcircus]

@srmklive I don't think that is the issue in this case.
To test the given issue, just open tinker and do the following, with any regular model that has a standard incrementing id:

$user = App\User::where('email', '[email protected]')->first(['name']);
// returned $user will be an instance of App\User, but will only contain a 'name'.
// App\User #323 {
//    'name': 'Clay Stone'
//}
$user->favorite_color = 'blue';
$user->save();
// the save method will not fail and TRUE will be returned, however the model instance isn't actually updated.

I really don't think it is something that needs to be fixed, but maybe documented more clearly. The User model that is returned is basically a new instance with a 'name' property. It's not the actual instance representing that row in the database. We could possibly always make sure the primary key is included, but probably not worth the trouble.

[sedkis]

I agree with @devcircus. I don't understand why the save() method would return true if it hasn't done anything.

[themsaid]

I think save() here creates a new model?

[devcircus]

Probably so. I didn't check it but that makes sense.

[36864]

Quick tests on tinker show eloquent just tries to update a user with id "NULL"

>>> $user = App\User::first();
=> App\User {#748
     id: 1,
     name: "test",
     email: "[email protected]",
     created_at: "2017-11-03 13:14:52",
     updated_at: "2017-11-03 13:14:52",
   }
// select * from `users` limit 1;

>>> $user = App\User::select('name')->first();
=> App\User {#752
     name: "test",
   }
// select `name` from `users` limit 1;

>>> $user->save();
=> true
// no query executed

>>> $user->name = 'test2';
=> "test2"
>>> $user->save();
=> true
// update `users` set `name` = 'test2', `updated_at` = '2017-11-03 13:18:56' where `id` is null;

>>> App\User::all();
=> Illuminate\Database\Eloquent\Collection {#749
     all: [
       App\User {#99
         id: 1,
         name: "test",
         email: "[email protected]",
         created_at: "2017-11-03 13:14:52",
         updated_at: "2017-11-03 13:14:52",
       },
     ],
   }
// select * from `users`;

>>> dump($user)
App\User {#752
  #fillable: array:3 [
    0 => "name"
    1 => "email"
    2 => "password"
  ]
  #hidden: array:2 [
    0 => "password"
    1 => "remember_token"
  ]
  +exists: true
  +wasRecentlyCreated: false
  #attributes: array:2 [
    "name" => "test2"
    "updated_at" => "2017-11-03 13:18:56"
  ]
  #original: array:2 [
    "name" => "test2"
    "updated_at" => "2017-11-03 13:18:56"
  ]
  #changes: array:2 [
    "name" => "test2"
    "updated_at" => "2017-11-03 13:18:56"
  ]
// truncated for brevity
}
=> null

My guess is, since "exists" is set to true, Eloquent tries the update and there's no null check for the id. Out of curiosity, I tried calling refresh() on the model:

>>> $user->refresh()
Illuminate\Database\Eloquent\ModelNotFoundException with message 'No query results for model [App\User] '

Fixing this would require always selecting the id field and storing it, even if the user didn't ask for it. That might be breaking if someone is relying on a specific list of columns to be fetched and iterated through instead of directly accessed by name.

[themsaid]

Yeah, so make sure the ID is always selected for this to work, I don't think we want to enforce that in the core, but you can suggest it in laravel/internals if you want.

[AlexandreBonneau]

I'm not sure why this is issue is closed.
The problem is still present : no errors whatsoever are shown when using save() without an id.

I understand that you guys are not willing to 'fix' that by always selecting the id (although perhaps Laravel could store that in a way that could not add any side effects), but at least a notice or warning should be shown when save() is used with a null id, don't you think?

[devcircus]

Seems like something should be tweaked here, but not sure what. I've attached a screenshot showing the progression in tinker. @themsaid

It's actually quite easy to follow what's going wrong, isDirty() evaluates to true, but performUpdate expects a primary key to be available. Also, at the end of performUpdate, it just returns true regardless.

Personally, I would never expect this code to work and would have never tried it without first seeing this issue, but I can see how it can be confusing.

[hastinbe]

I think my commit above is the appropriate solution. By simply throwing an exception we prevent the problem and make developers aware of a bug in their existing apps.

[Niwantha97]

$signup=new Signup;

    $signup->email= $request->email;
    $signup->password= $request->pass;
    $signup->save();

not working help me