[Feature Request] Call Repository method for saving

[vincentchalamon]

Description

I have a Factory which create a Foo Entity with a custom Repository as following:

#[ORM\Entity(repositoryClass: FooRepository::class)]
class Foo
{
    // ...
}

final class FooRepository extends ServiceEntityRepository
{
    // ...

    public function save(Foo $foo): void
    {
        // ...
    }
}

I need to call FooRepository::save when the Entity is saved, to perform some custom behavior.

Problem Statement

Currently, the PersistentObjectFactory::create method calls Configuration::persistence::save, which is internal so it cannot easily be overridden (and I guess it doesn't intend to be). Additionally, this method is overridden by PersistentProxyObjectFactory::create which is final.

Therefore, it's not possible to override the save process of an Entity in a Factory without duplicating the whole PersistentObjectFactory::create method and disabling the proxy feature (which I found useful).

Alternative Solution

A solution would be to use Foundry hooks (afterInstanciate and afterPersist), but this would require to duplicate the custom behavior code from the Repository::save method, or change my code design (move this custom behavior to a public method, for instance, but it would be inappropriate)

Proposal

Allow to optionally configure a Repository method to save the Entity would solve my issue:

final class FooFactory extends PersistentProxyObjectFactory
{
    // ...

    #[\Override]
    protected function initialize(): static
    {
        return $this
            // The repository might be injected in the callback
            // but I thought to maybe let the developer handle the persist himself/herself
            // in case of very-custom-persistence-not-using-the-repository (yes it is a valid word 😆)
            ->persistWith(fn (Foo $foo): void => self::repository()->save($foo));
    }
}

If this custom persistence callback is not set, the default behavior would continue ($om->persist()).

Of course, this persistence method would be call only if the persistence is enabled 😃

[dgoosens]

sounds like an excellent idea @vincentchalamon !

[nikophil]

hi @vincentchalamon 👋

does this save() method call $em->flush()?

I'm asking this, because in #836 I'm exploring a new way to save objects. flush() being a global thing, such feature may break this new mechanism.

[vincentchalamon]

hi @vincentchalamon 👋

does this save() method call $em->flush()?

I'm asking this, because in #836 I'm exploring a new way to save objects. flush() being a global thing, such feature may break this new mechanism.

Hi @nikophil,

It's possible but not compulsory:

public function save(Foo $foo, bool $flush = true): void
{
    // custom behavior
    $this->getEntityManager()->persist($foo);
    if ($flush) {
        $this->getEntityManager()-flush();
    }
}

final class FooFactory extends PersistentProxyObjectFactory
{
    // ...

    #[\Override]
    protected function initialize(): static
    {
        return $this
            // ...
            // Disable flush on Repository::save for #836 compliance
            ->persistWith(fn (Foo $foo): void => self::repository()->save($foo, false));
    }
}

[nikophil]

What would you expect Foundry to do towards Doctrine in this case? I guess you'd want it to let you handle anything related to persistence?

In #836, we're deferring the flush call for some reasons, that this mechanism would break.

Moreover this just seems like a fancy way of doing:

final class FooFactory extends PersistentProxyObjectFactory
{
    // ...

    #[\Override]
    protected function initialize(): static
    {
        return $this
            ->withoutPersisting()
            ->afterInstantiate(fn (Foo $foo): void => self::repository()->save($foo));
    }
}

A solution would be to use Foundry hooks (afterInstanciate and afterPersist), but this would require to duplicate the custom behavior code from the Repository::save method, or change my code design (move this custom behavior to a public method, for instance, but it would be inappropriate)

I'm not sure to understand this part: in your example Repository::save() is already a public method.

By the way, in the next minor release, we'll ship global hooks, then you'll be able to declare Foundry hooks as services, outside of a factory, which could help you not to duplicate your logic.

Aren't these workarounds sufficient to meet your needs?

[vincentchalamon]

I'm not sure to understand this part: in your example Repository::save() is already a public method.

By the way, in the next minor release, we'll ship global hooks, then you'll be able to declare Foundry hooks as services, outside of a factory, which could help you not to duplicate your logic.

Aren't these workarounds sufficient to meet your needs?

My needs are kinda specific (DDD project...), but I think this feature request would be useful even out of my needs in a generic way.

For instance, let's say I need to check business validation constraints on the Repository::save method:

final class FooRepository extends ServiceEntityRepository
{
    public function __construct(..., private readonly FooValidator $validator)
    {
        // ...
    }

    public function save(Foo $foo, bool $flush = true): void
    {
        // @throws FooValidationException in case of violation
        $this->validator->validate($foo);
        $this->getEntityManager()->persist($foo);
        if ($flush) {
            $this->getEntityManager()->flush();
        }
    }
}

Using Foundry hooks would require to duplicate this behavior in the Factory:

final class FooFactory extends PersistentProxyObjectFactory
{
    public function __construct(private readonly FooValidator $validator)
    {
        parent::__construct();
    }
    // ...

    #[\Override]
    protected function initialize(): static
    {
        return $this
            ->withoutPersisting()
            ->afterInstantiate(fn (Foo $foo): void => $this->validator->validate($foo));
    }
}

Note: In this example, this is a just a single line duplicated (with its DI), but it could be a more complex duplication of code.

If this approach doesn't fit your needs for Foundry developments, I would be happy to hear any suggestion to control the persistence of an Entity, without duplicating/refactoring the whole PersistentProxyObjectFactory nor PersistentObjectFactory, and ideally with a good DX (so without code duplication) 😃

[nikophil]

For instance, let's say I need to check business validation constraints

The next Foundry version got you covered for this as well 😆
https://symfony.com/bundles/ZenstruckFoundryBundle/current/index.html#hooks-as-service-global-hooks

Using Foundry hooks would require to duplicate this behavior in the Factory

I don't understand this duplication problem, in which way this code does not fit your needs?

    #[\Override]
    protected function initialize(): static
    {
        return $this
            ->withoutPersisting()
            ->afterInstantiate(fn (Foo $foo): void => self::repository()->save($foo));
    }

Basically this is what a persistWith() method would do internally, doesn't it?

And in Foundry 2.4, you could do this globally:

final class CustomFoundryPerisisterHook
{
    #[AsFoundryHook]
    public function afterInstantiateGlobal(AfterInstantiate $event): void
    {
        if (!$event->object instanceof CustomPersistedEntity) {
            return;
        }

        $this->customPersister->persist($object);
    }
}

I'm really not opposed to any new feature, but I'm a bit reluctant to add another way to do things you can already do. I need to understand why you cannot achieve what you want to do with the current API.

At some point we might offer a whole way to plug a fully custom persisting logic (there is an issue about that) but we're not ready yet, mainly because we're still quite coupled with Doctrine, and some stuff Foundry is doing is quite complex.

[vincentchalamon]

I don't understand this duplication problem, in which way this code does not fit your needs?

    #[\Override]
    protected function initialize(): static
    {
        return $this
            ->withoutPersisting()
            ->afterInstantiate(fn (Foo $foo): void => self::repository()->save($foo));
    }

Hmmm that would work indeed (I need to check). But won't be any side effect of disabling the Foundry persistence on this Entity, notably on Foundry proxies?

[nikophil]

notably on Foundry proxies?

nope, the proxies would still work: if you extends PersistentProxyObjectFactory, you will get a proxy.

But won't be any side effect of disabling the Foundry persistence on this Entity,

The more I think about it, the more I think you may have some troubles. We're relying on Doctrine metadata to deal with inversed relationships (mainly inversed one to one), and disabling persistence in the Factory would have this effect that Foundry would be less "clever".

What you need is to still rely on Doctrine metadata, but have full control over $em->persist() and $em->flush() calls? what about nested objects in your "main" object? I guess they are also Doctrine entities. Imagine this kind of code:

class AggregateRootFactory extends PersistentObjectFactory
{
    #[\Override]
    protected function initialize(): static
    {
        return $this
            // notice the "inner()" here, which is what you want, I guess
            ->persistWith(fn (AggregateRoot $ar): void => self::repository()->inner()->save($ar));
    }
}

AggregateRootFactory::createOne(
    [
        'property1' => ChildEntityFactory::new()
    ]
);

how the persistence of the ChildEntityFactory would be handled? from AggregateRootRepository::save()?

I think sharing some code would help to fully grasp the need

[nikophil]

We're relying on Doctrine metadata to deal with inversed relationships (mainly inversed one to one), and disabling persistence in the Factory would have this effect that Foundry would be less "clever"

I just thought that this part should be fixed: inside a kernel test case, even if we're disabling the persistence, we should be able to rely on Doctrine metadata to understand the relationships between objects, I'll soon fix this

[vincentchalamon]

Looks like your proposal almost fits my needs 😃

Thanks for your suggestion and feedback!

Feel free to close this issue or keep it open regarding your last comment.

[nikophil]

almost

❓ 👀

Feel free to close this issue or keep it open regarding

I'll close it at some point, and open a dedicated issue :)

[nikophil]

@vincentchalamon I've merged #841 and I will release it very soon in a patch version.

I'm closing this issue. Still, feel free to re-open it, if you're having troubles with the withoutPersisting() stuff, we'll find solutions.

[vincentchalamon]

almost

❓ 👀

@nikophil It's totally our project issue, I'm working on it, and Foundry has nothing to do with this "almost" limitation 😃

Details of the "almost" limitation

I'm working on a DDD project, which includes a bit of a complexity (welcome to DDD...). For a reason, we're separating Domain DTO to Infrastructure DTO (Doctrine Entity), but still want to use Foundry Factories in our tests, as it's particularly useful!

But Foundry cannot use persistence on Domain DTO as they're not Doctrine Entities. And ideally, we should not directly create Doctrine Entities but rely on Domain objects...

I've found a solution thanks to your proposal, but it has a limitation due to the separation of Domain & Infrastructure DTOs: we can't use proxy methods as the proxy methods (auto-refresh) will rely on the current object class...the Domain DTO (which is not a Doctrine Entity).

But as I said: it's totally our project issue, and I'm working on it 😃

Anyway, "almost" limitation apart, we still need to call the repository on object persist to check business rules compliance. And your solution fits our needs! Thanks!

[nikophil]

ok then!

the fix is released in https://github.com/zenstruck/foundry/releases/tag/v2.3.7