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

[oojacoboo]

Currently, after you remove an entity, it's still queryable by the pk, as it's cached in the ObjectManager. This PR fixes this bug by removing it postFlush. See test for more details.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.53%. Comparing base (8264aad) to head (1fe0d03).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2930      +/-   ##
==========================================
+ Coverage   78.51%   78.53%   +0.01%     
==========================================
  Files         168      168              
  Lines        8782     8790       +8     
==========================================
+ Hits         6895     6903       +8     
  Misses       1887     1887              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[oojacoboo]

@phansys it'd be nice if the tests could be run on new commits. Also, I believe the CHANGELOG will need to be something you commit, as I don't have the target version.

[phansys]

@phansys it'd be nice if the tests could be run on new commits.

Tests are running.

Also, I believe the CHANGELOG will need to be something you commit, as I don't have the target version.

Please, read the Changelog section at CONTRIBUTING.md.

[phansys]

@mbabker, could you please provide your feedback here?

Thanks.

[mbabker]

I don't use soft deletes in my Doctrine based projects so at face value this seems fine to me.

What's the current behavior if you do a $em->getRepository(SoftDeleteable::class)->find($pk); when the soft-deleted record hasn't been previously loaded? I'm not 100% sure where the query filter is supposed to kick in but if calling find() when a soft-deleted record has never been loaded into the object manager is supposed to return null, then this would definitely fix the bug of it returning a soft-deleted record because the object manager is aware of it.

[oojacoboo]

@mbabker You're correct in your assumption. If it's not already cached in the ObjectManager, it would return null. This fixes a bug around the cache. We've been using it in production for years on a fork of this lib. I'm trying to get this lib caught up with all the patches we've made over the years, now that the ActorProvider implementation is in place.

[phansys]

@oojacoboo, could you please rebase this PR and fix the conflicts?

[oojacoboo]

The PR says there aren't any conflicts with the base branch.

[phansys]

The PR says there aren't any conflicts with the base branch.

This is what I get from the GitHub UI:

[oojacoboo]

Why would you be rebasing it and not merging? You can squash and merge.

[phansys]

Thank you @oojacoboo!

[phansys]

Why would you be rebasing it and not merging? You can squash and merge.

Good point. Squash was working with no conflicts.
Thanks!

[rotdrop]

Currently, after you remove an entity, it's still queryable by the pk, as it's cached in the ObjectManager. This PR fixes this bug by removing it postFlush. See test for more details.

One question: what about the soft-deleteable filter? Should the object be removed from the ObjectManager if the soft-deleteable filter is disabled? Shouldn't it stay in the cache in that case?

[oojacoboo]

@rotdrop if that filter is disabled... that'd be a real database delete, no? Why would it remain in theObjectManager if you deleted it? If you need access to values post-delete, you should assign them to variables prior.

[rotdrop]

@rotdrop if that filter is disabled... that'd be a real database delete, no? Why would it remain in theObjectManager if you deleted it? If you need access to values post-delete, you should assign them to variables prior.

No, I think you misunderstood my point. The "soft-deleteable" filter is the filter which actually enables filtering out soft-deleted entities. If that filter is disabled (and it is disabled by default) then select statements will just retrieve entities as usual. Only if the soft-deleteable entity filter is enabled then soft-deleted entities are omitted from the result set.

So by "the filter" I do not mean the SoftDeleteable extension, but the soft-deleteable entity filter, please compare with the documentation here:

DoctrineExtensions/doc/softdeleteable.md

Line 32 in ea1d375

With SoftDeleteable there's one more step you need to do. You need to add the filter to your configuration:

Hence: without the filter being enabled the entities show IMHO stay in the cache.

[rotdrop]

@rotdrop if that filter is disabled... that'd be a real database delete, no? Why would it remain in theObjectManager if you deleted it? If you need access to values post-delete, you should assign them to variables prior.

@oojacoboo Perhaps this PR #2939 clarifies what I am talking about.

[oojacoboo]

@rotdrop I see what you mean now - regarding the filter and being enabled/disabled.

Honestly, I don't see the difference. I get where you're coming from and I see your logic, but I think it's more of a personal use case, or a flawed way of looking at it.

Deleting an entity, regardless of whether soft-delete is enabled/disabled or even setup in the first place, should remove the object from the ObjectManager. You're deleting it... that's an explicit intention. Leaving it in the ObjectManager is a flawed side-effect.

That said, if you have the soft-deleteable filter disabled, your subsequent call will just fetch it again. I do not think that's a reason to leave it in the ObjectManager though. The filter is a query filter, meaning it's purpose is to affect the results of a "fetch"/query on the database, and the results of that. It's not intended to alter your state or change the behavior of state.

[rotdrop]

@oojacoboo I do not think that the scenario is that clear. Also, if we talk about state, explicit intent and that calling remove should remove the entity from the object manager: this still does not happen until you call flush(). However, flush() is meant to sync the state of the object manager with the database, flush() is not a cache garbage collector.

Also, this entire SoftDeleteable extension already breaks "explicit intent", actually, the whole purpose of this plugin is to alter the meaning of remove(). So, I am not convinced that the way this PR #2930 handles things is desirable.

[oojacoboo]

flush should also be seen as an abstraction for the EntityManager, to flush/update state. The EntityManager is abstracted from the DBAL layer already.

I won't argue that SoftDeletable is a broken antipattern. That's been well discussed, and I'm not going to disagree. However, many people are stuck using it. But that's no reason to try and increase cohesion.

[bobvandevijver]

I'm torn by this change. From one point of view I agree with the reasoning behind this change, but from another point I do not. Simply detaching the soft-deleted entity during a request/message context seems to introduce more issues than what it solves.

As it is required to call flush() to get the softdeletable entity state updated (to set the deleted at time and possibly the actor), you are forced to take that route if you need those values for further handling down the line. However, with this change that is no longer possible, which forces you to first disable the filter again to retrieve the same object that was already there. My use case here is to create an additional log entry after the object has been removed, which references the removed object.

In our projects we have acted on knowing that a softdeleted object is still available within the same request/message context, as you know it won't be fully deleted in the end.

Next to that, issues #2942, #2947 & #2955 seem to be directly caused by this change. Especially #2955 will be almost impossible to fully solve.

So, I believe this needs to be either reverted to solves the issues that have risen, or to make this behaviour configurable.

If anyone is interested, here is a compiler pass to remove the `postFlush` handler when using the stof bundle.

use Exception;
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;

class DisablePostFlushDetachCompilerPass implements CompilerPassInterface
{

  const TAG_NAME = 'doctrine.event_listener';

  public function process(ContainerBuilder $container): void
  {
    $definition = $container->getDefinition('stof_doctrine_extensions.listener.softdeleteable');
    if (!$definition->hasTag(self::TAG_NAME)) {
      throw new Exception('Doctrine event listener not configured?');
    }

    // Remove the postFlush tag
    $tags = $definition->getTags();
    $tags[self::TAG_NAME] = array_filter(
      $tags[self::TAG_NAME],
      static fn (array $tag): bool => ($tag['event'] ?? null) !== 'postFlush'
    );

    // Update the listener tags
    $definition->clearTags();
    $definition->setTags($tags);
  }
}

// Register in your bundle with the following:
// Needs to be run before RegisterEventListenersAndSubscribersPass is run
// $container->addCompilerPass(new DisablePostFlushDetachCompilerPass(),  priority: 10);

[rotdrop]

I'm torn by this change. From one point of view I agree with the reasoning behind this change, but from another point I do not. Simply detaching the soft-deleted entity during a request/message context seems to introduce more issues than what it solves.

As it is required to call flush() to get the softdeletable entity state updated (to set the deleted at time and possibly the actor), you are forced to take that route if you need those values for further handling down the line. However, with this change that is no longer possible, which forces you to first disable the filter again to retrieve the same object that was already there. My use case here is to create an additional log entry after the object has been removed, which references the removed object.

In our projects we have acted on knowing that a softdeleted object is still available within the same request/message context, as you know it won't be fully deleted in the end.

Next to that, issues #2942, #2947 & #2955 seem to be directly caused by this change. Especially #2955 will be almost impossible to fully solve.

So, I believe this needs to be either reverted to solves the issues that have risen, or to make this behaviour configurable.
If anyone is interested, here is a compiler pass to remove the postFlush handler when using the stof bundle.

Also I am still not convinced, although @oojacoboo had the last word so far. But though I did not answer him yet I really do not agree with his last comment in this affair.

So here is a feature request: please revert #2930

Please let us start a discussion and let us come to a better documented agreement about how "soft-deleteable" should behave.

[kopfsalat]

Hi,

I have a follow-up question about this change. Since updating to version 3.20, we have seen errors regarding cascade operations. The flow in our application is as follows (maybe sub optimal but right now it is what it is):

1. An entity that is soft-deletable gets removed.
   1.1. There is a cascade DELETE operation on a "child" entity, which is "hard"-deleted. This child entity has a cascade SET NULL.
2. Flush() is called.
3. Other logic is executed, and at some point, another flush() call is made.
4. The second flush() fails because the child cannot perform the SET NULL operation on the deleted parent.

We now believe that this happens because the cascade delete is not executed during the first flush due to the detachment from the object manager. The second flush fails because of the cascade SET NULL on an object that is no longer part of the object manager.

Could this be a regression, or are we on the wrong path?

Thanks and best wishes,
Jeldrik

[bobvandevijver]

@kopfsalat Yeah, sounds related to this change. You can confirm it by using the CompilerPass I posted in my previous comment here: if using that solves your issue you know for sure.

[kopfsalat]

@bobvandevijver Thanks for the quick response! Using your CompilerPass indeed resolved the issue. I will post a bug report and link it to this PR.