Skip to content

fix: flush only once per calls to Factory::create() in userland #836

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

Closed
wants to merge 3 commits into from
Closed

fix: flush only once per calls to Factory::create() in userland #836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
28715d1
fix: flush only once per Factory::create() calls in userland
nikophil Mar 2, 2025
a4d38a8
fix: only call om::persist() once all objects are created
nikophil Mar 6, 2025
0fbfdee
fix: introduce so-called transactions
nikophil Mar 24, 2025
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
4 changes: 2 additions & 2 deletions src/Factory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ protected function normalizeParameter(string $field, mixed $value): mixed
);
}

return \is_object($value) ? $this->normalizeObject($value) : $value;
return \is_object($value) ? $this->normalizeObject($field, $value) : $value;
}

/**
Expand All @@ -253,7 +253,7 @@ protected function normalizeCollection(string $field, FactoryCollection $collect
/**
* @internal
*/
protected function normalizeObject(object $object): object
protected function normalizeObject(string $field, object $object): object
{
return $object;
}
Expand Down
48 changes: 20 additions & 28 deletions src/ORM/OrmV2PersistenceStrategy.php
View file
Open in desktop
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 just cleaned a little bit how we get "inversedRelationshipMetadata", hence these changes - I needed to have more information (we did only get "one to many" and "inversed one to one" but I now needed also "normal one to one"

Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
use Doctrine\ORM\Mapping\ClassMetadataInfo;
use Doctrine\ORM\Mapping\MappingException as ORMMappingException;
use Doctrine\Persistence\Mapping\MappingException;
use Zenstruck\Foundry\Persistence\InverseRelationshipMetadata;
use Zenstruck\Foundry\Persistence\Relationship\OneToManyRelationship;
use Zenstruck\Foundry\Persistence\Relationship\OneToOneRelationship;
use Zenstruck\Foundry\Persistence\Relationship\RelationshipMetadata;

/**
* @internal
Expand All @@ -25,49 +27,39 @@
*/
final class OrmV2PersistenceStrategy extends AbstractORMPersistenceStrategy
{
public function inversedRelationshipMetadata(string $parent, string $child, string $field): ?InverseRelationshipMetadata
public function inversedRelationshipMetadata(string $parent, string $child, string $field): ?RelationshipMetadata
{
$metadata = $this->classMetadata($child);
$associationMapping = $this->getAssociationMapping($parent, $child, $field);

$inversedAssociation = $this->getAssociationMapping($parent, $child, $field);

if (null === $inversedAssociation || !$metadata instanceof ClassMetadataInfo) {
if (null === $associationMapping) {
return null;
}

if (!\is_a(
$child,
$inversedAssociation['targetEntity'],
$associationMapping['targetEntity'],
allow_string: true
)) { // is_a() handles inheritance as well
throw new \LogicException("Cannot find correct association named \"{$field}\" between classes [parent: \"{$parent}\", child: \"{$child}\"]");
}

// exclude "owning" side of the association (owning OneToOne or ManyToOne)
if (!\in_array(
$inversedAssociation['type'],
[ClassMetadataInfo::ONE_TO_MANY, ClassMetadataInfo::ONE_TO_ONE],
true
)
|| !isset($inversedAssociation['mappedBy'])
) {
return null;
}
$inverseField = $associationMapping['isOwningSide'] ? $associationMapping['inversedBy'] ?? null : $associationMapping['mappedBy'] ?? null;

$association = $metadata->getAssociationMapping($inversedAssociation['mappedBy']);

// only keep *ToOne associations
if (!$metadata->isSingleValuedAssociation($association['fieldName'])) {
if (null === $inverseField) {
return null;
}

$inversedAssociationMetadata = $this->classMetadata($inversedAssociation['sourceEntity']);

return new InverseRelationshipMetadata(
inverseField: $association['fieldName'],
isCollection: $inversedAssociationMetadata->isCollectionValuedAssociation($inversedAssociation['fieldName']),
collectionIndexedBy: $inversedAssociation['indexBy'] ?? null
);
return match (true) {
ClassMetadataInfo::ONE_TO_MANY === $associationMapping['type'] => new OneToManyRelationship(
inverseField: $inverseField,
collectionIndexedBy: $associationMapping['indexBy'] ?? null
),
ClassMetadataInfo::ONE_TO_ONE === $associationMapping['type'] => new OneToOneRelationship(
inverseField: $inverseField,
isOwning: $associationMapping['isOwningSide'] ?? false
),
default => null,
};
}

/**
Expand Down
45 changes: 22 additions & 23 deletions src/ORM/OrmV3PersistenceStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,50 +14,49 @@
namespace Zenstruck\Foundry\ORM;

use Doctrine\ORM\Mapping\AssociationMapping;
use Doctrine\ORM\Mapping\ClassMetadata;
use Doctrine\ORM\Mapping\InverseSideMapping;
use Doctrine\ORM\Mapping\MappingException as ORMMappingException;
use Doctrine\ORM\Mapping\ToManyAssociationMapping;
use Doctrine\ORM\Mapping\OneToManyAssociationMapping;
use Doctrine\ORM\Mapping\OneToOneAssociationMapping;
use Doctrine\Persistence\Mapping\MappingException;
use Zenstruck\Foundry\Persistence\InverseRelationshipMetadata;
use Zenstruck\Foundry\Persistence\Relationship\OneToManyRelationship;
use Zenstruck\Foundry\Persistence\Relationship\OneToOneRelationship;
use Zenstruck\Foundry\Persistence\Relationship\RelationshipMetadata;

final class OrmV3PersistenceStrategy extends AbstractORMPersistenceStrategy
{
public function inversedRelationshipMetadata(string $parent, string $child, string $field): ?InverseRelationshipMetadata
public function inversedRelationshipMetadata(string $parent, string $child, string $field): ?RelationshipMetadata
{
$metadata = $this->classMetadata($child);
$associationMapping = $this->getAssociationMapping($parent, $child, $field);

$inversedAssociation = $this->getAssociationMapping($parent, $child, $field);

if (null === $inversedAssociation || !$metadata instanceof ClassMetadata) {
if (null === $associationMapping) {
return null;
}

if (!\is_a(
$child,
$inversedAssociation->targetEntity,
$associationMapping->targetEntity,
allow_string: true
)) { // is_a() handles inheritance as well
throw new \LogicException("Cannot find correct association named \"{$field}\" between classes [parent: \"{$parent}\", child: \"{$child}\"]");
}

// exclude "owning" side of the association (owning OneToOne or ManyToOne)
if (!$inversedAssociation instanceof InverseSideMapping) {
return null;
}

$association = $metadata->getAssociationMapping($inversedAssociation->mappedBy);
$inverseField = $associationMapping->isOwningSide() ? $associationMapping->inversedBy : $associationMapping->mappedBy;

// only keep *ToOne associations
if (!$metadata->isSingleValuedAssociation($association->fieldName)) {
if (null === $inverseField) {
return null;
}

return new InverseRelationshipMetadata(
inverseField: $association->fieldName,
isCollection: $inversedAssociation instanceof ToManyAssociationMapping,
collectionIndexedBy: $inversedAssociation->isIndexed() ? $inversedAssociation->indexBy() : null
);
return match (true) {
$associationMapping instanceof OneToManyAssociationMapping => new OneToManyRelationship(
inverseField: $inverseField,
collectionIndexedBy: $associationMapping->isIndexed() ? $associationMapping->indexBy() : null
),
$associationMapping instanceof OneToOneAssociationMapping => new OneToOneRelationship(
inverseField: $inverseField,
isOwning: $associationMapping->isOwningSide()
),
default => null,
};
}

/**
Expand Down
104 changes: 77 additions & 27 deletions src/Persistence/PersistenceManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
use Zenstruck\Foundry\ORM\AbstractORMPersistenceStrategy;
use Zenstruck\Foundry\Persistence\Exception\NoPersistenceStrategy;
use Zenstruck\Foundry\Persistence\Exception\RefreshObjectFailed;
use Zenstruck\Foundry\Persistence\Relationship\RelationshipMetadata;
use Zenstruck\Foundry\Persistence\ResetDatabase\ResetDatabaseManager;

/**
Expand All @@ -31,9 +32,14 @@ final class PersistenceManager
private bool $flush = true;
private bool $persist = true;

/** @var list<object> */
private array $objectsToPersist = [];

/** @var list<callable():void> */
private array $afterPersistCallbacks = [];

private bool $transactionStarted = false;

/**
* @param iterable<PersistenceStrategy> $strategies
*/
Expand Down Expand Up @@ -75,18 +81,48 @@ public function save(object $object): object
$om->persist($object);
$this->flush($om);

if ($this->afterPersistCallbacks) {
$afterPersistCallbacks = $this->afterPersistCallbacks;
$this->afterPersistCallbacks = [];
return $object;
}

foreach ($afterPersistCallbacks as $afterPersistCallback) {
$afterPersistCallback();
/**
* We're using so-called "transactions" to group multiple persist/flush operations
* This prevents such code to persist the whole batch of objects in the normalization phase:
* ```php
* SomeFactory::createOne(['item' => lazy(fn() => OtherFactory::createOne())]);
* ```.
*/
public function startTransaction(): void
{
$this->transactionStarted = true;
}

public function isTransactionStarted(): bool
{
return $this->transactionStarted;
}

public function commit(): void
{
$objectManagers = [];

$objectsToPersist = $this->objectsToPersist;
$this->objectsToPersist = [];
$this->transactionStarted = false;

foreach ($objectsToPersist as $object) {
$om = $this->strategyFor($object::class)->objectManagerFor($object::class);
$om->persist($object);

if (!\in_array($om, $objectManagers, true)) {
$objectManagers[] = $om;
}
}

$this->save($object);
foreach ($objectManagers as $om) {
$this->flush($om);
}

return $object;
$this->callPostPersistCallbacks();
}

/**
Expand All @@ -103,25 +139,16 @@ public function scheduleForInsert(object $object, array $afterPersistCallbacks =
$object = unproxy($object);
}

$om = $this->strategyFor($object::class)->objectManagerFor($object::class);
$om->persist($object);
$this->objectsToPersist[] = $object;

$this->afterPersistCallbacks = [...$this->afterPersistCallbacks, ...$afterPersistCallbacks];
$this->afterPersistCallbacks = [
...$this->afterPersistCallbacks,
...$afterPersistCallbacks,
];

return $object;
}

public function forget(object $object): void
{
if ($this->isPersisted($object)) {
throw new \LogicException('Cannot forget an object already persisted.');
}

$om = $this->strategyFor($object::class)->objectManagerFor($object::class);

$om->detach($object);
}

/**
* @template T
*
Expand All @@ -137,11 +164,9 @@ public function flushAfter(callable $callback): mixed

$this->flush = true;

foreach ($this->strategies as $strategy) {
foreach ($strategy->objectManagers() as $om) {
$this->flush($om);
}
}
$this->flushAllStrategies();

$this->callPostPersistCallbacks();

return $result;
}
Expand Down Expand Up @@ -280,7 +305,7 @@ public function repositoryFor(string $class): ObjectRepository
* @param class-string $parent
* @param class-string $child
*/
public function inverseRelationshipMetadata(string $parent, string $child, string $field): ?InverseRelationshipMetadata
public function inverseRelationshipMetadata(string $parent, string $child, string $field): ?RelationshipMetadata
{
$parent = unproxy($parent);
$child = unproxy($child);
Expand Down Expand Up @@ -372,6 +397,31 @@ public static function isOrmOnly(): bool
})();
}

private function flushAllStrategies(): void
{
foreach ($this->strategies as $strategy) {
foreach ($strategy->objectManagers() as $om) {
$this->flush($om);
}
}
}

private function callPostPersistCallbacks(): void
{
if (!$this->flush || [] === $this->afterPersistCallbacks) {
return;
}

$afterPersistCallbacks = $this->afterPersistCallbacks;
$this->afterPersistCallbacks = [];

foreach ($afterPersistCallbacks as $afterPersistCallback) {
$afterPersistCallback();
}

$this->flushAllStrategies();
}

/**
* @param class-string $class
*
Expand Down
3 changes: 2 additions & 1 deletion src/Persistence/PersistenceStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use Doctrine\Persistence\Mapping\ClassMetadata;
use Doctrine\Persistence\Mapping\MappingException;
use Doctrine\Persistence\ObjectManager;
use Zenstruck\Foundry\Persistence\Relationship\RelationshipMetadata;

/**
* @author Kevin Bond <[email protected]>
Expand Down Expand Up @@ -63,7 +64,7 @@ public function objectManagers(): array
* @param class-string $parent
* @param class-string $child
*/
public function inversedRelationshipMetadata(string $parent, string $child, string $field): ?InverseRelationshipMetadata
public function inversedRelationshipMetadata(string $parent, string $child, string $field): ?RelationshipMetadata
{
return null;
}
Expand Down
Loading