Skip to content

[5.0] Fix Bug with push() Method in Eloquent Models#6067

Closed
v-jacob wants to merge 1 commit into
laravel:masterfrom
v-jacob:master
Closed

[5.0] Fix Bug with push() Method in Eloquent Models#6067
v-jacob wants to merge 1 commit into
laravel:masterfrom
v-jacob:master

Conversation

@v-jacob
Copy link
Copy Markdown

@v-jacob v-jacob commented Oct 13, 2014

This fixes issue #6066 where using the push() method in eloquent models

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use short array syntax here? Taylor seems to be using it for all new code.

@GrahamCampbell
Copy link
Copy Markdown
Collaborator

👍

@taylorotwell
Copy link
Copy Markdown
Member

This doesn't seem right. Would'nt models already be an array?

@v-jacob
Copy link
Copy Markdown
Author

v-jacob commented Oct 14, 2014

$models is not an array currently, it is an object, the eloquent model. In 4.2, the Illuminate\Support\Collection::make() was converting the $model to an array like this return new static(is_array($items) ? $items : array($items));. In 5.0, this conversion is no longer being made.

If we dont convert $models to an array, Collection::make() will return an eloquent collection containing just the data in our model. If we convert to an array, it will return a collection containing our model object. As it stands now we are calling $model->push() on what ever value is in our table, which is why a Symfony \ Component \ Debug \ Exception \ FatalErrorException (E_ERROR) Call to a member function push() on string error is coming up

@v-jacob
Copy link
Copy Markdown
Author

v-jacob commented Oct 14, 2014

I don't know if the change to Illuminate\Support\Collection::make() was intended behavior or not, but converting $models to an array fixes the issue with push().

@taylorotwell
Copy link
Copy Markdown
Member

Fixed a little differently. I still don't think this code was totally correct.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants