Skip to content
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

fix: can create inversed one to one #659

Draft
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

nikophil
Copy link
Member

@nikophil nikophil commented Jun 28, 2024

fixes #655

Comment on lines +51 to +53
isCascadePersist: ($inversedAssociation ?? $association)->isCascadePersist(),
inverseField: $metadata->isSingleValuedAssociation($association->fieldName) ? $association->fieldName : null,
isCollection: ($inversedAssociation ?? $association) instanceof ToManyAssociationMapping
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about those... but we want to know if the "inversed" association is a collection here, if it exsts. It's the only way to know that we are handling an "inverse one to one"

};

// creation delegated to afterPersist hook - return empty array here
return null;
Copy link
Member Author

@nikophil nikophil Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it is OK to return null here.
Can a "OneToOne" relationship be "not nullable" on the both sides of the relationship?

EDIT: I think it can:

#[Entity]
class Contact
{
    #[ORM\OneToOne(targetEntity: Address::class, inversedBy: 'contact')]
    #[ORM\JoinColumn(nullable: false)]
    public Address $address;

    public function __construct()
    {
        $this->address = new Address($this);
    }
}

#[Entity]
class Address
{
    public function __construct(
	    #[ORM\OneToOne(targetEntity: Contact::class, mappedBy: 'address')]
	    public Contact $contact;
    ) {}
}

and then, it would not be valid to return null here 🤷

Copy link
Member Author

@nikophil nikophil Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this very specific case, where both side of OneToOne are mandatory, could not be fixed : it is a circular dependency. Maybe we could find a way to emit a meaningful error for the user?

Also, maybe we could find a way to return something else than null here: in Foundry2, we used to return a PostPersistCallback which was not added to the normalized parameters. Maybe we could mimic this (but it would complexify the whole normalization phase)

BTW, I just figured out that you did notice this case was error prone: https://github.com/zenstruck/foundry/blob/2.x/tests/Integration/ORM/EntityFactoryRelationshipTestCase.php#L183

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I don't remember the exact context of what happened but I think you're right, it's the circular dependency issue.

@nikophil nikophil requested a review from kbond June 28, 2024 11:58
@@ -52,6 +52,7 @@ public function relationshipMetadata(string $parent, string $child, string $fiel
return new RelationshipMetadata(
isCascadePersist: $association['isCascadePersist'],
inverseField: $metadata->isSingleValuedAssociation($association['fieldName']) ? $association['fieldName'] : null,
isCollection: $metadata->isCollectionValuedAssociation($association['fieldName']),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to fix this, to mimic how it is done with orm v3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[2.0] Problem with inversed OneToOne relationships
2 participants