New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[feature] allow creating "rich domain models" #312
base: 1.x
Are you sure you want to change the base?
Conversation
@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::createOne([
'comments' => CommentFactory::new()->many(1),
])->object();
$post->getComments()->isEmpty(); // currently true, the expected result is false |
Hi @X-Coder264
we would like to try another way of doing this: |
Hey @X-Coder264, I'd like finish this one up shortly, just getting back from holidays. |
642bb6c
to
a5222e3
Compare
a5222e3
to
da05b92
Compare
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:
But these can be added in the future (probably when we split the instantiator). |
da05b92
to
e730719
Compare
very strange 🤔 |
would you mind rebasing on 1.x? |
e730719
to
a2175aa
Compare
@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 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; |
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). |
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. |
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). |
a2175aa
to
0bbf694
Compare
/** | ||
* @test | ||
*/ | ||
public function can_create_factories_for_rich_domain_models(): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test passes and adds the comments via RichPost::setComment()
(property-accessor).
/** | ||
* @test | ||
*/ | ||
public function can_create_factories_for_rich_domain_models_using_force(): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test passes and adds the comments by forcing RichPost::$comments
.
/** | ||
* @test | ||
*/ | ||
public function can_create_factories_for_cascade_rich_domain_models(): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fails due to the cascade persist on CascadePost::$comments
.
/** | ||
* @test | ||
*/ | ||
public function can_create_factories_for_cascade_rich_domain_models_using_force(): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fails due to the cascade persist on CascadePost::$comments
.
I now test using 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... |
I think when we split the instantiator into an instantiator and a hydrator we might be able to make
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. |
I think we really should have this work with |
Agreed, I'm hoping #395 will help solve. |
@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 I'm considering to add a custom PHPStan rule since it's been multiple times I've lost time on this. |
Possible solution for #305. When forcing properties with the
Instantiator
, covertarray
's toArrayCollection
's if the typehint demands it.Not a huge fan of this solution... I think I'd prefer
FactoryCollection
generate anArrayCollection
and then it can be set directly. I'll explore this option.