Named `FactoryCollection` constructors don't have proper type declarations

[janopae]

The constructor of FactoryCollection has a @phpstan-param Factory<TObject> $factory. Using this constructor is deprecated, and named constructors like set, range etc. should be used instead. However, these named constructors are missing this annotation. Also, the annotation is only for PHPStorm, not for Psalm.

This leads to Psalm errors like

Argument 1 of Zenstruck\Foundry\FactoryCollection::set expects Zenstruck\Foundry\Factory<object>, but My\Factory\Class provided (see https://psalm.dev/004)

[janopae]

One general problem with the FactoryCollection<T> template type is that it only references the class of the object to be produced – not the actual factory class to produce the object.

So the factory class is technically known when a factory collection is created (because a factory instance is passed), but the information gets lost in the construction.

Therefore, calls like

$factories = MyFactory::new()->many();

foreach ($factories->all() as $factory) {
    $factory->myMethodDefinedInMyFactoryClass();
}

are not possible, because the moment we get the factory in the loop, it is just a general Factory<MyObject>, not a MyFactory.

This could be changed if T on FactoryCollection was changed to represent the factory class instead of the to produced object class.

[nikophil]

Hi @janopae

sorry for the late reply.

One general problem with the FactoryCollection template type is that it only references the class of the object to be produced – not the actual factory class to produce the object.

This is actually true. In foundry v2, this is still the case. We have:

/**
 * @template T
 * @implements \IteratorAggregate<Factory<T>>
 */
final class FactoryCollection implements \IteratorAggregate

Maybe your solution would as well fix some problem with proxy/not-proxy objects in factory collection. But this would break some CI in the wild... on the other hand, I think a lot of user still have not migrated to Foundry v2, so maybe this would be acceptable...

I'm just wondering how we should document FactoryCollection::create() which is now:

    /**
     * @return T[]
     */
    public function create(array|callable $attributes = []): array

Any thoughts, @kbond?

By the way:

Therefore, calls like

$factories = MyFactory::new()->many();
foreach ($factories->all() as $factory) {
   $factory->myMethodDefinedInMyFactoryClass();
}

are not possible, because the moment we get the factory in the loop, it is just a general Factory, not a MyFactory.

genuine question: do you really need to do this? I usually go for MyFactory::new()->myMethodDefinedInMyFactoryClass()->many()->create() and never need to call FactoryCollection::all()

[janopae]

Thanks for taking the time to reply :)

I'm just wondering how we should document FactoryCollection::create()

I'd write it like this:

/**
 * @template TModel
 * @template TFactory of Factory<TModel>
 * @implements \IteratorAggregate<TFactory>
 */
final class FactoryCollection implements \IteratorAggregate
// ...
    /**
     * @return TModel[]
     */
    public function create(array|callable $attributes = []): array

genuine question: do you really need to do this? I usually go for MyFactory::new()-> myMethodDefinedInMyFactoryClass()->many()->create() and never need to call FactoryCollection::all()

In my case, I needed some data that was all the same for all created objects, and some data that needed to be different.

I wrote something like

$factories = MyFactory::new()->setDataThatShouldBeTheSame()->many();
foreach ($factories->all() as $factory) {
    $factory->dataThatShouldBeDifferent(faker()->randomElement($someDataToChooseFrom));
}

If there's a more elegant way to do this, I'd love to switch to that.

[nikophil]

Ho yes, of course, double @template, that makes sense. I'm in favor of this idea, even if it would break some CIs... that's not a big deal IMO. What's your opinon on this @kbond?

About $factory->dataThatShouldBeDifferent(faker()->randomElement($someDataToChooseFrom));, I think you could use the lazy() helper.