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

Nested model configuration #57

Open
booni3 opened this issue Jun 27, 2020 · 5 comments
Open

Nested model configuration #57

booni3 opened this issue Jun 27, 2020 · 5 comments

Comments

@booni3
Copy link

booni3 commented Jun 27, 2020

Is it currently possible to achieve something like this?

We have the relationship:

Order -> hasMany -> OrderItems

I want to create an order with a status of new and then create 4 order items with a status of allocated. Is it possible to hook into the orderItemFactory method from within the with method, and apply the allocated status?

// Test
OrderFactory::new()
    ->withOrderItems(5)
    ->create();

// OrderFactory.php
public function withOrderItems($qty)
{
    return $this->with(
        OrderItem::class,
        'orderItems',
        $qty
    );
}

// OrderItemFactory.php
public function allocated()
{
    return tap(clone $this)->overwriteDefaults([
        'state' => 'allocated',
    ]);
}
@booni3
Copy link
Author

booni3 commented Jun 27, 2020

I think what I am looking for is a syntax like this:

public function user_has_customers()
{
    $user = OrderFactory::new()
        ->withOrderItems(
            OrderItemFactory::times(30)->forProduct(ProductFactory::new())->allocated())
        )
        ->create();

    $this->assertCount(30, $user->orderItems);
}

(the above is from another factory package, but it has a little too much "magic" for my tastes and does not allow deeper configuration as I hope we can achieve with this package)

@christophrumpel
Copy link
Owner

Hey, thanks for trying out this package.

Here is how I would do that currently:

OrderFactory::new()
    ->withAllocatedOrderItems(5)
    ->create();

The withAllocatedOrderItems would be a new method you create on your OrderFactory:

public function withAllocatedOrderItems(int $times)
{
    $this->orderItems = OrderItemFactory::new()
                                            ->allocated()
                                            ->times($times)
                                            ->create();
    return $this;
}

To make this work you need to add an orderItems property to you class and to add this relation manually in the create/make method:

public function create(array $extra = []): Order
{
        $order = $this->build($extra);

	$order->saveMany($this->orderItems)

	return $order;
}

I just typed that out of my head, so it should work something like that and this way I have been doing it.

I also don't like it if there is too much magic, but I see that you scenario might be easier with a new feature. I think something like that would be cool right?

$order = OrderFactory::new()
        ->with(OrderItemFactory::new()->allocated()->times(30)->create())
        ->create();

or with a closure:

$order = OrderFactory::new()
        ->with(function() {
                return OrderItemFactory::new()->allocated()->times(30)->create())
        })
        ->create();

What do you think?

@shaffe-fr
Copy link
Contributor

Hi,
With the closure syntax, I don't see how we would guess the relation name on the model. Maybe you have another idea?

We could maybe allow MultiFactoryCollection instances in BaseFactory::with first argument.

 $user = OrderFactory::new()
        ->with(
            OrderItemFactory::times(30)->allocated(),
            'orderItems'
        )
        ->create();

However, how should we handle the third argument?

 $user = OrderFactory::new()
        ->with(
            OrderItemFactory::times(30)->allocated(),
            'orderItems',
            2
        )
        ->create();

Should we create 60 orders items? Should we ignore it? Should we throw an InvalidArgumentException?

@booni3
Copy link
Author

booni3 commented Jun 29, 2020

I am not too sure about the detail of the package, but from a users perspective, I would love to see the same kind of behaviour as in the model relationship builders.

i.e.

public function orderItems(): HasMany
{
    return $this->hasMany(OrderItem::class); // Laravel knows that primary key/forign keys are "id", "order_item_id".
    //
    return $this->hasMany(OrderItem::class, 'product_id'); // but we can override if needed
}

So if passing a factory into the with() method then can the 2nd argument be used only for an override, and the 3rd is not needed/invalid.

The syntax without the closure looks great

$order = OrderFactory::new()
        ->with(OrderItemFactory::new()->allocated()->times(30)->create())
        ->create();

I am sure you guys have already seen this, but along with Brents post, this article by Tighten provides an extremely configurable solution

@christophrumpel
Copy link
Owner

@shaffe-fr I mean we could guess the relation name from the factory class name without the factory and the plural version. This might work in most cases. of course not all, where the second argument could override that if needed. I get your point though. It also doesn't look that nice from a user point:

$user = OrderFactory::new()
        ->with(
            OrderItemFactory::times(30)->allocated(),
            'orderItems',
            2
        )
        ->create();

But it would work (after deciding what to do with the other arguments).

@booni3 Yeah I've seen both articles. Both are not packages and only need to solve a problem per "model". So their solution is similar to the one I provided first. (also the way I would do it)

Even if we can make this happen with one of the examples above, there might be more details where a user needs something similarly adapted. This is always where it gets tricky for a package to make everyone happy :-)

My main concern currently is how can we make this happen without making it more difficult for the current features. Using the with method with different arguments would also mean changing return types, given the user more options to use it, which could get more complicated for a user. I was also thinking about creating a new method.

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

No branches or pull requests

3 participants