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