Skip to content

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

Merged
taylorotwell merged 1 commit into
laravel:5.6from
themsaid:pr/11013
Mar 2, 2018
Merged

[5.6] Prevent considering an array attribute as callable in model factories#23372
taylorotwell merged 1 commit into
laravel:5.6from
themsaid:pr/11013

Conversation

@themsaid
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

@roberto-aguilar
Copy link
Copy Markdown
Contributor

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

@themsaid
Copy link
Copy Markdown
Member Author

themsaid commented Mar 2, 2018

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

@roberto-aguilar
Copy link
Copy Markdown
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