Incorrect array destructuring when processing disassocitions about auditor

[DamienHarper]

@janklan 2.0.1 has just been released, including your fixes, thanks!

[janklan]

@DamienHarper I found another related issue that I fixed in the associated PR - after adding the missing $id, one relationship worked fine, but one did not. I don't know enough about Auditor's internals, so I did not track why, but https://github.com/DamienHarper/auditor/blob/master/src/Provider/Doctrine/Auditing/Transaction/TransactionHydrator.php#L102 does not supply the entity ID.

Effectively there are two incompatible disassociation events, one with the ID, one without. I assume the with is the right scenario?

Neither issues were picked up by the test suite.

Also I don't mean to tell you how to do things, but I've had extremely bad experience relying on arrays as data objects passing data around.

Why not use a DTO class with specified properties, data types etc?

So not,

[
    $collection->getOwner(),
    $entity,
    $this->id($entityManager, $entity),
    $mapping,
]

rather

new DisassociationEvent($collection->getOwner(), $entity, $this->id($entityManager, $entity), $mapping);

In a perfect world, this would look like

class DisassociationEvent(public readonly object $source, public readonly object $target, public readonly mixed $id, public readonly array $mapping) {}

Clearly with the need to support older PHP, the actual class would be more verbose, but something like this would let static code analysers pick up errors of this kind - unlike when using vague arrays?

The auditor is an awesome tool, thank you for your work.

[DamienHarper]

@janklan Thanks for that clear report and the associated PR, much appreciated.
You're right regarding DTO vs basic arrays, it has to be changed. Feel free to submit a PR if you want to, I'd be glad to merge it.

[janklan]

Thanks, it's great to see some quick action!

[janklan]

@DamienHarper I tried to work out why didn't the test suite fail even though the coverage was there. It turns out the failing code is not really being covered - I'm not quite sure how would the listed test cases cover the problematic line:

Sure enough, when you only run php -d pcov.enabled=1 ./vendor/bin/phpunit --filter=TransactionProcessorTest::testDissociateManyToMany, none of the lines are covered:

I tried to extend the test case to actually employ TransactionHydrator to populate a real Transaction object that could then be passed to the TransactionProcessor to trip the error, but quite frankly, my understanding of Doctrine internals is not good enough to get it done. I failed to make the UoW object report any removals from the Post::$tags collection.

If you could push me in the right direction, I'm happy to help out by adding a more complete test suite, and then swap the arrays to DTO classes. I don't dare to touch the code if it's not reliably covered. I'm concerned I could blow up other people's projects.

[DamienHarper]

@janklan #93 brings better coverage