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

Resolved a bug where a soft-deleted object isn't remove from the ObjectManager #2930

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
5 changes: 4 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ a release.

## [Unreleased]

### Fixed
- SoftDeleteable: Resolved a bug where a soft-deleted object isn't remove from the ObjectManager (#2930)

## [3.19.0] - 2025-02-24
### Added
- Actor provider for use with extensions with user references (#2914)
Expand Down Expand Up @@ -103,7 +106,7 @@ a release.
- Dropped support for doctrine/dbal < 3.2

### Deprecated
- Calling `Gedmo\Mapping\Event\Adapter\ORM::getObjectManager()` and `getObject()` on EventArgs that do not implement `getObjectManager()` and `getObject()` (such as old EventArgs implementing `getEntityManager()` and `getEntity()`)
- Calling `Gedmo\Mapping\Event\Adapter\ORM::getObjectManager()` and `getObject()` on EventArgs that do not implement `getObjectManager()` and `getObject()` (such as old EventArgs implementing `getEntityManager()` and `getEntity()`)
- Calling `Gedmo\Uploadable\Event\UploadableBaseEventArgs::getEntityManager()` and `getEntity()`. Call `getObjectManager()` and `getObject()` instead.

## [3.13.0] - 2023-09-06
Expand Down
27 changes: 26 additions & 1 deletion src/SoftDeleteable/SoftDeleteableListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,13 @@ class SoftDeleteableListener extends MappedEventSubscriber
*/
public const POST_SOFT_DELETE = 'postSoftDelete';

/**
* Objects soft-deleted on flush.
*
* @var array<object>
*/
private array $softDeletedObjects = [];

/**
* @return string[]
*/
Expand All @@ -59,6 +66,7 @@ public function getSubscribedEvents()
return [
'loadClassMetadata',
'onFlush',
'postFlush',
];
}

Expand Down Expand Up @@ -102,7 +110,7 @@ public function onFlush(EventArgs $args)

$evm->dispatchEvent(
self::PRE_SOFT_DELETE,
$preSoftDeleteEventArgs
$preSoftDeleteEventArgs,
);
}

Expand All @@ -129,10 +137,27 @@ public function onFlush(EventArgs $args)
$postSoftDeleteEventArgs
);
}

$this->softDeletedObjects[] = $object;
Copy link
Collaborator

@phansys phansys Mar 9, 2025

Choose a reason for hiding this comment

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

More than a suggestion, this is a question or a request for analysis.

Given storing the list of deleted objects in this array could lead to high memory consumption, do you evaluated a way to mitigate this by using the object references (like the ones produced by spl_object_id(), spl_object_hash(), or a combination of the model class and its id)?

I don' t know if this can produce better results, but I think this is a consideration we should take into account.

Copy link
Author

Choose a reason for hiding this comment

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

Being that these objects are unset (cleared from memory) postFlush, I really don't see it as much of a concern - certainly not enough to introduce spl_object_id complexity and the potential to expose bugs through that.

}
}
}

/**
* Detach soft-deleted objects from object manager.
*
* @return void
*/
public function postFlush(EventArgs $args)
{
$ea = $this->getEventAdapter($args);
$om = $ea->getObjectManager();
foreach ($this->softDeletedObjects as $index => $object) {
$om->detach($object);
unset($this->softDeletedObjects[$index]);
}
}

/**
* Maps additional metadata
*
Expand Down
2 changes: 1 addition & 1 deletion tests/Gedmo/Blameable/Fixture/Entity/Article.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class Article implements Blameable
* @ORM\OneToMany(targetEntity="Gedmo\Tests\Blameable\Fixture\Entity\Comment", mappedBy="article")
*/
#[ORM\OneToMany(targetEntity: Comment::class, mappedBy: 'article')]
private $comments;
private Collection $comments;

/**
* @Gedmo\Blameable(on="create")
Expand Down
8 changes: 8 additions & 0 deletions tests/Gedmo/SoftDeleteable/Fixture/Entity/Article.php
Original file line number Diff line number Diff line change
Expand Up @@ -92,4 +92,12 @@ public function addComment(Comment $comment): void
{
$this->comments[] = $comment;
}

/**
* @return Collection<int, Comment>
*/
public function getComments(): Collection
{
return $this->comments;
}
}
36 changes: 36 additions & 0 deletions tests/Gedmo/SoftDeleteable/SoftDeleteableEntityTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,42 @@ public function testShouldFilterBeQueryCachedCorrectlyWhenToggledForEntity(): vo
static::assertCount(0, $data);
}

public function testSoftDeletedObjectIsRemovedPostFlush(): void
{
$repo = $this->em->getRepository(Article::class);
$commentRepo = $this->em->getRepository(Comment::class);

$comment = new Comment();
$commentValue = 'Comment 1';
$comment->setComment($commentValue);

$art0 = new Article();
$field = 'title';
$value = 'Title 1';
$art0->setTitle($value);
$art0->addComment($comment);

$this->em->persist($art0);
$this->em->flush();

$art = $repo->findOneBy([$field => $value]);

static::assertNull($art->getDeletedAt());
static::assertNull($comment->getDeletedAt());
static::assertCount(1, $art->getComments());

$this->em->remove($comment);

// The Comment has been marked for removal, but not yet flushed. This means the
// Comment should still be available.
static::assertInstanceOf(Comment::class, $commentRepo->find($comment->getId()));

$this->em->flush();

// Now that we've flushed, the Comment should no longer be available and should return null
static::assertNull($commentRepo->find($comment->getId()));
}

public function testPostSoftDeleteEventIsDispatched(): void
{
$this->em->getEventManager()->addEventSubscriber(new WithPreAndPostSoftDeleteEventArgsTypeListener());
Expand Down