Skip to content

Fix insert() method for related models. #1143

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

Merged
merged 2 commits into from
Sep 2, 2012

Conversation

franzliedke
Copy link
Contributor

As described in this forum topic, passing a model object to a relationship's insert() method caused problems, if custom attribute setters were specified.

The reason for this is that the model wasn't filled with the raw attribute data. Instead, every attribute's value was passed to the attribute's setter method (if it existed), causing e.g. duplicate hashing of passwords.

In fact, the copying of the attributes of the model object passed to insert() to an entirely new object is completely unnecessary. Instead, we can simply set the foreign key and then save the model directly.

@stefanneubig
Copy link
Contributor

+1

1 similar comment
@curana
Copy link

curana commented Aug 26, 2012

+1

@taylorotwell
Copy link
Member

There is a behavior change here. One path returns ->save() which is a boolean, and other returns ->create() with is a model instance. A model instance should always be returned to not break expected functionality.

@curana
Copy link

curana commented Aug 27, 2012

@taylorotwell So what do you suggest, how to save objects in this scenario then?

@franzliedke
Copy link
Contributor Author

@taylorotwell Fixed. This wasn't properly documented in the method's PHPDoc, either. It returned false, too, when the insert failed.

(Right now, there'd be other return values in case the passed model already existed in the database. But then, you're not supposed to use insert() in those cases.)

taylorotwell added a commit that referenced this pull request Sep 2, 2012
Fix insert() method for related models.
@taylorotwell taylorotwell merged commit 852b10e into laravel:develop Sep 2, 2012
@taylorotwell
Copy link
Member

Thanks!

zoe-edwards pushed a commit to zoe-edwards/laravel that referenced this pull request Oct 14, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants