New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Model::create() misses Database-Default-Values #21449

Closed
johannesschobel opened this Issue Sep 29, 2017 · 6 comments

Comments

Projects
None yet
4 participants
@johannesschobel

johannesschobel commented Sep 29, 2017

  • Laravel Version: 5.5.x
  • PHP Version: 7.1
  • Database Driver & Version:

Dear Laravel Dev-Team and community,

I recently stumbled upon an issue when creating a new model that has some default values set in the migration file. This causes clients that interact with the application to retrieve "wrong" (incomplete) data, which is - at least in my opinion - not desired behavior.

Description:

If i insert a new Model to the database, the default values from the database are correctly set (in the database), the Model, however, do not have them.

Example:

Consider the following example, that illustrates the issue in more detail:
I have a projects Table that holds all Projects in my web application. A Project may be flagged as is_private (boolean) to indicate if it is accessible by everyone (false) or not (true).

The database migration for this table looks like this (shortened)

$table->string('name');
$table->text('description');
$table->boolean('is_private')->default(false);

So the is_private field is optional with a default value false on the database. This is created correctly and works like a charm.

Now, I am making a Request to my API in order to create a new Project. The Request body looks like this:

{
   "data" : {
      "name" : "My Project",
      "description" : "This is a fancy description for the project"
   }
}

So there is no is_private attribute in the JSON Request body. Next, after sanitizing the request data I do a:

$project = Project::create($data); // where $data is all data from the request

As the is_private field is not passed (which is completely fine), this is not present in the $data array. If I then return the $project it is missing the is_private attribute (was it was never set!). When i return this to the client (e.g., by using a Transformer like league/fractal) this would result in

is_private = null // because it was not set in the request

instead of

is_private = false // this is the default value according to the database-migration that is set after creating the model

Steps To Reproduce:

Create a completely new Laravel application, add a new (optional) field to the users table and set a default value for this. Then use php artisan tinker to create a new User with

$user = User::create(['name' => 'a', 'email' => 'a@a.com', 'password' => 'test']);

Possible Solutions:

At the moment, I see 2 possible ways to solve this issue, however, both have different issues:

Refresh the Model after CREATE

When creating the model it must be immediately refreshed before returning. This would cause Eloquent to reload / refresh the entire model. Then, the "missing" attributes (which have default values on DB level) will be added.

$project = Project::create($data);
$project = $project->refresh();
return $project;

However, this results in an additional database-query to retrieve the newly created model..

Set Default Values on Model Level

I know that there is the possibility to set some kind of "default values" on the Model level by using

protected $attributes = [
   'is_private' = false;
];

This would cause Eloquent to add these attributes to a newly created model and then overwrite them with values that are passed to the ::create() method.
However, this way you would define "default values" on the database-layer and on the model layer - which is not convenient, I think.

Question

Is this a bug or is this a "feature"? If it is a "feature", how shall I tell Laravel to use the "correct" (depending on the point of view!) behavior. I define correct in that way, that a ::create() also returns the default values from the database - because the Model that was created does, obviously, have those values set!

Cheers and thanks a lot for your reply!
Johannes

@devcircus

This comment has been minimized.

Show comment
Hide comment
@devcircus

devcircus Sep 29, 2017

Contributor

There's really no other way to tell your code about something that happened at the database level, but to hit the db again. Otherwise, you have to tell the model beforehand about your default fields and values, just as you mentioned above. This is the way it's designed and has been discussed many times here.

Contributor

devcircus commented Sep 29, 2017

There's really no other way to tell your code about something that happened at the database level, but to hit the db again. Otherwise, you have to tell the model beforehand about your default fields and values, just as you mentioned above. This is the way it's designed and has been discussed many times here.

@themsaid

This comment has been minimized.

Show comment
Hide comment
@themsaid

themsaid Sep 29, 2017

Member

yes for these cases you have the choice to refresh the model and have this extra DB query, but we can't force it in the core since it's not always needed.

Member

themsaid commented Sep 29, 2017

yes for these cases you have the choice to refresh the model and have this extra DB query, but we can't force it in the core since it's not always needed.

@themsaid themsaid closed this Sep 29, 2017

@johannesschobel

This comment has been minimized.

Show comment
Hide comment
@johannesschobel

johannesschobel Sep 29, 2017

@themsaid would adding an additional autorefresh ( = false) param for the create() be an option?

johannesschobel commented Sep 29, 2017

@themsaid would adding an additional autorefresh ( = false) param for the create() be an option?

@johannesschobel

This comment has been minimized.

Show comment
Hide comment
@johannesschobel

johannesschobel Sep 29, 2017

@devcircus yes, but in case of really creating a new model it is quite obvious, that I would like to have the correct version of the object - and not a "half-filled" version.. So - at least for create - the current approach is not sufficient..

In return, this means, that there is absolutely no use-case to use ->default() in migrations, because you will need to implement it on Model level as well..

johannesschobel commented Sep 29, 2017

@devcircus yes, but in case of really creating a new model it is quite obvious, that I would like to have the correct version of the object - and not a "half-filled" version.. So - at least for create - the current approach is not sufficient..

In return, this means, that there is absolutely no use-case to use ->default() in migrations, because you will need to implement it on Model level as well..

@Kyslik

This comment has been minimized.

Show comment
Hide comment
@Kyslik

Kyslik Sep 29, 2017

Contributor

@johannesschobel you can always do that yourself, overload Laravel's create method and use what you've proposed (send $refresh boolean to the method as well).

->default() has its use-case even if model does not refreshes itself after creation, because user (of Laravel) mentally knows that such and such attribute is set to "default" value by database engine.

Contributor

Kyslik commented Sep 29, 2017

@johannesschobel you can always do that yourself, overload Laravel's create method and use what you've proposed (send $refresh boolean to the method as well).

->default() has its use-case even if model does not refreshes itself after creation, because user (of Laravel) mentally knows that such and such attribute is set to "default" value by database engine.

@johannesschobel

This comment has been minimized.

Show comment
Hide comment
@johannesschobel

johannesschobel Sep 29, 2017

@Kyslik yes, agree - but you will need to maintain this on 2 different "sides".. The database and model level.. This means, in return, that you don't have a "single point of truth"...

johannesschobel commented Sep 29, 2017

@Kyslik yes, agree - but you will need to maintain this on 2 different "sides".. The database and model level.. This means, in return, that you don't have a "single point of truth"...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment