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.

Sign up for GitHub

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

Jump to bottom

fix: can create inversed one to one #659

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

fix: can create inversed one to one #659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/ORM/OrmV2PersistenceStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

);
}

Expand Down
4 changes: 3 additions & 1 deletion src/ORM/OrmV3PersistenceStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use Doctrine\ORM\Mapping\ClassMetadata;
use Doctrine\ORM\Mapping\InverseSideMapping;
use Doctrine\ORM\Mapping\MappingException as ORMMappingException;
use Doctrine\ORM\Mapping\ToManyAssociationMapping;
use Doctrine\Persistence\Mapping\MappingException;
use Zenstruck\Foundry\Persistence\RelationshipMetadata;

Expand Down Expand Up @@ -47,8 +48,9 @@ public function relationshipMetadata(string $parent, string $child, string $fiel
}

return new RelationshipMetadata(
isCascadePersist: $association->isCascadePersist(),
isCascadePersist: ($inversedAssociation ?? $association)->isCascadePersist(),
inverseField: $metadata->isSingleValuedAssociation($association->fieldName) ? $association->fieldName : null,
isCollection: ($inversedAssociation ?? $association) instanceof ToManyAssociationMapping
Comment on lines +51 to +53
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"

);
}

Expand Down
21 changes: 19 additions & 2 deletions src/Persistence/PersistentObjectFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,8 +275,25 @@ protected function normalizeParameter(string $field, mixed $value): mixed
$value->persist = $this->persist; // todo - breaks immutability
}

if ($value instanceof self && Configuration::instance()->persistence()->relationshipMetadata(static::class(), $value::class(), $field)?->isCascadePersist) {
$value->persist = false;
if ($value instanceof self) {
$pm = Configuration::instance()->persistence();

$relationshipMetadata = $pm->relationshipMetadata($value::class(), static::class(), $field);

// handle inversed OneToOne
if ($relationshipMetadata && !$relationshipMetadata->isCollection && $inverseField = $relationshipMetadata->inverseField) {
$this->tempAfterPersist[] = static function(object $object) use ($value, $inverseField, $pm) {
$value->create([$inverseField => $object]);
$pm->refresh($object);
};

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

@nikophil nikophil Jun 28, 2024 •
edited
Loading

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 •
edited
Loading

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.

}

if (Configuration::instance()->persistence()->relationshipMetadata(static::class(), $value::class(), $field)?->isCascadePersist) {
$value->persist = false;
}
}

return unproxy(parent::normalizeParameter($field, $value));
Expand Down
1 change: 1 addition & 0 deletions src/Persistence/RelationshipMetadata.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ final class RelationshipMetadata
public function __construct(
public readonly bool $isCascadePersist,
public readonly ?string $inverseField,
public readonly bool $isCollection,
) {
}
}
12 changes: 12 additions & 0 deletions tests/Fixture/Entity/Address.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
#[ORM\MappedSuperclass]
abstract class Address extends Base
{
protected Contact|null $contact = null;

#[ORM\Column(length: 255)]
private string $city;

Expand All @@ -28,6 +30,16 @@ public function __construct(string $city)
$this->city = $city;
}

public function getContact(): Contact|null
{
return $this->contact;
}

public function setContact(Contact|null $contact): void
{
$this->contact = $contact;
}

public function getCity(): string
{
return $this->city;
Expand Down
4 changes: 4 additions & 0 deletions tests/Fixture/Entity/Address/CascadeAddress.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,15 @@

use Doctrine\ORM\Mapping as ORM;
use Zenstruck\Foundry\Tests\Fixture\Entity\Address;
use Zenstruck\Foundry\Tests\Fixture\Entity\Contact;
use Zenstruck\Foundry\Tests\Fixture\Entity\Contact\CascadeContact;

/**
* @author Kevin Bond <[email protected]>
*/
#[ORM\Entity]
class CascadeAddress extends Address
{
#[ORM\OneToOne(targetEntity: CascadeContact::class, mappedBy: 'address', cascade: ['persist', 'remove'])]
protected Contact|null $contact = null;
}
4 changes: 4 additions & 0 deletions tests/Fixture/Entity/Address/StandardAddress.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,15 @@

use Doctrine\ORM\Mapping as ORM;
use Zenstruck\Foundry\Tests\Fixture\Entity\Address;
use Zenstruck\Foundry\Tests\Fixture\Entity\Contact;
use Zenstruck\Foundry\Tests\Fixture\Entity\Contact\StandardContact;

/**
* @author Kevin Bond <[email protected]>
*/
#[ORM\Entity]
class StandardAddress extends Address
{
#[ORM\OneToOne(targetEntity: StandardContact::class, mappedBy: 'address')]
protected Contact|null $contact = null;
}
2 changes: 1 addition & 1 deletion tests/Fixture/Entity/Contact/CascadeContact.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class CascadeContact extends Contact
#[ORM\JoinTable(name: 'category_tag_cascade_secondary')]
protected Collection $secondaryTags;

#[ORM\OneToOne(targetEntity: CascadeAddress::class, cascade: ['persist', 'remove'])]
#[ORM\OneToOne(targetEntity: CascadeAddress::class, inversedBy: 'contact', cascade: ['persist', 'remove'])]
#[ORM\JoinColumn(nullable: false)]
protected Address $address;
}
16 changes: 16 additions & 0 deletions tests/Integration/ORM/EntityFactoryRelationshipTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,22 @@ public function inverse_many_to_many_with_two_relationships_same_entity(): void
$this->contactFactory()::assert()->count(6);
}

/**
* @test
*/
public function inversed_one_to_one(): void
{
$addressFactory = $this->addressFactory();
$contactFactory = $this->contactFactory();

$address = $addressFactory->create(['contact' => $contactFactory]);

self::assertNotNull($address->getContact());

$addressFactory::assert()->count(1);
$contactFactory::assert()->count(1);
}

/**
* @test
*/
Expand Down
Loading