auditor-bundle icon indicating copy to clipboard operation
auditor-bundle copied to clipboard

Dissociate action not being logged for ManyToMany when all entries are removed or last entry is removed leaving an empty collection

Open johnvanham opened this issue 3 years ago • 7 comments

I have a number of ManyToMany bidirectional relationships and have noticed that associate actions are logged fine, however disassociate actions are not logged if all items are removed or if the final item is removed. Dissociate actions are only logged if some items remain.

Is there something I need to check for and run in my controllers to trigger the disassociate action to be logged if a collection has been emptied?

Currently the controllers simply have an $em->flush() on after form validation check so there is nothing custom involved I do not think.

johnvanham avatar Jul 07 '21 16:07 johnvanham

This seems relevant although it's from 2013:

https://stackoverflow.com/questions/15311083/whats-the-proper-use-of-unitofwork-getscheduledcollectiondeletions-in-doctr/15394241#15394241

$collection->toArray() is empty in..

DH\Auditor\Provider\Doctrine\Auditing\Transaction\TransactionHydrator::hydrateWithScheduledCollectionDeletions

.. when all entries of a collection are removed and the entity is flushed.

My forms are using EntityType with 'multiple' set to true. So perhaps Symfony is using the clear method and therefore the hydrator has no data in the unitOfWork?

Is it possible in the controller to do something to remove the collection without using clear before running the normal $em->flush(), or is there a setting to change Symfony's behaviour so it doesn't use clear? I'm guessing this isn't something that can be 'fixed' by the event listner in the auditor? But is the some sort of workaround for this scenario?

johnvanham avatar Jul 08 '21 09:07 johnvanham

This issue is mentioned here: https://github.com/symfony/symfony/issues/17154 and symfony/symfony#13713

And was going to be fixed in Doctrine but looks like it never was? https://github.com/doctrine/orm/pull/7120

Still hunting for a workaround.

johnvanham avatar Jul 08 '21 10:07 johnvanham

@johnvanham Thanks for the extensive research! Unfortunately, I don't have any workaround as of now, it's clearly linked to Doctrine's internals :/

DamienHarper avatar Jul 15 '21 20:07 DamienHarper

Hi @DamienHarper - thanks for the reply

I've done some more research on this and developed both a kludgy workaround, and a PR for symfony/doctrine-bridge

I've confirmed the problem to be a decision by Symfony to use a doctrine clear() on collections when they become empty. The thinking behind this is that a clear has better performance. That's called by an EventListener in symfony/doctrine-bridge here:

https://github.com/symfony/symfony/blob/d58b5c3b76b8e6a630a841d284ed43dbbc56ade7/src/Symfony/Bridge/Doctrine/Form/EventListener/MergeDoctrineCollectionListener.php#L49

That event listener is added to form fields with types that inherit DoctrineType e.g. EntityType if 'multiple' is enabled which is what is required for ManyToMany fields.

https://github.com/symfony/symfony/blob/572864b223fd3f8937cc626bed969313e0c9e224/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php#L111

The PR will add an option to disable the clear. This gives the developer the ability to choose between full auditing or a performance enhancement.

Here's the commit I've made to a test version of symfony\doctrine-bridge for that: https://github.com/johnvanham/doctrine-bridge/commit/f46c924f64d72e75bcf1a2b3cf8c4386e08e84ac

To use it you just add the option 'disable_clear' to the relevant fields.

Now the workaround - it's messy but it works and can be used if this PR is rejected for any reason (I've not submitted it yet). It's kind of messy, but it's to add two event listeners, one with higher priority than MergeDoctrineCollectionListener and one lower. The one that runs first adds some dummy data to the field which is empty, this prevents MergeDoctrineCollectionListener from using clear because it thinks there is still data. Then the event listener which runs after MergeDoctrineCollectionListener empties the collection again and doctrine handles it as normal without the clear which allows auditing to work. If the PR gets rejected I'll provide details in case others find that useful. I'm not sure if there are any implications of that which I've not realised but it did seem to work for me!

johnvanham avatar Jul 16 '21 08:07 johnvanham

Thank you, @johnvanham your workaround works perfectly well. I ended up extending EntityType and adding the Subscribers as you suggested which handles it nicely. As Symfony and Doctrine have been playing ping-pong with the responsibilities around this issue for quite some time and neither team seems eager to fix it, this will have to do.

jhard avatar Nov 01 '21 16:11 jhard

@jhard no problem, I haven't yet done the PR to symfony/doctrine-bridge since I am unsure how to write a test for it which I expect they will require to approve the tweak. Any help on that would be gratefully received :-)

johnvanham avatar Nov 10 '21 09:11 johnvanham

@johnvanham thanks for research.. I just got this issue in my project.. I think not "hack" solution is do not use symfony form for save and use API. In api use example $company->removeUser($user) and it will log dissociate if $user is last in relation.

Another hack way what i just tried and in this moment i do not know if it is worth.. Use FormEvents::PRE_SET_DATA where get and check how many items there are. Save it example $this->countUsers. Save entity $this->entity = $event->getData(). Use in same form FormEvents::PRE_SUBMIT and there check if $event->getData() has field users. If not it is mean all users are deleted so we can use $this->entity->removeUser(get user from $this->entity->$getUsers->toArray() foreach it) .. And we can get dissociate logs and remove before submit.

how i saying i do not know if it is worth.. Anyway i prefer APIs and now i know if need in future audit so start with api and do not use symfony forms.

HrjSnz avatar Apr 22 '22 14:04 HrjSnz

As I previously said, the issue is out of the scope of auditor so I close this GH issue.

DamienHarper avatar Dec 05 '22 19:12 DamienHarper

Thanks for the reminder @DamienHarper - someone has suggested a test for this to submit to symfony so I shall do that. But yes that is outside the control of auditor.

johnvanham avatar Dec 06 '22 09:12 johnvanham