[feature] allow creating "rich domain models"

[kbond]

trafficstars

Possible solution for #305. When forcing properties with the Instantiator, covert array's to ArrayCollection's if the typehint demands it.

Not a huge fan of this solution... I think I'd prefer FactoryCollection generate an ArrayCollection and then it can be set directly. I'll explore this option.

[X-Coder264]

@kbond Is there anything left to be done before this can be merged?

We are currently trying to migrate to Foundry but this is blocking us.

As CommentFactory in our case has 'post' => PostFactory::new(), in its getDefaults method (so that the CommentFactory::createOne(); call can work) the piece of code below creates two post records and one comment record in the database (instead of the expected one post record with one comment). Plus the created comment record gets created for the other post, not the one that actually gets returned by the post factory call.

        $post = PostFactory::createOne([
            'comments' => CommentFactory::new()->many(1),
        ])->object();

       $post->getComments()->isEmpty(); // currently true, the expected result is false

[nikophil]

Hi @X-Coder264

Is there anything left to be done before this can be merged?

we would like to try another way of doing this: FactoryCollection::create() could return directly an ArrayCollection. We could give a try in the next few days if this feature is missing for someone

[kbond]

Hey @X-Coder264, I'd like finish this one up shortly, just getting back from holidays.

[kbond]

I think this is ready as-is. @X-Coder264, can you try this out in your project?

There's some things I'd still like to do:

1. Split Instantiator into separate instantiator/hydrator (#388)
2. Use symfony/property-info to detect doctrine collections (right now the prop needs to be type-hinted as Collection).
3. Add FactoryCollection::asDoctrineCollection() or something like that.

But these can be added in the future (probably when we split the instantiator).

[kbond]

@nikophil, any idea on the CI failure? Related to #393 somehow?

[nikophil]

very strange :thinking: I already had these errors in #393 but they were fixed by adding a condition on the migrations related to PHP 8.1 https://github.com/zenstruck/foundry/blob/1.x/tests/Fixtures/Migrations/Version20230109134337.php#L19

[nikophil]

would you mind rebasing on 1.x?

[kbond]

would you mind rebasing on 1.x?

Done but still a problem. I see the failures from #393 that had the same issue, was this the solution for that one?

[X-Coder264]

@kbond I've tried it out and for some reason I'm still getting the same old/wrong behavior even though I can see that the new $value = new ArrayCollection($value); code path gets successfully triggered when I put a Xdebug breakpoint there. I can't see any meaningful difference between your test case and my code (except that I use cascade persist, orphan removal and fetch extra lazy on my mapping but none of those should make a difference) :thinking:

EDIT: It does make a difference.

This works (same scenario as your test case):

    /**
     * @ORM\OneToMany(targetEntity="Comment", mappedBy="post")
     */
    private Collection $comments;

    /**
     * @ORM\ManyToOne(targetEntity="Post", inversedBy="comments")
     * @ORM\JoinColumn(nullable=false)
     */
    private Post $post;

This doesn't work as expected:

    /**
     * @ORM\OneToMany(targetEntity="Comment", mappedBy="post", cascade={"persist"}, orphanRemoval=true, fetch="EXTRA_LAZY")
     */
    private Collection $comments;

    /**
     * @ORM\ManyToOne(targetEntity="Post", inversedBy="comments", cascade={"persist"})
     * @ORM\JoinColumn(nullable=false)
     */
    private Post $post;

[X-Coder264]

Ok, found the issue. Removing the cascade persist "fixes" the issue. So if I change my mapping to this it works as expected:

    /**
     * @ORM\OneToMany(targetEntity="Comment", mappedBy="post", orphanRemoval=true, fetch="EXTRA_LAZY")
     */
    private Collection $comments;

I'm not sure why does cascade persist change anything, the behavior with or without cascade persist should be the same (getting back only one post with X comments).

[kbond]

Ok, thanks for testing and finding this edge case. I'll create a test case that uses cascade: persist and see if I can figure out why it doesn't work as expected.

There are some nuances with cascade persist that I don't fully understand (as I never use this feature myself). These are accounted for elsewhere in foundry but I didn't write this code. Maybe something with one of these is causing the problem.

[kbond]

I found the issue but not sure yet how to solve:

When NOT using cascade persist, the instantiation of the comment collection is done after persisting the post. The comments are instantiated with the previously created post (this overrides the default post so the extra post per comment aren't created).

When using cascade persist, the collection needs to be instantiated before creating the post so I don't have a chance to set the post (as it isn't instantiated yet).

[kbond]

I now test using rich entities with and without alwaysForceProperties() and this all works as expected.

The same tests using cascade persist do not pass. Still not sure how to solve. We're kind of in a chicken-egg situation: to create the comment, we need the post but to create the post, we need the comment...

[kbond]

I think when we split the instantiator into an instantiator and a hydrator we might be able to make cascade: persist work here.

We're kind of in a chicken-egg situation: to create the comment, we need the post but to create the post, we need the comment...

We could instantiate the post (w/o hydrating), then instantiate and hydrate the comments (using the post object), then hydrate the post with the created comments. Complicated but doable I think.

[nikophil]

I think we really should have this work with cascade={"persist"} since it is really common to have this

[kbond]

I think we really should have this work with cascade={"persist"} since it is really common to have this

Agreed, I'm hoping #395 will help solve.

[nikophil]

@kbond I've found why some tests not related to this PR are failing. Could you add this code to all migration which does not have it?

    public function isTransactional(): bool
    {
        return false;
    }

Migrations are only tested without DAMA, then they are only tested for "php8.0/sf4.4/lowest" and "php8.2/sf6.2/highest" but I don't understand why it does not fail in the second scenario... maybe doctrine/migration is clever enough to know transaction mode should be disabled in certain cases?

I'm considering to add a custom PHPStan rule since it's been multiple times I've lost time on this.

[nikophil]

hey @kbond could we close this PR now, since we'e merged https://github.com/zenstruck/foundry/pull/645 ?