Skip to content
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

[5.6] Prevent considering an array attribute as callable in model factories #23372

Merged
merged 1 commit into from
Mar 2, 2018

Conversation

themsaid
Copy link
Member

@themsaid themsaid commented Mar 2, 2018

Having:

$factory->define(App\User::class, function (Faker $faker) {
    return [
        'name' => ['Storage', 'Bar'],
    ];
});

While building the factory, is_callable(['Storage', 'Bar']) will return true so the code will try to call the Bar method on the Storage class and that'll error.

In this PR we exclude array forms from being dealt with as callables.

@themsaid themsaid changed the title [5.6] Prevent considering an array attribute as callable while model factories [5.6] Prevent considering an array attribute as callable in model factories Mar 2, 2018
@roberto-aguilar
Copy link
Contributor

roberto-aguilar commented Mar 2, 2018

Wow, that's a very curious finding @themsaid, nice catch!

I just have a question, what is the real use case behind this? when do you want to declare a factory that has one array as property?, i tried this case with a non existent class (and therefore a not existen method) and it throws a:

Illuminate\Database\QueryException with message 'PHP Notice: Array to string conversion.

On the contrary, i think that it will be useful to be able to still be able to do this:

$factory->define(App\User::class, function () {
    return [
        'lorem' => [new App\Ipsum, 'create'],
    ];
});

as proposed in #20692

@devcircus
Copy link
Contributor

Check #23361 for the use case that discovered the bug.

@roberto-aguilar
Copy link
Contributor

Oh, i get it now, a json casteable attribute, thanks @devcircus 👍

@themsaid
Copy link
Member Author

themsaid commented Mar 2, 2018

@DojoGeekRA I was surprised it's not discovered earlier actually :)

@roberto-aguilar
Copy link
Contributor

Yeah, me too, and even more considering that i use that kind of attributes a lot 😅

I totally agree with the fix then, thanks @themsaid!

@taylorotwell taylorotwell merged commit 90f785a into laravel:5.6 Mar 2, 2018
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