zenstruck / foundry

A model factory library for creating expressive, auto-completable, on-demand dev/test fixtures with Symfony and Doctrine.
https://symfony.com/bundles/ZenstruckFoundryBundle/current/index.html
MIT License
644 stars 70 forks source link

Feature request: Single Lazy Value #473

Closed ndench closed 1 year ago

ndench commented 1 year ago

First of all, thanks for the great bundle! It's been a massive improvement to our DX over the last few months ♥️

I have an interesting use case with regards to my entity model which has multiple relations between entities which need to be setup for the entity to be "valid".

For example, say we have a Project which has both a list of Tasks and a list of Users. A Task has a single User as the owner, but that owner must exist in the list of Project::users for that Task:

<?php

class User {}

class Task
{
    public function __construct(public readonly Project $project, public readonly User $owner) {}
}

class Project
{
    /** @var User[] */
    private array $users = [];
    /** @var Task[] */
    private array $tasks = [];

    public function addUser(User $user): void
    {
        $this->users[] = $user;
    }

    /** @return User[] */
    public function getUsers(): array
    {
        return $this->users;
    }

    public function addTask(Task $task): void
    {
        $this->tasks[] = $task;
    }

    /** @return Task[] */
    public function getTasks(): array
    {
        return $this->tasks;
    }
}

At the moment, my TaskFactory looks like this:

class TaskFactory extends ModelFactory
{
    protected function getDefaults(): array
    {
        $owner = UserFactory::new();

        return [
            'project' => ProjectFactory::new(['users' => [$owner]]),
            'owner'   => $owner,
        ];
    }

    protected static function getClass(): string
    {
        return Task::class;
    }
}

However, the UserFactory will actually create two different instances of a User, when in fact, I need them to be the same instance. There are currently two ways to work around this (that I know of):

  1. Use UserFactory::createOne() in getDefaults(), however this is not recommended
  2. Everytime this factory is used in tests, you must first create the project and the owner and pass them both into the factorry

That being said, I think there is a very simple feature we could add to enable this use case by building off the existing LazyValue.

class SingleLazyValue extends LazyValue
{
    private mixed $value = null;

    public function __invoke(): mixed
    {
        if ($this->value === null) {
            $this->value = parent::__invoke();
        }

        return $this->value;
    }
}

Which the TaskFactory would use like this:

    protected function getDefaults(): array
    {
-        $owner = UserFactory::new();
+        $owner = new SingleLazyValue(fn () => UserFactory::createOne());

        return [
            'project' => ProjectFactory::new(['users' => [$owner]]),
            'owner'   => $owner,
        ];
    }

I'm more than happy to submit a PR for this if you agree it's valuable. I'd also like some feedback on this approach (it does require either making LazyValue non-final, or duplicating it). Or potentially there's another approach I haven't considered?

kbond commented 1 year ago

Interesting use-case and solution! I'm all for a PR.

Add a flag to the existing lazy value object or new object?

ndench commented 1 year ago

A new object means we either have to make LazyValue non-final, or copy paste it's internals. But a bool flag on the LazyValue is not very readable IMO.

$owner = new LazyValue(fn () => UserFactory::createOne(), true);

I'd probably lean towards making LazyValue non-final. This would then allow users to extend it themselves if they have other use-cases they'd like to handle.

However, I'm unsure if it was made final for a specific reason originally?

nikophil commented 1 year ago

what about using a named constructor?

$owner = LazyValue::singleValue(fn () => UserFactory::createOne()); // or maybe a better name 😅
ndench commented 1 year ago

Perfect! Thanks for the idea @nikophil! I'll get a PR up soon.

jeffreymueller commented 1 year ago

If we are trying to allow other projects to implement their own, what about using an empty interface and replace $value instaceof self checks with $value instanceof LazyValueInterface. Would look something like the following:

interface LazyValueInterface { }

final class LazyValue implements LazyValueInterface
{

...

    public function __invoke(): mixed
    {
        $value = ($this->factory)();

        if ($value instanceof LazyValueInterface) {
            return ($value)();
        }

        if (\is_array($value)) {
            return self::normalizeArray($value);
        }

        return $value;
    }

    /**
     * @internal
     */
    public static function normalizeArray(array $value): array
    {
        \array_walk_recursive($value, static function(mixed &$v): void {
            if ($v instanceof LazyValueInterface) {
                $v = $v();
            }
        });

        return $value;
    }
}

Then projects can implement something invokable for their own needs

final class MyLazyValue implements LazyValueInterface
{
    public function __invoke(): mixed
    {
    }
}
nikophil commented 1 year ago

indeed, if we decide to allow users to define custom lazy values, we'd introduce an interface, but I'm really wondering if this would actually be used? I mean: besides the current point, do you see any other valid use case?

kbond commented 1 year ago

What about using the term memoize? A bit computer science-y but perfectly describes the intent.

nikophil commented 1 year ago

I like it, it express well what it does, and it will be documented anyway

ndench commented 1 year ago

Agreed LazyValue::memoize() it is.

RE

I mean: besides the current point, do you see any other valid use case?

Only other use case I can think of would be a more complicate construction than creating a single entity. However, the $factory is a callable, which should allow you to be as complicated as you like. You could even use a service with dependency injection and an __invoke() method defined.

So I don't think it's necessary to allow extension at this point. It can always be added later if a use-case arises.