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

[11.x] add ability to disable relationships in factories #53450

Merged
merged 7 commits into from
Nov 18, 2024

Conversation

browner12
Copy link
Contributor

sometimes when makeing or createing a model, we don't actually care about the relationships. this new property and method allow us to turn them all off at once. when disabled, any attribute that is assigned a factory will instead return null.

imagine an Organization and User models. the organizations table is:

  • id
  • name

and the users table is:

  • id
  • name
  • age
  • organization_id

When we define the UserFactory we setup the relationship with a factory:

public function definition(): array
{
    return [
        'name'            => fake()->name(),
        'age'             => rand(1, 100),
        'organization_id' => Organization::factory(),
    ];
}

This is great for when we seed and when we run feature tests, because it will automatically create the Organizations for us.

However, this is not always desired, often when you're writing Unit tests. Imagine my user model has an isAdult() function:

class User
{
    public function isAdult()
    {
        return $this->age >= 18;
    }
}

To write a Unit test for this I would make a User:

public function test_can_determine_adult()
{
    $user1 = User::factory()->make(['age' => 17]);
    $user2 = User::factory()->make(['age' => 18]);
    $user3 = User::factory()->make(['age' => 19]);

    $this->assertFalse($user1->isAdult());
    $this->assertTrue($user2->isAdult());
    $this->assertTrue($user3->isAdult());
}

Unfortunately, this would try storing Organizations in a database, even though we don't care about that. It would also fail if you didn't have RefreshDatabase or something similar on your test.

With this PR, you could easily disable all relationships defined in your factories and make this test work:

public function test_can_determine_adult()
{
    $user1 = User::factory()->disableRelationships()->make(['age' => 17]);
    $user2 = User::factory()->disableRelationships()->make(['age' => 18]);
    $user3 = User::factory()->disableRelationships()->make(['age' => 19]);

    $this->assertFalse($user1->isAdult());
    $this->assertTrue($user2->isAdult());
    $this->assertTrue($user3->isAdult());
}

sometimes when `make`ing or `create`ing a model, we don't actually care about the relationship. this new property and method allow us to turn them all off at once. when disabled, any attribute that is assigned a factory will instead return `null`.
@taylorotwell
Copy link
Member

taylorotwell commented Nov 11, 2024

Would the state of this property be lost if you called a method like afterMaking which creates a new factory instance? I also wonder if it should be named withoutRelationships to have a bit more consistency.

@taylorotwell taylorotwell marked this pull request as draft November 11, 2024 20:46
@browner12
Copy link
Contributor Author

that name change seems fine to me.

do you want the property adjusted as well to withoutRelationships, and then invert the logic?

I'm not positive, but you're probably right that anything that builds a new object probably would lose this state. the whole factory is a little bit of a mess of static vs object. not sure how you would want to handle this. I believe I had a PR a long time ago that got rid of the whole static new object paradigm but it was closed.

@browner12 browner12 marked this pull request as ready for review November 18, 2024 18:35
@browner12
Copy link
Contributor Author

@taylorotwell I changed the method name to your suggestion, and also updated the property name, and inverted the logic so it matched semantically.

I also persisted the state (I think) across calls to newInstance.

this is a separate discussion, but I'd love to talk about what the value of making the factories immutable was/is. it makes sense for Collections, but I'm really just struggling to see the value in factories, and it's making new features and maintenance difficult IMO.

@taylorotwell
Copy link
Member

Hey @browner12 - sorry about the immutable stuff... was it really that bad? It looks like you just had to pass the property through the constructor? 😅

@taylorotwell taylorotwell merged commit 038cde3 into laravel:11.x Nov 18, 2024
31 checks passed
@browner12
Copy link
Contributor Author

@taylorotwell it's definitely doable. what I'm struggling with though is I don't see the value of immutability on the factories. maybe I don't fully understand how everyone uses them, so I'd like to gain perspective on that. but if not, I think there'd be value in refactoring the immutability out, and simplifying the factories greatly.

@christophrumpel
Copy link
Contributor

@browner12 was this released? I thought so but can't find it anymore.

@christophrumpel
Copy link
Contributor

Ah I see it was renamed later to: withoutParents 👍

@browner12
Copy link
Contributor Author

@christophrumpel was it really? lol

@browner12 browner12 deleted the AB-factory-disable-relationships branch November 30, 2024 04:51
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.

3 participants