[5.4] Call getKey() if instance of factory model

[dwightwatson]

I've seen a few examples where a closure is used in a factory definition to build up a related model, and then return that new model's ID. Here's an example of how a Post factory might generate it's User association.

$factory->define(App\Post::class, function (Faker\Generator $faker) {
    return [
        'user_id' => function() {
            return factory(App\User::class)->create()->id;
        }
   ];
});

Getting the id attribute from the new model doesn't really look quite right, so I'm proposing we call getKey() on the result of the Closure if it's an Eloquent model. Would just make it a tad cleaner to write.

$factory->define(App\Post::class, function (Faker\Generator $faker) {
    return [
        'user_id' => function() {
            return factory(App\User::class)->create();
        }
   ];
});

[huntie]

Damn, this broke most of my unit tests.

I was using factory states to create model variants with defined loaded relations by setting object fields to instances of Model from other factories. This was a useful pattern, since states provide a clean way to define these variants, and directly assigning a model object meant my entire test suite ran without need for a database.

Simplified example:

$factory->state(Post::class, 'withAuthor', function (Generator $faker) {
    return [
        'author' => factory(User::class)->make(),
    ];
});

With a change at this level (where a model instance is reduced to its key after being returned in a factory closure or directly assigned as an attribute), I now have to move this logic into local helper methods in my tests themselves. A pain to trace the cause to here, and not expected at a patch release level :(

[dwightwatson]

Ah, sorry about that - I suspect your use-case is a little out of the ordinary. Anyway, I've made another PR above that should fix the issue for you - it will only call getKey() on models that have been persisted to the database (and have a primary key), otherwise will fall back to the raw provided value.

[huntie]

Yeah, I suppose my expectation when setting a model attribute is that it can be an instance of any PHP object it needs to be (e.g. Carbon) – Model as a value is special, but this is totally allowed currently on a regular Eloquent model (I wouldn't try to save it :P).

I don't know if treating an attribute differently in the context of factories is particularly helpful for only the slight benefit above, but perhaps what Taylor brings up in reply to #18843 suggests that, if this is a useful addition, it should be implemented at the Eloquent level when qualified against an existing foreign key perhaps – this is pretty much the magic behaviour of assigning relations already – if you wanted to set the key, would you not use the value?

I appreciate my use case is non-typical, and so I'd rather leave it as is than add extra conditional behaviour to the framework.

Thanks for replying so swiftly :)