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

Conversation

oojacoboo
Copy link

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.

@phansys phansys added Bug A confirmed bug in Extensions that needs fixing. SoftDeleteable Needs Changelog note labels Mar 9, 2025
Copy link

codecov bot commented Mar 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.53%. Comparing base (8264aad) to head (1fe0d03).

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
Copy link
Author

@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
Copy link
Collaborator

phansys commented Mar 9, 2025

@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.

@@ -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.

oojacoboo and others added 4 commits March 9, 2025 16:51
Co-authored-by: Javier Spagnoletti <phansys@gmail.com>
Co-authored-by: Javier Spagnoletti <phansys@gmail.com>
Co-authored-by: Javier Spagnoletti <phansys@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A confirmed bug in Extensions that needs fixing. Needs Rebase SoftDeleteable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants