[BUG] Cascade persist order

[mpiot]

Q	A
Foundry's version?	8.0.5
PHP version	8.4
PHPUnit version	13
symfony/framework-bundle version?	8.0.5
Do you use dama/doctrine-test-bundle?	no
Which reset database strategy do you use?	schema
Do you use Foundry's PHPUnit extension?	yes

I've previously post an issue #1038, detecting an issue in the availability of Children fron parent inside Factory afterInstantiate method.
But it seem, there is another problem with doctrine cascade, I do a little reproducer: https://github.com/mpiot/zenstruck-foundry-issue-1100.

If we create 3 entities:

• A: Parent, OneToMany on B
• B: First child, ManyToOne on A, OneToMany on C
• C: Second Child, ManyToOne on B

Each entity having a PrePersist EventListener, Doctrine and Factory call them in the same order:

• A: PrePersist
• B: PrePersist
• C: PrePersist

But there is a difference in collection accessibility:

• Doctrine:
  • A PrePersist: Count of Bs: 1
  • B PrePersist: Count of Cs: 1
  • B PrePersist: Has A: yes
  • C PrePersist: Has B: yes
• Factory:
  • A PrePersist: Count of Bs: 0
  • B PrePersist: Count of Cs: 0
  • B PrePersist: Has A: yes
  • C PrePersist: Has B: yes

How to use reproducer:
https://github.com/mpiot/zenstruck-foundry-issue-1100

docker compose up
symfony server:start -d
APP_ENV=test symfony php bin/phpunit

[nikophil]

Hi @mpiot

thanks for the reproducer. Is this a regression? I don't think so, and I don't even think it is a bug: in your first example you explicitly call $a->addB(), so your $bs are added to A::$bs

basically, when you do AFactory::createOne();, Foundry does the following, in this order:

$entityManager = self::getEntityManager();

$a = new A();
$entityManager->persist($a);

$b = new B();
$b->setA($a);
$entityManager->persist($b);

$c = new C();
$c->setB($b);
$entityManager->persist($c);

$entityManager->flush();

no A::addB() called, no collection hydrated at the "pre persist" time. The reason why the condition self::assertSame($b, $a->getBs()->first()); does not fail is thanks to Doctrine's lazy loading I guess.

That being said, I think it would be not so hard to automatically call adders on collections by using the property hydrator when we set the property.

But that would not totally fix your problem, because in some cases we're calling $em->persist() very early (which is what triggers Doctrine's "pre persist" event) so this has to be changed, and this might be a little bit more challenging.

Willing to try a PR?

[mpiot]

I continue to use v1 (1.38.6), because of lot of breaking changes, when I have some times I try to upgrade the version.

Actually, it break things, because in some entities (eg: A), we define properties based on B content, and B define properties based on C content. It works well when use it, and with foundry ^1, but when using foundry ^2 errors appear because trying to loop over children to define some properties, but without success.

Here the problem is that the persisting order is good, but the collection is still empty.
But the other side do something strange too (I've use afterInstantiate to remove from collection sometimes), if we define A in the BFactory, the collection of bs is set in A (normally, we should only have it on B but A should not be informed about that).

In code we call the setter in the adder to permit both to know about the other.

I've started to change some prePersist location, but it's called many time: on B prePerists (maybe 20 times), I generate the info for A. Before, we just call it once on A.

[nikophil]

I continue to use v1 (1.38.6), because of lot of breaking changes

sorry to hear that

Actually, it break things

still, for Foundry 2 which exist for more than 1 year and half, this would be considered as a new feature 😅

But actually this is not this complex to do, I'm currently working on it, and Foundry could benefit from that, so... stay tuned!

[nikophil]

after some digging, the problem is actually more complex than I thought and I don't see how to fix it correctly in all cases (meaning: with all possible "cascade: persist" variants)

If a relation is configured to cascade persist, Doctrine will trigger PrePersist events for the relation as well as for the current object when $em->persist() is called.

Consider this code:

#[ORM\Entity(repositoryClass: ARepository::class)]
#[ORM\HasLifecycleCallbacks]
class A
{
    #[ORM\OneToMany(targetEntity: B::class, mappedBy: 'a', cascade: ['persist', 'remove'])]
    private Collection $bs;

    public function __construct()
    {
        $this->bs = new ArrayCollection();
    }

    public function addB(B $b): void
    {
        if (!$this->bs->contains($b)) {
            $this->bs->add($b);
            $b->setA($this);
        }
    }

    #[ORM\PrePersist]
    public function prePersist(): void
    {
        dump("A PrePersist");
    }
}

#[ORM\Entity]
#[ORM\HasLifecycleCallbacks]
class B
{
    public function __constuct(
	    #[ORM\ManyToOne(inversedBy: 'bs', cascade: ['persist'])]
        #[ORM\JoinColumn(nullable: false)]
	    public A $a;
    ){}

    #[ORM\PrePersist]
    public function prePersist(): void
    {
        dump("B PrePersist");
    }
}

Notice the cascade: ['persist'] on the ManyToOne, and that A is required to create B, because it is in its constructor. This kind of mapping is perfectly correct, even if the additional cascade persist is debatable, this is a very common way of writing Doctrine relations.

With this configuration you could not rely on PrePersist events to control the collection A::$bs. Because A::prePersist() will be called when $em->persist() is called on the first B, so it will not be called when A have the full collection.

Thus, I'm sorry but unless we come up with a good solution (PRs are welcome of course), I'm reluctant to make this change, because the only solution I found to make it work was a bit hacky, and it would only work in certain scenario (ie: the ones where ManyToOne does not cascade persists).

As a side note: I really feel like Doctrine events are clunky and I try to never rely on them 😁 😅

[mpiot]

Not sure of that, but actually I put withoutPersisting() when I want to use cascade system (I've done a PR a long time ago about cascade that in the v1).

Actually, the problem seems to be don't call the adder but the setter from the other side.

In my mind, if I write a Factory like that:

final class AFactory extends PersistentObjectFactory
{
    protected function defaults(): array
    {
        return [
            'bs' => BFactory->withoutPersisting()->many(1)
        ];
    }

I want to use the cascade persist (Doctrine do the stuff, because I disable persisting), and use the adder from the A class.

But, if I write something like:

final class BFactory extends PersistentObjectFactory
{
    protected function defaults(): array
    {
        return [
            'a' => AFactory::new()
        ];
    }

I want to create A before, then call the setter from the B class.

Actually, it seems the system always use the second way.
That's why we don't retrieve children in the collection, but children have the parent relation setted.

In fact, the Persist order is ok, because we dump it in the same order in both cases, just the setter of the other side is called instead of the adder.

There is a similar issue, I think due to a fix to an older issue where we don't find children in the collection of the class in afterInstantiate. Because of that, in the second case, the A class know about B collection (have a B), but because we never call the adder it should not (we fill the relation in the oher side inside the factory to explicitely call the setter and not give the information to A).

[nikophil]

Not sure of that, but actually I put withoutPersisting() when I want to use cascade system

I think that in Foundry 2.x this is not true, Foundry will explicitly call $em->persist() on your B entity as well

Actually, the problem seems to be don't call the adder but the setter from the other side.

yes that was more or less what I was describing here. We can make the change so that the adders are called, I'm totally OK with that. But I've already tried it, and it won't fix your "pre persist" problem: because of this line, $em->persist(A) is called before any B gets persisted.

When you do AFactory::createOne(), what's happening inside Foundry is:

• 1 => create A
• 2 => call $em->persist(A) (because of the mentioned line)
• 3 => create B
• 4 => call $em->persist(B)
• ...

so, even if the adders are called, your problem will stay the same.

I'd be happy to merge a PR If you find a way to fix your problem, but we need to support every possible combination of cascade persist for all existing relationships - which could be very tricky sometimes

[mpiot]

Do you have your try (adder call) ? Because, I've take a look but not really understand how it works... I remember take days to understand the previous version and find a way, but the code seems changed a lot.

Can it works without break if:

• In AFactory set 'bs' => Bfactory::new()->many(1) <= Call the A->add(B), and don't call the B->set(A) if the adder do not do it internally.
• In BFactory set 'a' => AFactory::new() <= Call the B->set(A), and don't call the A->add(B) if the setter do not do it internally.

Inside the Factorychain, if an intermediate Factory have a withoutPersisting(): disable the ->persist() call, that permit to let doctrine manage it ?

In the first version, I know we have done something similar to manage it.

[nikophil]

Do you have your try (adder call) ?

yes, I've tried it

Can it works without break if: [...]

yeah but... that's not so simple, the number of edge cases we have to handle is really huge, and the code has to work in every case.

In the first version, I know we have done something similar to manage it.

yes I know, but v1 was also buggy in a lot of different ways, and the code was very much hard to maintain. v2's code is not perfect neither, and I think at some point it would need a rewrite of some part to completely decouple "persistence" from "object factory" but I've still not found the time to do it, nor the good design I'd like to achieve.

BTW, we've migrated almost all tests from v1 to v2, so v1 was lacking a lot of tests cases (hence the regressions). And we're now testing A LOT more cases regarding cascade persist, and all other stuff that may differ from one project to another (v1 only had ~500 test cases and v2 has almost 2000).

here is a PR where the adders are automatically called: #1104

I've also reproduced your case in the test \Zenstruck\Foundry\Tests\Integration\ORM\EdgeCasesRelationshipTest::pre_persist_can_access_populated_one_to_many_collection

Don't hesitate to use this PR to give a try to fix your problem