Entity cascading unexpected behavior

[mpiot]

In the case we have 3 entities linked together likes:

class A
{
  /**
   * @ORM\OneToMany(targetEntity=B::class, mappedBy="a", cascade={"persist"})
   */
  private Collection $bs;

  private ?string $reference = null;

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

class B
{
  /**
    * @ORM\ManyToOne(targetEntity=A::class, inversedBy="bs")
    * @ORM\JoinColumn(nullable=false)
    */
  ​private ?A $a = null;

  /**
   * @ORM\ManyToOne(targetEntity=C::class, inversedBy="bs", cascade={"persist"})
   * @ORM\JoinColumn(nullable=false)
   */
  ​private ?C $c = null;

  ​private ?string $reference = null;
}

class C
{
  /**
   * @ORM\OneToMany(targetEntity=B::class, mappedBy="c")
   */
  private Collection $bs;

  ​private ?string $reference = null;

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

There is listener on A and C to generate tha A, B and C references. The A listener loop over the collection of B. It works well in the application, but when I want to create a fixture for A... It totally explode, when Foundry call the persist, there is no B inside the collection (in A class). But to generate the A reference I need the first B class of the collection and the linked C class.

Someone know how we can do this ? Actually I've something similar to:

AFactory::createOne();

Internally:

• AFactory call BFactory::new()->many(3);
• BFactory call CFactory::new();

[kbond]

Does it work if you create the factories manually on their own? Create C, create A, then create B with these?

I find using foundry for one-to-many a bit finicky for complex scenarios. I tend to favor creating many-to-one's only.

[mpiot]

It didn't for A and B, it's ok for C. In this case, C can live alone. But a B can't exists without a A, and a A can't exists without at least a B.

All work without the prePersist listener that generate the reference. Because to generate the A reference the listener get the first B, then the attached C. And to define the B reference, get the C then the A. A and B must be persisted together with a pre-existing C or by saving the C to.

Apparently, if I have correctly understand, foundry save the A entity in database (persist and flush) but have not yet added the B to the entity (there is no link, the adder is never called before), then when the listener call the getBs() method, it return an empty collection. This is the real problem, because in this case we just need A have B and B have C objects, to do the reference. After, we can save the first without B and C for exemple.

I can see that to in:

        $a= AFactory::new()
            ->instantiateWith(function(array $attibutes, string $class): object {
                dd($attibutes);
            })
            ->create()
        ;

The dumped array has an empty array for the Collection of B.

[kbond]

In your app where it works correctly, are you creating/persisting all the entities and then calling flush() just once?

I think this is related to #37. Are you able to test with #84. This allows you to create all your factories inside a callback. When they are created they are not immediately flushed. A single flush occurs after the callback has run. #84's description has an example on how to use.

[mpiot]

@kbond In the app, I just fush and persist one time, the A class, then it cascade persist B that cascade persist C.
I look at your PR and test it.

[kbond]

Ah, yes, the cascade persist - I'm not very familiar with that doctrine feature but I think that PR could still help.

[mpiot]

Yes, I try it. For the moment it return to me an error about auto refreshing:

RuntimeException: Cannot auto refresh "App\Entity\Specificity" as there are unsaved changes. Be sure to call ->save() or disable auto refreshing (see https://github.com/zenstruck/foundry#auto-refresh for details).

Related to the Factory::new() inside, I'll try to disable refreshing globally to check, then if it works, by disabled it only for that part.

[mpiot]

By disabling auto-refreshing it works well for that auto-refresh error. But, return the same error that before, the Collection in A class is still empty.

In the Factory, when we define attributes, and an attributes call a Factory, this one is called, procces its attributes, execute persist() then flush() on it, then comeback to the initial factory and continue to resolves its attributes, that's it ?

When it find a ->many(), it create a FactoryCollection, that can't match with a Doctrine Collection, then the adder is never call I think ? In my case, more than a problem of sore it in database before the other, it's a problem of we can't reach the Collection from the entity in the listener because never added. I've not well understand how it works. :/ (sorry)

[kbond]

For the moment it return to me an error about auto refreshing:

I've fixed this in the PR - disabling auto-refresh within the callback is no longer required.

In the Factory, when we define attributes, and an attributes call a Factory, this one is called, procces its attributes, execute persist() then flush() on it, then comeback to the initial factory and continue to resolves its attributes, that's it ?

Yes (but within the callback in #84 flush() isn't called for each factory, just persist()).

When it find a ->many(), it create a FactoryCollection, that can't match with a Doctrine Collection, then the adder is never call I think ? In my case, more than a problem of sore it in database before the other, it's a problem of we can't reach the Collection from the entity in the listener because never added. I've not well understand how it works. :/ (sorry)

Did you make any changes to the default instantiation of factories? If not, then the adder should be called (by symfony/property-accessor).

What happens when a FactoryCollection is encountered is: call FactoryCollection::create() which returns an array of objects. Using the default instantiator, this array is set on the object using the adders.

I know this is a bit of an ask but, can you create a minimal reproducer? Even better would be a failing test but I can use a reproducer to build a failing test myself.

[mpiot]

I investigate more, because in fact I have a collection that is filled with adder called and the other not. I check in my own code if something can do that.

Else, the only difference is that one is a Collection of a class with only scalar attributes, than the other is a Collection of a class with relation to other entities, and have Collection to.

[kbond]

I have a collection that is filled with adder called and the other not

How is the other's collection being set ->setCollection($collection)? That might be a problem although I'd expect the error to be different... Like a TypeError: "cannot set array as Collection" exception.

[mpiot]

Sorry, I didn't express myself well, both use adder. I just want to say for a collection the adder is called, but not for the other.
When I dump the attributes, I have an empty array for this Collection. I do not understand why, in my factory:

ProbeFactory.php

    protected function getDefaults(): array
    {
        return [
            //...
            'usages' => UsageFactory::new()->many(1),
            //...
            'probeAcoustics' => ProbeAcousticFactory::new()->withoutProbe()->many(1),
        ];
    }

Usages contains a Collection of persisted object, but the probeAcoustics is empty.

ProbeAcousticFactory.php

    protected function getDefaults(): array
    {
        return [
            'acoustic' => AcousticFactory::new(),
            'probe' => ProbeFactory::new(),
        ];
    }

    public function withoutProbe(): self
    {
        return $this->addState([
            'probe' => null,
        ]);
    }

[kbond]

$a= ProbeFactory::new()
    ->instantiateWith(function(array $attibutes, string $class): object {
        dd($attibutes['probeAcoustics']); // THIS is [] (an empty array)?
    })
    ->create()
;

[kbond]

Maybe a bug in Factory::normalizeCollection()? See how it's returning an empty array if inverse. I think I'm not taking cascade: persist into account here.

Can you try commenting out these lines and see if that works?

[mpiot]

I check it tomorrow, and say you what I find.
Thanks a lot for your help 🙏

[mpiot]

$a= ProbeFactory::new()
->instantiateWith(function(array $attibutes, string $class): object {
dd($attibutes['probeAcoustics']); // THIS is [] (an empty array)?
})
->create()
;

Yep it return an empty array.

I've commented lines in Factory::normalizeCollection(), by doing it only: it throw an error about not defined Probe (null) in ProbeAcoustic. Butby adding it with Factory::delayFlush seem works well for this part. Logical, it create the ProbeAcoustic, then use the adder to add it to Probe, at this point nothing flush, the adder create the link between us (addProbeAcoustic, call $probeAcoustic->setProbe()).

But, inevitably break all other parts because it create all dependencies and flush them incompletly (no added to the one that "invoke" them).

This is tricky because all work well, all sub Entities are created on demand, persisted and flushed, then after linked to the parent. In case of revert relation, the link is done in prePersist actually. This problem only come from eventListener used in the application, when this one need to access a revertCollection. (In the exemple bellow between Usage collection and ProbeAcoustic collection is here: Usage is a ManyToMany relation owned by Probe, but ProbeAcoustic is a OneToMany relation on Probe); that's why it's escaped in the Factory. But by escaping it, all inversed relation are empty on persist (not totally sure of having all understand).

[mpiot]

I thought about it for a while, I think there is 2 possibilities of creating the entity:

1. The entity need to be linked to another existing entity (eg: select a Category), that works very well, because we create, persist and flush it on this own, then refer to it in our entity and that's exactly the expected behavior.
2. The entity has a relation with a cascade={"persist"} on it, in this case:
   - The related entity must be created, but not persisted, and not flushed
   - Use our original entity setter/adder to attach this entity on, and continue (this adder, permit to reference this entity on the main one by adding it in the collection, or just set it. Often it call the "child" setter to refer the parent. On flush, because cascade persist, doctrine do the necessary).

I take a look about it, and see if I find an idea of how to do that. Normally it should solve the issue, and be more realistic with how the fixture is created, compared to how this same entity(ies) are created from the application logic.

[kbond]

I've commented lines in Factory::normalizeCollection(), by doing it only: it throw an error about not defined Probe (null) in ProbeAcoustic.

Yeah, I thought this might cause an issue with your other entities. I can try to create a failing test with cascade persist when I have a chance. I still think this is related to the cascade.

[mpiot]

A try to support relation with cascade persist: create the collection of Proxy, and don't persist them:

class Factory
{
    // ...

    /** @var bool */
    private $persist = true;

    /** @var bool */
    private $cascadePersisted = false;

    // ...

    /**
     * @param array|callable $attributes
     *
     * @return Proxy|object
     *
     * @psalm-return Proxy<TObject>
     */
    final public function create($attributes = []): Proxy
    {
        //...

        if (!$this->isPersisting() || true === $this->cascadePersisted) {
            return $proxy;
        }

        //...
    }

    //...

    final public function setCascadePersisted(bool $cascadePersisted): self
    {
        $this->cascadePersisted = $cascadePersisted;

        return $this;
    }

    //...

    private function normalizeCollection(FactoryCollection $collection): array
    {
        $field = $this->inverseRelationshipField($collection->factory());
        $cascadePersisted = $this->hasCascadePersist($collection->factory(), $field);

        if ($this->isPersisting() && $field && false === $cascadePersisted) {
            $this->afterPersist[] = static function(Proxy $proxy) use ($collection, $field) {
                $collection->create([$field => $proxy]);
                $proxy->refresh();
            };

            // creation delegated to afterPersist event - return empty array here
            return [];
        }

        return $collection->all($cascadePersisted);
    }

    private function hasCascadePersist(self $factory, ?string $field): bool
    {
        if (null === $field) {
            return false;
        }

        $collectionClass = $factory->class;
        $factoryClass = $this->class;
        $collectionMetadata = self::configuration()->objectManagerFor($collectionClass)->getClassMetadata($collectionClass);
        $classMetadataFactory = self::configuration()->objectManagerFor($factoryClass)->getMetadataFactory()->getMetadataFor($factoryClass);

        // Find inversedBy key
        $inversedBy = $collectionMetadata->associationMappings[$field]['inversedBy'] ?? null;

        // Find cascade metatada for the inversedBy field
        $cascadeMetadata = $classMetadataFactory->associationMappings[$inversedBy]['cascade'] ?? [];

        return in_array('persist', $cascadeMetadata, true);
    }
}

final class FactoryCollection
{
    /**
     * @return Factory[]
     *
     * @psalm-return list<Factory<TObject>>
     */
    public function all(bool $cascadePersisted = false): array
    {
        /** @psalm-suppress TooManyArguments */
        return \array_map(
            function() use ($cascadePersisted) {
                $cloned =  clone $this->factory;
                $cloned->setCascadePersisted($cascadePersisted);

                return $cloned;
            },
            \array_fill(0, \random_int($this->min, $this->max), null)
        );
    }
}

See to use it on no collection relation:

• ManyToOne
• OneToMany
• ManyToMany (owner side)
• ManyToMany (reverse side)
• OneToOne (owner side)
• OneToOne (reverse side

[kbond]

That's inline with what I was thinking except, instead of having to manually set, detect from the mapping.

Did the above work for your use case?

[mpiot]

Yep, this solve my problem, and do not break others.

I work on a way to enable it on owner side to, then do a PR for it.