Support closures for model data #56

dbushy727 · 2020-06-26T04:53:32Z

What & Why

Currently in the Laravel factories setup, we are able to use a function when defining a field if we rely on data being dynamically set earlier in the factory.

https://laravel.com/docs/7.x/database-testing#relationships

In the library as it stands today, it throws an error when trying to use a closure.

I love this library because I was in the process of transitioning my factories into classes and this library made it so much easier, but I rely on closures a lot for dynamically generating content that is dependent on the fields that are created earlier. I think others who also have already been using closures might find this update useful.

Solution

This PR aims to allow closures to be used when setting data in the getDefaults method as well as using a closure to override on make or create.

dbushy727 · 2020-06-26T04:57:35Z

src/TranslatesFactoryData.php

+    private static function isCallable($field): bool
    {
-        return collect($defaultModelFields)
-            ->map(function ($item) {
-                if ($this->isFactory($item)) {
-                    return $item->create()->getKey();
-                }
-
-                return $item;
-            })
-            ->toArray();
+        return is_callable($field) && ! is_string($field) && ! is_array($field);
+    }
+
+    private function transformModelFields(array $defaultModelFields): array
+    {
+        foreach ($defaultModelFields as &$field) {
+            $field = $this->transformField($field, $defaultModelFields);
+        }
+
+        return $defaultModelFields;
+    }
+
+    private function transformField($field, array $defaultModelFields)
+    {
+        if ($this->isFactory($field)) {
+            return $field->create()->getKey();
+        }
+
+        if ($this->isCallable($field)) {
+            return $field($defaultModelFields);
+        }
+
+        return $field;


This portion was heavily inspired by how Laravel's FactoryBuilder@expandAttributes method works.

The main takeaway here is that we use foreach with a variable passed by reference so with each iteration, the individual field transforms into the closure's return value, thus making it usable in the next factory fields. With map, the fields aren't affected until it loops all the way through.

I can provide screenshots if i didn't explain this part well.

Looks good. Except I'm not sure what you meant with map?

For me the tests were passing as well with this:

return collect($defaultModelFields) ->map(fn ($field) => $this->transformField($field, $defaultModelFields)) ->toArray();

Did I miss your point?

yeah, i think my test wasnt testing the actual problem with using map. I have updated the case in 627161f

The problem is when i define an array of defaults, for example:

public function getDefaults(Generator $faker): array { return [ 'first_field' => fn (array $factory) => 'some string', 'second_field' => fn(array $factory) => someFunctionThatRequiresString($factory['first_field']), ]; }

In the array above each key's value is a function. When we loop through these one at a time and transform them to the callbacks return value, the first_field key will turn into a string.

When we get to the second field, it's relying on $factory['first_field'] to already be a string, not a closure.

When we use ->map to iterate, we get to the first_field, we transform it into a string, but don't update it yet. Instead, it holds in a different memory location and will switch at the end. Then, when we get to the second_field key, it throws an error because someFunctionThatRequiresString requires a string, but a closure is passed because the first field hasn't been transformed yet.

using foreach with passing $field by reference via &$field, it allows the value to transform on each iteration, rather than waiting to switch the entire array at once to the new array of values.

Here is a printout of what the $defaultModelFields looks like on each iteration using map and foreach:

Using Map

// first iteration array:2 [ "name" => Closure(array $ingredient) {#1354 class: "Christophrumpel\LaravelFactoriesReloaded\Tests\FactoryTest" this: Christophrumpel\LaravelFactoriesReloaded\Tests\FactoryTest {#759 …} } "description" => Closure(array $ingredient) {#1355 class: "Christophrumpel\LaravelFactoriesReloaded\Tests\FactoryTest" this: Christophrumpel\LaravelFactoriesReloaded\Tests\FactoryTest {#759 …} } ] // second iteration array:2 [ "name" => Closure(array $ingredient) {#1354 class: "Christophrumpel\LaravelFactoriesReloaded\Tests\FactoryTest" this: Christophrumpel\LaravelFactoriesReloaded\Tests\FactoryTest {#759 …} } "description" => Closure(array $ingredient) {#1355 class: "Christophrumpel\LaravelFactoriesReloaded\Tests\FactoryTest" this: Christophrumpel\LaravelFactoriesReloaded\Tests\FactoryTest {#759 …} } ]

Using Foreach

// first iteration array:2 [ "name" => & Closure(array $ingredient) {#1354 class: "Christophrumpel\LaravelFactoriesReloaded\Tests\FactoryTest" this: Christophrumpel\LaravelFactoriesReloaded\Tests\FactoryTest {#759 …} } "description" => Closure(array $ingredient) {#1355 class: "Christophrumpel\LaravelFactoriesReloaded\Tests\FactoryTest" this: Christophrumpel\LaravelFactoriesReloaded\Tests\FactoryTest {#759 …} } ] // second iteration array:2 [ "name" => "Basil" "description" => & Closure(array $ingredient) {#1355 class: "Christophrumpel\LaravelFactoriesReloaded\Tests\FactoryTest" this: Christophrumpel\LaravelFactoriesReloaded\Tests\FactoryTest {#759 …} } ]

Ah got it. Thanks for the details. Then I'm fine with the current implementation 👍

cool. I prefer moving things to collections as well, so i get the hesitation. I couldnt find a way to update the variables by reference via Collection Methods, so i opted for this. I believe this is how laravel handles it behind the scenes as well.

dbushy727 · 2020-06-26T04:58:48Z

src/TranslatesFactoryData.php

+        return is_callable($field) && ! is_string($field) && ! is_array($field);
+    }
+
+    private function transformModelFields(array $defaultModelFields): array


I also renamed this because i felt it wasn't just transforming factories anymore, but not super opinionated on this, so if you feel strongly about the name, i can change it back.

Thanks, I also think the name is now better 👍

dbushy727 · 2020-06-26T05:00:09Z

tests/FactoryTest.php

+use ExampleApp\Models\Group;
+use ExampleApp\Models\Ingredient;
+use ExampleApp\Models\Recipe;


I think my IDE just auto-alphabetized these.

Ok no worries. I will see if I can automate that with the Github Actions as well. I just want them to be consistent, not matter which way.

yeah, im not opinionated on this either tbh. Whenever i auto-import something, it just re-alphabetizes the list. I can manually put it back if you like.

It's ok, just leave it 👍

christophrumpel · 2020-06-29T12:33:25Z

Hey, thanks a lot for this PR. Just came back from holidays and need to give a closer look 👍

dbushy727 · 2020-06-29T18:57:01Z

Hey, thanks a lot for this PR. Just came back from holidays and need to give a closer look 👍

how dare you take time off from open source 😜 .. ha, of course, take your time.

christophrumpel

Just went through. Looks good to me. Just some small comments.

Thanks a lot for this PR and I'm happy that this package could help you with your projects.

christophrumpel · 2020-06-30T08:43:25Z

src/TranslatesFactoryData.php

+        return is_callable($field) && ! is_string($field) && ! is_array($field);
+    }
+
+    private function transformModelFields(array $defaultModelFields): array


Thanks, I also think the name is now better 👍

christophrumpel · 2020-06-30T08:57:08Z

src/TranslatesFactoryData.php

+    private static function isCallable($field): bool
    {
-        return collect($defaultModelFields)
-            ->map(function ($item) {
-                if ($this->isFactory($item)) {
-                    return $item->create()->getKey();
-                }
-
-                return $item;
-            })
-            ->toArray();
+        return is_callable($field) && ! is_string($field) && ! is_array($field);
+    }
+
+    private function transformModelFields(array $defaultModelFields): array
+    {
+        foreach ($defaultModelFields as &$field) {
+            $field = $this->transformField($field, $defaultModelFields);
+        }
+
+        return $defaultModelFields;
+    }
+
+    private function transformField($field, array $defaultModelFields)
+    {
+        if ($this->isFactory($field)) {
+            return $field->create()->getKey();
+        }
+
+        if ($this->isCallable($field)) {
+            return $field($defaultModelFields);
+        }
+
+        return $field;


Looks good. Except I'm not sure what you meant with map?

For me the tests were passing as well with this:

return collect($defaultModelFields) ->map(fn ($field) => $this->transformField($field, $defaultModelFields)) ->toArray();

Did I miss your point?

christophrumpel · 2020-06-30T08:58:07Z

tests/FactoryTest.php

+use ExampleApp\Models\Group;
+use ExampleApp\Models\Ingredient;
+use ExampleApp\Models\Recipe;


Ok no worries. I will see if I can automate that with the Github Actions as well. I just want them to be consistent, not matter which way.

christophrumpel · 2020-06-30T08:59:04Z

tests/FactoryTest.php

@@ -192,6 +193,31 @@ public function it_lets_you_use_faker_for_defining_data(): void
        $this->assertIsString($group->mobile);
    }

+    /** @test * */
+    public function it_lets_you_use_a_closure_for_defining_data(): void


Please change it to it_lets_you_use_a_closure_for_defining_default_data

fixed in 345e064

christophrumpel · 2020-06-30T08:59:25Z

tests/FactoryTest.php

+    }
+
+    /** @test * */
+    public function it_lets_you_use_a_closure_for_overriding_data(): void


And here please it_lets_you_use_a_closure_for_overriding_default_data

fixed in 345e064

dbushy727 · 2020-06-30T16:04:30Z

tests/FactoryTest.php

+    public function it_lets_you_use_a_closure_for_overriding_default_data(): void
+    {
+        $ingredient = IngredientFactoryUsingClosure::new()->create([
+            'name' => fn (array $ingredient) => 'Basil',


I updated the test to define two closures. This will accurately test that the second closure can use the value from the first closure.

christophrumpel

Awesome. Looks good to me! I think we are ready to merge, right?

dbushy727 · 2020-07-02T07:00:37Z

Awesome. Looks good to me! I think we are ready to merge, right?

Yep, let’s do it

christophrumpel · 2020-07-02T07:43:36Z

Wohoo. Thanks a lot for the PR. I will update the docs and release a new version today 👍

dbushy727 added 2 commits June 25, 2020 21:45

create test cases for accepting closures as data fields

7833926

update transformation step to account for closures

f22166e

dbushy727 commented Jun 26, 2020

View reviewed changes

christophrumpel reviewed Jun 30, 2020

View reviewed changes

dbushy727 added 2 commits June 30, 2020 08:32

change test names

345e064

fix test to fail if using map

627161f

dbushy727 requested a review from christophrumpel June 30, 2020 16:03

dbushy727 commented Jun 30, 2020

View reviewed changes

christophrumpel reviewed Jul 2, 2020

View reviewed changes

christophrumpel merged commit 38d5407 into christophrumpel:master Jul 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support closures for model data #56

Support closures for model data #56

dbushy727 commented Jun 26, 2020 •

edited

dbushy727 Jun 26, 2020

christophrumpel Jun 30, 2020

dbushy727 Jun 30, 2020 •

edited

christophrumpel Jun 30, 2020

dbushy727 Jun 30, 2020

dbushy727 Jun 26, 2020

christophrumpel Jun 30, 2020

dbushy727 Jun 26, 2020

christophrumpel Jun 30, 2020

dbushy727 Jun 30, 2020

christophrumpel Jun 30, 2020

christophrumpel commented Jun 29, 2020

dbushy727 commented Jun 29, 2020

christophrumpel left a comment

christophrumpel Jun 30, 2020

christophrumpel Jun 30, 2020

christophrumpel Jun 30, 2020

christophrumpel Jun 30, 2020

dbushy727 Jun 30, 2020

christophrumpel Jun 30, 2020

dbushy727 Jun 30, 2020

dbushy727 Jun 30, 2020

christophrumpel left a comment

dbushy727 commented Jul 2, 2020

christophrumpel commented Jul 2, 2020

Support closures for model data #56

Support closures for model data #56

Conversation

dbushy727 commented Jun 26, 2020 • edited

What & Why

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbushy727 Jun 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophrumpel commented Jun 29, 2020

dbushy727 commented Jun 29, 2020

christophrumpel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophrumpel left a comment

Choose a reason for hiding this comment

dbushy727 commented Jul 2, 2020

christophrumpel commented Jul 2, 2020

dbushy727 commented Jun 26, 2020 •

edited

dbushy727 Jun 30, 2020 •

edited