Feature: add a new event for many-to-many relationships#498
Feature: add a new event for many-to-many relationships#498andrestejerina97 wants to merge 2 commits intomainfrom
Conversation
ffb0da8 to
e142976
Compare
1613295 to
bd0d027
Compare
|
@martinquiroga-exo ready to review |
app/Audit/AuditEventListener.php
Outdated
| ]; | ||
|
|
||
| $eventType = $isManyToMany | ||
| ? ($isDeletion ? IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_DELETE : IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_UPDATE) |
There was a problem hiding this comment.
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.
caseylocker
left a comment
There was a problem hiding this comment.
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.
090b9c5 to
186913f
Compare
|
@caseylocker it's ready to review |
caseylocker
left a comment
There was a problem hiding this comment.
Two fixes needed before merge:
- 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).
- 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.
186913f to
8af3df1
Compare
02a1841 to
be8a5f1
Compare
be8a5f1 to
6b78204
Compare
6b78204 to
9d27bf5
Compare
|
@caseylocker it's ready to review |
app/Audit/ConcreteFormatters/EntityManyToManyCollectionDeleteAuditLogFormatter.php
Outdated
Show resolved
Hide resolved
app/Audit/ConcreteFormatters/EntityManyToManyCollectionUpdateAuditLogFormatter.php
Outdated
Show resolved
Hide resolved
1d0e92b to
b3d0359
Compare
b3d0359 to
1e069fd
Compare
📝 WalkthroughWalkthroughThis PR extends the audit system to support many-to-many collection update and deletion events. It adds new event types, corresponding audit formatters, and listener enhancements to capture and format collection changes with metadata extraction and detailed messaging. Changes
Sequence DiagramsequenceDiagram
participant UOW as UnitOfWork
participant Listener as AuditEventListener
participant Formatter as M2M Formatter
participant Formatter2 as Child Formatter
participant Strategy as OTLP Strategy
UOW->>Listener: onFlush (collection change detected)
Listener->>Listener: auditCollection() - determine event type
activate Listener
Listener->>Listener: fetchManyToManyIds() - extract affected IDs
Listener->>Listener: buildAuditContext() - prepare payload
deactivate Listener
Listener->>Strategy: record collection event (UPDATE/DELETE)
activate Strategy
Strategy->>Strategy: Extract collection metadata<br/>(type, counts, dirty state)
deactivate Strategy
Strategy->>Formatter: format($subject, $change_set)
activate Formatter
Formatter->>Formatter: Extract mapping & validate<br/>PersistentCollection
alt has added_ids/removed_ids
Formatter->>Formatter: formatManyToManyDetailedMessage()
else no added_ids/removed_ids
alt has child formatter
Formatter->>Formatter2: format each diff item
Formatter2-->>Formatter: formatted message
else no child formatter
Formatter->>Formatter: buildManyToManyDetailedMessage()
end
end
deactivate Formatter
Formatter-->>Listener: formatted audit log message
Listener->>Listener: publish audit event
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
@martinquiroga-exo ready to review |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/OpenTelemetry/Formatters/AllFormattersIntegrationTest.php (1)
147-169:⚠️ Potential issue | 🟠 MajorFail on unsupported event types.
$unsupportedis populated but never asserted. Any formatter that still rejectsEVENT_COLLECTION_MANYTOMANY_UPDATEorEVENT_COLLECTION_MANYTOMANY_DELETEthrough the "event type" path will keep this test green, so the new coverage can miss the PR's main behavior.🛠️ Proposed fix
- $this->assertEmpty($errors, "Event type handling failed:\n" . implode("\n", $errors)); + $this->assertEmpty($unsupported, "Unsupported event types:\n" . implode("\n", $unsupported)); + $this->assertEmpty($errors, "Event type handling failed:\n" . implode("\n", $errors));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OpenTelemetry/Formatters/AllFormattersIntegrationTest.php` around lines 147 - 169, The test collects unsupported formatters in $unsupported but never fails on them, allowing regressions; after the loop and before asserting $errors, add an assertion that $unsupported is empty (with a clear message) so any formatterClass from discoverFormatters() that rejects event types like EVENT_COLLECTION_MANYTOMANY_UPDATE or EVENT_COLLECTION_MANYTOMANY_DELETE (caught via the "event type" path in the catch block) will fail the test; locate the loop using FormatterTestHelper::assertFormatterCanBeInstantiated and the $unsupported array and assertEmpty($unsupported, "Unsupported event types found:\n" . implode("\n", $unsupported)).
🧹 Nitpick comments (7)
app/Audit/ConcreteFormatters/SummitAttendeeAuditLogFormatter.php (2)
118-123: Remove extraneous blank lines.Lines 119-121 contain unnecessary whitespace that appears to be a leftover from editing.
🧹 Proposed fix
try { $removed_ids = $collectionData['removed_ids'] ?? []; - - - $field = $collectionData['field'] ?? 'unknown';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Audit/ConcreteFormatters/SummitAttendeeAuditLogFormatter.php` around lines 118 - 123, Remove the extraneous blank lines between the assignment of $removed_ids and the subsequent assignments to $field and $targetEntity in the SummitAttendeeAuditLogFormatter class so the code reads compactly as consecutive assignments (ensure $removed_ids = $collectionData['removed_ids'] ?? []; is immediately followed by $field = $collectionData['field'] ?? 'unknown'; and $targetEntity = $collectionData['target_entity'] ?? 'unknown';).
59-76: Remove unused$subjectparameter.The
$subjectparameter is declared but never used withinhandleManyToManyCollection. Since$idand$nameare already extracted and passed separately, this parameter is redundant.♻️ Proposed fix
- private function handleManyToManyCollection(SummitAttendee $subject, array $change_set, $id, $name, bool $isDeletion): ?string + private function handleManyToManyCollection(array $change_set, $id, $name, bool $isDeletion): ?string {Also update the call sites in lines 47 and 50:
case IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_UPDATE: - return $this->handleManyToManyCollection($subject, $change_set, $id, $name, false); + return $this->handleManyToManyCollection($change_set, $id, $name, false); case IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_DELETE: - return $this->handleManyToManyCollection($subject, $change_set, $id, $name, true); + return $this->handleManyToManyCollection($change_set, $id, $name, true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Audit/ConcreteFormatters/SummitAttendeeAuditLogFormatter.php` around lines 59 - 76, The method handleManyToManyCollection has an unused $subject parameter; remove $subject from its signature (change function handleManyToManyCollection(SummitAttendee $subject, array $change_set, $id, $name, bool $isDeletion): ?string to handleManyToManyCollection(array $change_set, $id, $name, bool $isDeletion): ?string) and update all call sites that pass the subject (the two places invoking handleManyToManyCollection) to stop providing the extra argument so they pass only $change_set, $id, $name, $isDeletion; ensure type hints and usages inside handleManyToManyCollection remain consistent after removal.app/Audit/ConcreteFormatters/DefaultEntityManyToManyCollectionUpdateAuditLogFormatter.php (1)
55-56: Consider: Inconsistent key naming between added_ids/removed_ids and deleted_ids.The code checks for both
'removed_ids'and'deleted_ids'as fallback. While this provides flexibility, it may lead to confusion about which key should be used. Consider documenting the expected payload structure or standardizing on one key name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Audit/ConcreteFormatters/DefaultEntityManyToManyCollectionUpdateAuditLogFormatter.php` around lines 55 - 56, Standardize the change_set keys by treating "removed_ids" as the canonical name and explicitly map any legacy "deleted_ids" to it before use in DefaultEntityManyToManyCollectionUpdateAuditLogFormatter: in the code that prepares $change_set (or immediately before computing $addedIds/$removedIds) normalize $change_set['removed_ids'] = $change_set['removed_ids'] ?? $change_set['deleted_ids'] ?? null, then set $addedIds = $change_set['added_ids'] ?? null and $removedIds = $change_set['removed_ids']; also update any docblocks/comments or function/method docs that reference $change_set to state that "removed_ids" is the standard key (and "deleted_ids" is supported as an alias).app/Audit/ConcreteFormatters/DefaultEntityManyToManyCollectionDeleteAuditLogFormatter.php (1)
60-65: Debug logging may be verbose in production.The debug logging includes the full
deletedIdsarray content. For collections with many IDs, this could produce very large log entries. Consider logging only the count in debug mode.🔧 Suggested improvement
$deletedIds = isset($change_set['deleted_ids']) ? $change_set['deleted_ids'] : null; Log::debug("DefaultEntityManyToManyCollectionDeleteAuditLogFormatter::format - deletedIds", [ - 'deletedIds' => $deletedIds, 'isNull' => $deletedIds === null, 'count' => $deletedIds ? count($deletedIds) : 0 ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Audit/ConcreteFormatters/DefaultEntityManyToManyCollectionDeleteAuditLogFormatter.php` around lines 60 - 65, The debug statement in DefaultEntityManyToManyCollectionDeleteAuditLogFormatter::format currently logs the entire $deletedIds array which can be huge; change the Log::debug call to avoid serializing the full array and instead log only the count (use count($deletedIds) or 0 when null) and a short flag indicating null/empty, referencing the $deletedIds variable so the formatter still provides useful diagnostic info without dumping the full collection.app/Audit/AbstractAuditLogFormatter.php (1)
189-239: Edge case: Empty message detail when no IDs present.When both
$addedIdsand$removedIdsare empty arrays,formatManyToManyDetailedMessagewill produce a message ending with a colon and empty detail:"Many-to-Many collection 'field' action: ". Consider handling this edge case.🔧 Suggested improvement
protected static function formatManyToManyDetailedMessage(array $details, int $addCount, int $removeCount, string $action): string { $field = $details['field'] ?? 'unknown'; $target = $details['target_entity'] ?? 'unknown'; $addedIds = $details['added_ids'] ?? []; $removedIds = $details['removed_ids'] ?? []; $parts = []; if (!empty($addedIds)) { $parts[] = sprintf("Added %d %s(s): %s", $addCount, $target, implode(', ', $addedIds)); } if (!empty($removedIds)) { $parts[] = sprintf("Removed %d %s(s): %s", $removeCount, $target, implode(', ', $removedIds)); } $detailStr = implode(' | ', $parts); + if (empty($detailStr)) { + return sprintf("Many-to-Many collection '%s' %s: no changes", $field, $action); + } return sprintf("Many-to-Many collection '%s' %s: %s", $field, $action, $detailStr); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Audit/AbstractAuditLogFormatter.php` around lines 189 - 239, formatManyToManyDetailedMessage can produce a trailing colon with empty details when both added and removed ID lists are empty; change the method to detect when $parts is empty and return a cleaner message (e.g., "Many-to-Many collection 'FIELD' ACTION: no changes" or "Many-to-Many collection 'FIELD' ACTION" without the colon). Update the logic in formatManyToManyDetailedMessage to check if $parts is empty and set $detailStr to a sensible placeholder (or omit the colon) before building the final sprintf; reference the method name formatManyToManyDetailedMessage and the keys 'field', 'added_ids', 'removed_ids' to locate the code to modify. Ensure unit/output formatting remains consistent with existing messages.app/Audit/AuditLogOtlpStrategy.php (1)
200-209: Minor: Inconsistent error message format.The error message at line 209 has an unusual format with "AuditLogOtlpStrategy:: unknown targetEntity" (double colon followed by space). Consider using a consistent format.
🔧 Suggested fix
} catch (\Exception $ex) { - return 'AuditLogOtlpStrategy:: unknown targetEntity'; + return 'unknown'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Audit/AuditLogOtlpStrategy.php` around lines 200 - 209, The catch block in AuditLogOtlpStrategy that currently returns "AuditLogOtlpStrategy:: unknown targetEntity" should be changed to a consistently formatted message (for example "AuditLogOtlpStrategy: unknown targetEntity") and preferably include the exception details; update the return in the catch of the method that calls $collection->getMapping() to return a string like "AuditLogOtlpStrategy: unknown targetEntity - " . $ex->getMessage() (or at minimum remove the double colon and extra space so it reads "AuditLogOtlpStrategy: unknown targetEntity").tests/OpenTelemetry/Formatters/AuditEventListenerTest.php (1)
66-122: Add listener coverage for initialized collection deletions.
AuditEventListener::auditCollection()now has a separate delete path for initialized many-to-many collections viafetchDeletedIdsFromInitializedCollection(), but this file only exercises the non-many-to-many branch and the raw ID fetch helper. That leaves the new diff-baseddeleted_idspayload untested at the listener level.PersistentCollectionTestHelpershould make this path straightforward to cover.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OpenTelemetry/Formatters/AuditEventListenerTest.php` around lines 66 - 122, Add a new unit test that exercises AuditEventListener::auditCollection()'s many-to-many delete branch by constructing an initialized PersistentCollection (use PersistentCollectionTestHelper to create a collection that is initialized and has removed elements), attach it to a mock owner and mapping for a ManyToManyOwningSideMapping, inject a mocked EntityManager (and ClassMetadata) into the AuditEventListener instance, then call auditCollection() and assert the emitted payload includes the diff-style "deleted_ids" (e.g., [10,11]) produced by AuditEventListener::fetchDeletedIdsFromInitializedCollection(); ensure the test uses the same mocking pattern as existing tests (QueryBuilder/Connection mocks if needed) and targets the listener-level behavior rather than the helper method directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Audit/AuditEventListener.php`:
- Around line 216-285: Remove the unused $uow parameter from the
fetchManyToManyIds method signature and any related docblock, then update all
call sites that pass $uow into fetchManyToManyIds to call it without that
argument; specifically edit the AuditEventListener::fetchManyToManyIds
declaration to drop $uow and modify the internal callers within the class to
call fetchManyToManyIds($collection) instead of passing the unused $uow. Ensure
nothing else relies on the removed parameter and run static analysis/tests to
confirm no remaining references.
- Line 114: The $ui array is initialized but never populated, making later
accesses to $ui['app'] and $ui['flow'] always null; update the
AuditEventListener code to either remove the unused $ui initialization and use
null/defaults directly where $ui['app']/$ui['flow'] are read, or populate $ui
from the current request/route context (e.g. pull values from the Request or
route attributes) before those reads so $ui['app'] and $ui['flow'] contain the
intended values; ensure you update the same method in AuditEventListener where
$ui is declared and where $ui['app']/$ui['flow'] are accessed.
In
`@app/Audit/ConcreteFormatters/DefaultEntityManyToManyCollectionUpdateAuditLogFormatter.php`:
- Around line 96-109: The child-entity creation loop is pushing potentially null
returns from $this->child_entity_formatter->format(...) into $changes, causing
null entries in the final string; update the creation loop (the foreach over
$insertDiff calling format with
IChildEntityAuditLogFormatter::CHILD_ENTITY_CREATION) to only append non-null
results (e.g., check the formatter return is not null before $changes[] = ...)
or run an array_filter on $changes before the implode, mirroring the
null-filtering behavior used for deletions (format(..., CHILD_ENTITY_DELETION)).
In `@app/Audit/ConcreteFormatters/SummitAttendeeAuditLogFormatter.php`:
- Around line 109-112: The formatters in SummitAttendeeAuditLogFormatter are
inconsistent: formatManyToManyUpdate catches exceptions and returns a fallback
message, while formatManyToManyDelete returns null; change
formatManyToManyDelete to mirror formatManyToManyUpdate by catching \Throwable
and returning the same formatted fallback string (e.g., using $id, $name and
$this->getUserInfo()) and logging the exception with Log::warning so both update
and delete produce a consistent audit fallback on error.
In
`@tests/OpenTelemetry/Formatters/DefaultEntityManyToManyCollectionUpdateAuditLogFormatterTest.php`:
- Around line 250-264: The current formatter preserves an empty segment when a
child formatter returns null, causing a leading '|' in
DefaultEntityManyToManyCollectionUpdateAuditLogFormatter output; update the
format method in DefaultEntityManyToManyCollectionUpdateAuditLogFormatter so it
collects child outputs from the IChildEntityAuditLogFormatter->format calls,
filters out null/empty values, and then joins the remaining pieces with the '|'
separator (only inserting separators between non-empty segments) so null child
results are skipped and no leading separator is emitted.
In
`@tests/OpenTelemetry/Formatters/SummitAttendeeAuditLogFormatterManyToManyTest.php`:
- Around line 70-79: The test ManyToMany null-case provider currently always
calls format($attendee, []) so handleManyToManyCollection() exits on missing
collection and the DP_*_WITHOUT_CONTEXT rows never exercise the "collection
present but context missing" branch; update the DataProvider
'providesNullCasesForManyToMany' rows labeled DP_*_WITHOUT_CONTEXT to pass an
input array that includes a valid collection key (e.g. ['collection' => <dummy
collection value>]) but omits the required context key so
testManyToManyReturnsNullWithoutRequiredContextOrCollection (and the duplicate
cases around lines 203-210) will hit the no-context path in
handleManyToManyCollection().
---
Outside diff comments:
In `@tests/OpenTelemetry/Formatters/AllFormattersIntegrationTest.php`:
- Around line 147-169: The test collects unsupported formatters in $unsupported
but never fails on them, allowing regressions; after the loop and before
asserting $errors, add an assertion that $unsupported is empty (with a clear
message) so any formatterClass from discoverFormatters() that rejects event
types like EVENT_COLLECTION_MANYTOMANY_UPDATE or
EVENT_COLLECTION_MANYTOMANY_DELETE (caught via the "event type" path in the
catch block) will fail the test; locate the loop using
FormatterTestHelper::assertFormatterCanBeInstantiated and the $unsupported array
and assertEmpty($unsupported, "Unsupported event types found:\n" . implode("\n",
$unsupported)).
---
Nitpick comments:
In `@app/Audit/AbstractAuditLogFormatter.php`:
- Around line 189-239: formatManyToManyDetailedMessage can produce a trailing
colon with empty details when both added and removed ID lists are empty; change
the method to detect when $parts is empty and return a cleaner message (e.g.,
"Many-to-Many collection 'FIELD' ACTION: no changes" or "Many-to-Many collection
'FIELD' ACTION" without the colon). Update the logic in
formatManyToManyDetailedMessage to check if $parts is empty and set $detailStr
to a sensible placeholder (or omit the colon) before building the final sprintf;
reference the method name formatManyToManyDetailedMessage and the keys 'field',
'added_ids', 'removed_ids' to locate the code to modify. Ensure unit/output
formatting remains consistent with existing messages.
In `@app/Audit/AuditLogOtlpStrategy.php`:
- Around line 200-209: The catch block in AuditLogOtlpStrategy that currently
returns "AuditLogOtlpStrategy:: unknown targetEntity" should be changed to a
consistently formatted message (for example "AuditLogOtlpStrategy: unknown
targetEntity") and preferably include the exception details; update the return
in the catch of the method that calls $collection->getMapping() to return a
string like "AuditLogOtlpStrategy: unknown targetEntity - " . $ex->getMessage()
(or at minimum remove the double colon and extra space so it reads
"AuditLogOtlpStrategy: unknown targetEntity").
In
`@app/Audit/ConcreteFormatters/DefaultEntityManyToManyCollectionDeleteAuditLogFormatter.php`:
- Around line 60-65: The debug statement in
DefaultEntityManyToManyCollectionDeleteAuditLogFormatter::format currently logs
the entire $deletedIds array which can be huge; change the Log::debug call to
avoid serializing the full array and instead log only the count (use
count($deletedIds) or 0 when null) and a short flag indicating null/empty,
referencing the $deletedIds variable so the formatter still provides useful
diagnostic info without dumping the full collection.
In
`@app/Audit/ConcreteFormatters/DefaultEntityManyToManyCollectionUpdateAuditLogFormatter.php`:
- Around line 55-56: Standardize the change_set keys by treating "removed_ids"
as the canonical name and explicitly map any legacy "deleted_ids" to it before
use in DefaultEntityManyToManyCollectionUpdateAuditLogFormatter: in the code
that prepares $change_set (or immediately before computing
$addedIds/$removedIds) normalize $change_set['removed_ids'] =
$change_set['removed_ids'] ?? $change_set['deleted_ids'] ?? null, then set
$addedIds = $change_set['added_ids'] ?? null and $removedIds =
$change_set['removed_ids']; also update any docblocks/comments or
function/method docs that reference $change_set to state that "removed_ids" is
the standard key (and "deleted_ids" is supported as an alias).
In `@app/Audit/ConcreteFormatters/SummitAttendeeAuditLogFormatter.php`:
- Around line 118-123: Remove the extraneous blank lines between the assignment
of $removed_ids and the subsequent assignments to $field and $targetEntity in
the SummitAttendeeAuditLogFormatter class so the code reads compactly as
consecutive assignments (ensure $removed_ids = $collectionData['removed_ids'] ??
[]; is immediately followed by $field = $collectionData['field'] ?? 'unknown';
and $targetEntity = $collectionData['target_entity'] ?? 'unknown';).
- Around line 59-76: The method handleManyToManyCollection has an unused
$subject parameter; remove $subject from its signature (change function
handleManyToManyCollection(SummitAttendee $subject, array $change_set, $id,
$name, bool $isDeletion): ?string to handleManyToManyCollection(array
$change_set, $id, $name, bool $isDeletion): ?string) and update all call sites
that pass the subject (the two places invoking handleManyToManyCollection) to
stop providing the extra argument so they pass only $change_set, $id, $name,
$isDeletion; ensure type hints and usages inside handleManyToManyCollection
remain consistent after removal.
In `@tests/OpenTelemetry/Formatters/AuditEventListenerTest.php`:
- Around line 66-122: Add a new unit test that exercises
AuditEventListener::auditCollection()'s many-to-many delete branch by
constructing an initialized PersistentCollection (use
PersistentCollectionTestHelper to create a collection that is initialized and
has removed elements), attach it to a mock owner and mapping for a
ManyToManyOwningSideMapping, inject a mocked EntityManager (and ClassMetadata)
into the AuditEventListener instance, then call auditCollection() and assert the
emitted payload includes the diff-style "deleted_ids" (e.g., [10,11]) produced
by AuditEventListener::fetchDeletedIdsFromInitializedCollection(); ensure the
test uses the same mocking pattern as existing tests (QueryBuilder/Connection
mocks if needed) and targets the listener-level behavior rather than the helper
method directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 866ce53b-deac-415c-b5d9-86b32cd300b4
📒 Files selected for processing (15)
app/Audit/AbstractAuditLogFormatter.phpapp/Audit/AuditEventListener.phpapp/Audit/AuditLogFormatterFactory.phpapp/Audit/AuditLogOtlpStrategy.phpapp/Audit/ConcreteFormatters/DefaultEntityManyToManyCollectionDeleteAuditLogFormatter.phpapp/Audit/ConcreteFormatters/DefaultEntityManyToManyCollectionUpdateAuditLogFormatter.phpapp/Audit/ConcreteFormatters/SummitAttendeeAuditLogFormatter.phpapp/Audit/Interfaces/IAuditStrategy.phptests/OpenTelemetry/Formatters/AllFormattersIntegrationTest.phptests/OpenTelemetry/Formatters/AuditEventListenerTest.phptests/OpenTelemetry/Formatters/DefaultEntityManyToManyCollectionDeleteAuditLogFormatterTest.phptests/OpenTelemetry/Formatters/DefaultEntityManyToManyCollectionUpdateAuditLogFormatterTest.phptests/OpenTelemetry/Formatters/SummitAttendeeAuditLogFormatterManyToManyTest.phptests/OpenTelemetry/Formatters/Support/PersistentCollectionTestHelper.phptests/SummitRSVPServiceTest.php
| } | ||
|
|
||
| //$ui = app()->bound('ui.context') ? app('ui.context') : []; | ||
| $ui = []; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if $ui was previously populated from somewhere
rg -n -B5 -A5 '\$ui\s*=' app/Audit/AuditEventListener.php
# Search for any context that might provide app/flow values
rg -n "uiApp|uiFlow|ui_app|ui_flow" --type php -g '!tests/*'Repository: OpenStackweb/summit-api
Length of output: 1022
🏁 Script executed:
cat -n app/Audit/AuditEventListener.php | sed -n '110,140p'Repository: OpenStackweb/summit-api
Length of output: 1518
Remove unused $ui initialization or populate the array.
The $ui variable is initialized as an empty array at line 114, but it's never populated. Lines 134-135 attempt to access $ui['app'] and $ui['flow'], which will always be null due to the null coalescing operator. Either remove the initialization and use null directly, or add the logic to populate these array keys from the request/route context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Audit/AuditEventListener.php` at line 114, The $ui array is initialized
but never populated, making later accesses to $ui['app'] and $ui['flow'] always
null; update the AuditEventListener code to either remove the unused $ui
initialization and use null/defaults directly where $ui['app']/$ui['flow'] are
read, or populate $ui from the current request/route context (e.g. pull values
from the Request or route attributes) before those reads so $ui['app'] and
$ui['flow'] contain the intended values; ensure you update the same method in
AuditEventListener where $ui is declared and where $ui['app']/$ui['flow'] are
accessed.
| private function fetchManyToManyIds(PersistentCollection $collection, $uow): ?array | ||
| { | ||
| try { | ||
|
|
||
| $mapping = $collection->getMapping(); | ||
|
|
||
| $joinTable = $mapping->joinTable ?? null; | ||
| $joinTableName = is_array($joinTable) ? ($joinTable['name'] ?? null) : ($joinTable->name ?? null); | ||
| $joinColumns = is_array($joinTable) ? ($joinTable['joinColumns'] ?? []) : ($joinTable->joinColumns ?? []); | ||
| $inverseJoinColumns = is_array($joinTable) ? ($joinTable['inverseJoinColumns'] ?? []) : ($joinTable->inverseJoinColumns ?? []); | ||
|
|
||
| $joinColumn = $joinColumns[0] ?? null; | ||
| $inverseJoinColumn = $inverseJoinColumns[0] ?? null; | ||
| $joinColumnName = is_object($joinColumn) ? ($joinColumn->name ?? null) : ($joinColumn['name'] ?? null); | ||
| $inverseColumnName = is_object($inverseJoinColumn) ? ($inverseJoinColumn->name ?? null) : ($inverseJoinColumn['name'] ?? null); | ||
| if (!$joinTableName) { | ||
| Log::debug("AuditEventListener::fetchManyToManyIds - no join table name found"); | ||
| return null; | ||
| } | ||
|
|
||
| $owner = $collection->getOwner(); | ||
| if ($owner === null) { | ||
| return null; | ||
| } | ||
|
|
||
| $ownerMeta = $this->em->getClassMetadata(get_class($owner)); | ||
| $ownerIds = $ownerMeta->getIdentifierValues($owner); | ||
| if (empty($ownerIds)) { | ||
| Log::debug("AuditEventListener::fetchManyToManyIds - owner IDs are empty"); | ||
| return null; | ||
| } | ||
| $ownerId = reset($ownerIds); | ||
|
|
||
| if (empty($joinColumns) || empty($inverseJoinColumns)) { | ||
| Log::debug("AuditEventListener::fetchManyToManyIds - join or inverse columns are empty"); | ||
| return null; | ||
| } | ||
|
|
||
| Log::debug("AuditEventListener::fetchManyToManyIds - column names", [ | ||
| 'joinColumnName' => $joinColumnName, | ||
| 'inverseColumnName' => $inverseColumnName | ||
| ]); | ||
|
|
||
| if (!$joinColumnName || !$inverseColumnName) { | ||
| Log::debug("AuditEventListener::fetchManyToManyIds - column names are missing"); | ||
| return null; | ||
| } | ||
|
|
||
| $conn = $this->em->getConnection(); | ||
| $qb = $conn->createQueryBuilder(); | ||
|
|
||
| $qb->select($inverseColumnName) | ||
| ->from($joinTableName) | ||
| ->where($joinColumnName . ' = :ownerId') | ||
| ->setParameter('ownerId', $ownerId); | ||
|
|
||
| $ids = $qb->fetchFirstColumn(); | ||
|
|
||
| $result = !empty($ids) ? array_map('intval', $ids) : []; | ||
|
|
||
| return $result; | ||
|
|
||
| } catch (\Exception $e) { | ||
| Log::error("AuditEventListener::fetchManyToManyIds error: " . $e->getMessage(), [ | ||
| 'exception' => get_class($e), | ||
| 'trace' => $e->getTraceAsString() | ||
| ]); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove unused $uow parameter.
Static analysis correctly identifies that the $uow parameter is never used in fetchManyToManyIds(). The method uses $this->em for all EntityManager operations.
🐛 Proposed fix
- private function fetchManyToManyIds(PersistentCollection $collection, $uow): ?array
+ private function fetchManyToManyIds(PersistentCollection $collection): ?array
{Also update the call sites at lines 196 and 294:
- $deletedIds = $this->fetchManyToManyIds($subject, $uow);
+ $deletedIds = $this->fetchManyToManyIds($subject);- $allIds = $this->fetchManyToManyIds($collection, $uow);
+ $allIds = $this->fetchManyToManyIds($collection);🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 216-216: Avoid unused parameters such as '$uow'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Audit/AuditEventListener.php` around lines 216 - 285, Remove the unused
$uow parameter from the fetchManyToManyIds method signature and any related
docblock, then update all call sites that pass $uow into fetchManyToManyIds to
call it without that argument; specifically edit the
AuditEventListener::fetchManyToManyIds declaration to drop $uow and modify the
internal callers within the class to call fetchManyToManyIds($collection)
instead of passing the unused $uow. Ensure nothing else relies on the removed
parameter and run static analysis/tests to confirm no remaining references.
| if ($this->child_entity_formatter != null) { | ||
| foreach ($insertDiff as $child_changed_entity) { | ||
| $changes[] = $this->child_entity_formatter | ||
| ->format($child_changed_entity, IChildEntityAuditLogFormatter::CHILD_ENTITY_CREATION); | ||
| } | ||
|
|
||
| foreach ($deleteDiff as $child_changed_entity) { | ||
| $changes[] = $this->child_entity_formatter | ||
| ->format($child_changed_entity, IChildEntityAuditLogFormatter::CHILD_ENTITY_DELETION); | ||
| } | ||
|
|
||
| if (!empty($changes)) { | ||
| return implode("|", $changes); | ||
| } |
There was a problem hiding this comment.
Null values may be added to changes array.
Unlike the delete formatter (which filters out null results at line 108), this formatter adds all results from the child formatter to the $changes array without checking for null values. This could result in null entries in the output string.
🐛 Proposed fix
if ($this->child_entity_formatter != null) {
foreach ($insertDiff as $child_changed_entity) {
- $changes[] = $this->child_entity_formatter
+ $formatted = $this->child_entity_formatter
->format($child_changed_entity, IChildEntityAuditLogFormatter::CHILD_ENTITY_CREATION);
+ if ($formatted !== null) {
+ $changes[] = $formatted;
+ }
}
foreach ($deleteDiff as $child_changed_entity) {
- $changes[] = $this->child_entity_formatter
+ $formatted = $this->child_entity_formatter
->format($child_changed_entity, IChildEntityAuditLogFormatter::CHILD_ENTITY_DELETION);
+ if ($formatted !== null) {
+ $changes[] = $formatted;
+ }
}
if (!empty($changes)) {
return implode("|", $changes);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ($this->child_entity_formatter != null) { | |
| foreach ($insertDiff as $child_changed_entity) { | |
| $changes[] = $this->child_entity_formatter | |
| ->format($child_changed_entity, IChildEntityAuditLogFormatter::CHILD_ENTITY_CREATION); | |
| } | |
| foreach ($deleteDiff as $child_changed_entity) { | |
| $changes[] = $this->child_entity_formatter | |
| ->format($child_changed_entity, IChildEntityAuditLogFormatter::CHILD_ENTITY_DELETION); | |
| } | |
| if (!empty($changes)) { | |
| return implode("|", $changes); | |
| } | |
| if ($this->child_entity_formatter != null) { | |
| foreach ($insertDiff as $child_changed_entity) { | |
| $formatted = $this->child_entity_formatter | |
| ->format($child_changed_entity, IChildEntityAuditLogFormatter::CHILD_ENTITY_CREATION); | |
| if ($formatted !== null) { | |
| $changes[] = $formatted; | |
| } | |
| } | |
| foreach ($deleteDiff as $child_changed_entity) { | |
| $formatted = $this->child_entity_formatter | |
| ->format($child_changed_entity, IChildEntityAuditLogFormatter::CHILD_ENTITY_DELETION); | |
| if ($formatted !== null) { | |
| $changes[] = $formatted; | |
| } | |
| } | |
| if (!empty($changes)) { | |
| return implode("|", $changes); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/Audit/ConcreteFormatters/DefaultEntityManyToManyCollectionUpdateAuditLogFormatter.php`
around lines 96 - 109, The child-entity creation loop is pushing potentially
null returns from $this->child_entity_formatter->format(...) into $changes,
causing null entries in the final string; update the creation loop (the foreach
over $insertDiff calling format with
IChildEntityAuditLogFormatter::CHILD_ENTITY_CREATION) to only append non-null
results (e.g., check the formatter return is not null before $changes[] = ...)
or run an array_filter on $changes before the implode, mirroring the
null-filtering behavior used for deletions (format(..., CHILD_ENTITY_DELETION)).
| } catch (\Throwable $ex) { | ||
| Log::warning("SummitAttendeeAuditLogFormatter::formatManyToManyUpdate error: " . $ex->getMessage()); | ||
| return sprintf("Attendee (%s) '%s' association updated by user %s", $id, $name, $this->getUserInfo()); | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling between update and delete formatters.
formatManyToManyUpdate returns a fallback message on error (line 111), while formatManyToManyDelete returns null (line 139). This inconsistency could lead to unexpected behavior—updates will always produce an audit log entry, but deletes may silently fail to log.
Consider applying consistent behavior, either both return a fallback message or both return null.
🛠️ Proposed fix (align delete with update behavior)
} catch (\Throwable $ex) {
Log::warning("SummitAttendeeAuditLogFormatter::formatManyToManyDelete error: " . $ex->getMessage());
- return null;
+ return sprintf("Attendee (%s) '%s' association deleted by user %s", $id, $name, $this->getUserInfo());
}Also applies to: 137-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Audit/ConcreteFormatters/SummitAttendeeAuditLogFormatter.php` around
lines 109 - 112, The formatters in SummitAttendeeAuditLogFormatter are
inconsistent: formatManyToManyUpdate catches exceptions and returns a fallback
message, while formatManyToManyDelete returns null; change
formatManyToManyDelete to mirror formatManyToManyUpdate by catching \Throwable
and returning the same formatted fallback string (e.g., using $id, $name and
$this->getUserInfo()) and logging the exception with Log::warning so both update
and delete produce a consistent audit fallback on error.
| public function testFormatWithChildFormatterSkipsNullMessages(): void | ||
| { | ||
| $childFormatter = Mockery::mock(IChildEntityAuditLogFormatter::class); | ||
| $childFormatter | ||
| ->shouldReceive('format') | ||
| ->twice() | ||
| ->andReturn(null, 'child-delete'); | ||
|
|
||
| $formatterWithChild = new DefaultEntityManyToManyCollectionUpdateAuditLogFormatter($childFormatter); | ||
| $formatterWithChild->setContext(AuditContextBuilder::default()->build()); | ||
|
|
||
| $collection = $this->makeCollection(self::SNAPSHOT_IDS_UPDATE, self::CURRENT_IDS_UPDATE); | ||
| $result = $formatterWithChild->format(new \stdClass(), ['collection' => $collection]); | ||
|
|
||
| $this->assertSame('|child-delete', $result); |
There was a problem hiding this comment.
Don't lock in the leading separator bug.
This expectation codifies the current |child-delete artifact when one child formatter returns null. The delete formatter tests already treat null child output as skippable, so the update contract should do the same instead of preserving an empty segment.
🛠️ Proposed fix
- $this->assertSame('|child-delete', $result);
+ $this->assertSame('child-delete', $result);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testFormatWithChildFormatterSkipsNullMessages(): void | |
| { | |
| $childFormatter = Mockery::mock(IChildEntityAuditLogFormatter::class); | |
| $childFormatter | |
| ->shouldReceive('format') | |
| ->twice() | |
| ->andReturn(null, 'child-delete'); | |
| $formatterWithChild = new DefaultEntityManyToManyCollectionUpdateAuditLogFormatter($childFormatter); | |
| $formatterWithChild->setContext(AuditContextBuilder::default()->build()); | |
| $collection = $this->makeCollection(self::SNAPSHOT_IDS_UPDATE, self::CURRENT_IDS_UPDATE); | |
| $result = $formatterWithChild->format(new \stdClass(), ['collection' => $collection]); | |
| $this->assertSame('|child-delete', $result); | |
| public function testFormatWithChildFormatterSkipsNullMessages(): void | |
| { | |
| $childFormatter = Mockery::mock(IChildEntityAuditLogFormatter::class); | |
| $childFormatter | |
| ->shouldReceive('format') | |
| ->twice() | |
| ->andReturn(null, 'child-delete'); | |
| $formatterWithChild = new DefaultEntityManyToManyCollectionUpdateAuditLogFormatter($childFormatter); | |
| $formatterWithChild->setContext(AuditContextBuilder::default()->build()); | |
| $collection = $this->makeCollection(self::SNAPSHOT_IDS_UPDATE, self::CURRENT_IDS_UPDATE); | |
| $result = $formatterWithChild->format(new \stdClass(), ['collection' => $collection]); | |
| $this->assertSame('child-delete', $result); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/OpenTelemetry/Formatters/DefaultEntityManyToManyCollectionUpdateAuditLogFormatterTest.php`
around lines 250 - 264, The current formatter preserves an empty segment when a
child formatter returns null, causing a leading '|' in
DefaultEntityManyToManyCollectionUpdateAuditLogFormatter output; update the
format method in DefaultEntityManyToManyCollectionUpdateAuditLogFormatter so it
collects child outputs from the IChildEntityAuditLogFormatter->format calls,
filters out null/empty values, and then joins the remaining pieces with the '|'
separator (only inserting separators between non-empty segments) so null child
results are skipped and no leading separator is emitted.
| #[DataProvider('providesNullCasesForManyToMany')] | ||
| public function testManyToManyReturnsNullWithoutRequiredContextOrCollection( | ||
| string $eventType, | ||
| bool $withContext | ||
| ): void { | ||
| $formatter = $this->makeFormatter($eventType, $withContext); | ||
| $attendee = $this->makeAttendee(); | ||
|
|
||
| $result = $formatter->format($attendee, []); | ||
| $this->assertNull($result); |
There was a problem hiding this comment.
The "without context" cases never hit the no-context path.
Every provider row calls format($attendee, []), so handleManyToManyCollection() returns on missing collection before context is touched. The DP_*_WITHOUT_CONTEXT rows are therefore duplicates of the "without collection" scenario and won't catch a regression when a collection is present but context is missing.
🛠️ Proposed fix
public function testManyToManyReturnsNullWithoutRequiredContextOrCollection(
string $eventType,
- bool $withContext
+ bool $withContext,
+ bool $withCollection
): void {
$formatter = $this->makeFormatter($eventType, $withContext);
$attendee = $this->makeAttendee();
- $result = $formatter->format($attendee, []);
+ $changeSet = $withCollection
+ ? ['collection' => $this->makeCollection(self::SNAPSHOT_IDS_EMPTY, self::CURRENT_IDS_EMPTY)]
+ : [];
+
+ $result = $formatter->format($attendee, $changeSet);
$this->assertNull($result);
}
@@
return [
- self::DP_UPDATE_WITHOUT_CONTEXT => [IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_UPDATE, false],
- self::DP_DELETE_WITHOUT_CONTEXT => [IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_DELETE, false],
- self::DP_UPDATE_WITHOUT_COLLECTION => [IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_UPDATE, true],
- self::DP_DELETE_WITHOUT_COLLECTION => [IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_DELETE, true],
+ self::DP_UPDATE_WITHOUT_CONTEXT => [IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_UPDATE, false, true],
+ self::DP_DELETE_WITHOUT_CONTEXT => [IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_DELETE, false, true],
+ self::DP_UPDATE_WITHOUT_COLLECTION => [IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_UPDATE, true, false],
+ self::DP_DELETE_WITHOUT_COLLECTION => [IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_DELETE, true, false],
];
}Also applies to: 203-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/OpenTelemetry/Formatters/SummitAttendeeAuditLogFormatterManyToManyTest.php`
around lines 70 - 79, The test ManyToMany null-case provider currently always
calls format($attendee, []) so handleManyToManyCollection() exits on missing
collection and the DP_*_WITHOUT_CONTEXT rows never exercise the "collection
present but context missing" branch; update the DataProvider
'providesNullCasesForManyToMany' rows labeled DP_*_WITHOUT_CONTEXT to pass an
input array that includes a valid collection key (e.g. ['collection' => <dummy
collection value>]) but omits the required context key so
testManyToManyReturnsNullWithoutRequiredContextOrCollection (and the duplicate
cases around lines 203-210) will hit the no-context path in
handleManyToManyCollection().
ref: https://app.clickup.com/t/86b84w9x2
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests