[SoftDeleteable] Add option to enable or disable handling of postFlush

[HypeMC]

Closes #2956

As mentioned in this discussion, the original PR seems to have introduced a few new issues: #2942, #2947, and #2955.

Maybe it makes sense to revert it for now, at least until there's a fix for the original problem that doesn't cause new ones. At the moment, it feels like it's creating more trouble than it's solving.

[kopfsalat]

Hi,
I would like to add Issue #2963 to the list. For now, we can use the CompilerPass, but I would much prefer #2930 to be reverted as it seems to have too many side effects.

[rotdrop]

@phansys As you merged #2930, could you please also have a look and undo it? Kind Thanks!

[HypeMC]

Another option would be to make the postFlush event configurable.

[rotdrop]

Another option would be to make the postFlush event configurable.

Yes, why not. But I would then suggest that the default is "disabled" because it is a breaking change to have it enabled.

[HypeMC]

Ok, let's try it that way then, PR updated.

[mbabker]

I wish I had merge rights to help move this forward, but my two cents.

On the listener, it probably doesn't need the constructor parameter and the setter for the option, just the setter would be fine (and consistent with all the other bits that can be configured on a listener).

If it's possible, having some kind of failing test case over the exact issues that the changes here cover would be really helpful going forward because the changes in behavior that this post-flush change made starts getting deep into the internals of how the listener interacts with the object managers, and it's probably something that shouldn't be accidentally changed in a new way that breaks things later.

[HypeMC]

On the listener, it probably doesn't need the constructor parameter and the setter for the option, just the setter would be fine (and consistent with all the other bits that can be configured on a listener).

@mbabker I can remove it if needed. I included it because the ReferencesListener defines both a constructor argument and an adder method.

If it's possible, having some kind of failing test case over the exact issues that the changes here cover would be really helpful going forward because the changes in behavior that this post-flush change made starts getting deep into the internals of how the listener interacts with the object managers, and it's probably something that shouldn't be accidentally changed in a new way that breaks things later.

@mbabker The test I added fails when the new behavior is enabled.

[rotdrop]

If it's possible, having some kind of failing test case over the exact issues that the changes here cover would be really helpful going forward because the changes in behavior that this post-flush change made starts getting deep into the internals of how the listener interacts with the object managers, and it's probably something that shouldn't be accidentally changed in a new way that breaks things later.

As far as I can tell this updated merge request modifies and enhances the test case submitted by the author of the original merge request in order to fit the new scheme and adds one further test which bails out if the old pre-#2930 behaviour is not met.

The exact documented issues that this merge request covers are detailed in the first comment of this PR. If one would like to avoid to dig "deep into the internals" then #2930 should just be reverted.

[phansys]

IIUC, this PR should also have at least an entry for the Changed and Fixed sections, because the added option changes the way the previous fix behaves, while fix the issues mentioned in the description.

Could you please confirm?

[HypeMC]

Hi. Yes you're right. I've updated the changelog.

[codecov]

Codecov Report

❌ Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.55%. Comparing base (ea1d375) to head (fa0954b).
⚠️ Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
src/SoftDeleteable/SoftDeleteableListener.php	81.81%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2958      +/-   ##
==========================================
+ Coverage   78.43%   78.55%   +0.12%     
==========================================
  Files         168      169       +1     
  Lines        8806     8833      +27     
==========================================
+ Hits         6907     6939      +32     
+ Misses       1899     1894       -5     

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

[phansys]

Thank you @HypeMC!