Skip to content

Feature: add a new event for many-to-many relationships#498

Open
andrestejerina97 wants to merge 1 commit intomainfrom
feature/new-event-for-formatters
Open

Feature: add a new event for many-to-many relationships#498
andrestejerina97 wants to merge 1 commit intomainfrom
feature/new-event-for-formatters

Conversation

@andrestejerina97
Copy link
Contributor

@andrestejerina97 andrestejerina97 commented Feb 11, 2026

@andrestejerina97 andrestejerina97 force-pushed the feature/new-event-for-formatters branch 2 times, most recently from ffb0da8 to e142976 Compare February 13, 2026 19:22
@andrestejerina97 andrestejerina97 marked this pull request as ready for review February 18, 2026 14:02
@andrestejerina97 andrestejerina97 force-pushed the feature/new-event-for-formatters branch 3 times, most recently from 1613295 to bd0d027 Compare February 18, 2026 18:02
@andrestejerina97
Copy link
Contributor Author

@martinquiroga-exo ready to review

Copy link
Contributor

@martinquiroga-exo martinquiroga-exo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

];

$eventType = $isManyToMany
? ($isDeletion ? IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_DELETE : IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_UPDATE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this concatenated ternary operators, perhaps I would like to see an exit early guard clause when the event is an EVENT_COLLECTION_UPDATE.

I defer to Casey on this one since is a design issue rather than an implementation one.

Copy link

@caseylocker caseylocker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to defer to whatever Seb comes up with but this is what I'm seeing.

The auditCollection() method changed the calling convention for all collection updates not just ManyToMany. For non-ManyToMany collections, it now passes $owner (the entity) as the subject instead of $col (the collection itself), and $payload instead of [] as the change set.

Both strategies downstream expect a PersistentCollection as $subject for EVENT_COLLECTION_UPDATE:

  • OTLP strategy: instanceof PersistentCollection checks fail, formatter returns null, audit entries silently dropped
  • DB strategy: calls count($subject) and $subject->getSnapshot() on the entity — throws \Error (not \Exception), uncaught by the catch block

Fix: Unless the intent was to actually change the behavior for all for some reason, only change the subject/payload for ManyToMany events; preserve the original $strategy->audit($col, [], EVENT_COLLECTION_UPDATE, $ctx) call for non-ManyToMany collections.

@andrestejerina97 andrestejerina97 force-pushed the feature/new-event-for-formatters branch 3 times, most recently from 090b9c5 to 186913f Compare February 26, 2026 19:06
@andrestejerina97
Copy link
Contributor Author

@caseylocker it's ready to review

Copy link

@caseylocker caseylocker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two fixes needed before merge:

  1. app/Audit/AuditLogOtlpStrategy.php — Add M2M event mappings

mapEventTypeToAction() and getLogMessage() need explicit cases for EVENT_COLLECTION_MANYTOMANY_UPDATE (→ ACTION_COLLECTION_UPDATE / LOG_MESSAGE_COLLECTION_UPDATED) and EVENT_COLLECTION_MANYTOMANY_DELETE (→ ACTION_DELETE / LOG_MESSAGE_DELETED). Without this, M2M audit entries hit Elasticsearch with action=unknown, making them invisible to dashboards/queries filtering on action type.

buildAuditLogData() should also extract collection metadata from $change_set['collection'] for M2M events, since the subject is the owner entity (not a PersistentCollection).

  1. app/Audit/AuditLogFormatterFactory.php — Fix generic M2M fallback

Lines 110-123: when no context-specific formatter exists, the fallback creates EntityCollectionUpdateAuditLogFormatter and passes the owner entity as $subject. That formatter calls $subject->getInsertDiff() (line 50 of EntityCollectionUpdateAuditLogFormatter), which doesn't exist on the owner entity. This either silently drops the audit entry or throws an uncaught \Error. The fallback should pass $change_set['collection'] as the subject instead of the owner, or use a M2M-aware generic formatter.

Why this blocks merge: Without fix 1, OTLP audit data is misclassified. Without fix 2, any M2M collection change on an entity without a registered custom formatter will silently fail or crash.

@matiasperrone-exo matiasperrone-exo force-pushed the feature/new-event-for-formatters branch from 186913f to 8af3df1 Compare March 3, 2026 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants