Skip to content

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

Open
andrestejerina97 wants to merge 2 commits intomainfrom
feature/new-event-for-formatters
Open

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

Conversation

@andrestejerina97
Copy link
Contributor

@andrestejerina97 andrestejerina97 commented Feb 11, 2026

ref: https://app.clickup.com/t/86b84w9x2

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced audit logging now tracks many-to-many relationship changes (additions and removals) with clearer, more detailed messages for better visibility into data modifications.
  • Bug Fixes

    • Improved error handling in relationship change tracking to ensure all modifications are properly recorded in audit logs.
  • Tests

    • Expanded test coverage for relationship auditing scenarios to ensure reliability.

@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
@andrestejerina97 andrestejerina97 force-pushed the feature/new-event-for-formatters branch 2 times, most recently from 02a1841 to be8a5f1 Compare March 5, 2026 17:21
@smarcet
Copy link
Collaborator

smarcet commented Mar 5, 2026

@andrestejerina97 please review unit tests https://github.com/OpenStackweb/summit-api/actions/runs/22728527050/job/65910842285?pr=498#step:5:68

@andrestejerina97 andrestejerina97 force-pushed the feature/new-event-for-formatters branch from be8a5f1 to 6b78204 Compare March 5, 2026 18:55
@andrestejerina97 andrestejerina97 force-pushed the feature/new-event-for-formatters branch from 6b78204 to 9d27bf5 Compare March 5, 2026 18:55
@andrestejerina97
Copy link
Contributor Author

@caseylocker it's ready to review

@smarcet smarcet force-pushed the feature/new-event-for-formatters branch from 1d0e92b to b3d0359 Compare March 6, 2026 13:02
@andrestejerina97 andrestejerina97 force-pushed the feature/new-event-for-formatters branch from b3d0359 to 1e069fd Compare March 11, 2026 14:20
@coderabbitai
Copy link

coderabbitai bot commented Mar 11, 2026

📝 Walkthrough

Walkthrough

This 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

Cohort / File(s) Summary
Core Audit Infrastructure
app/Audit/Interfaces/IAuditStrategy.php
Added four new public constants: EVENT\_COLLECTION\_MANYTOMANY\_UPDATE, EVENT\_COLLECTION\_MANYTOMANY\_DELETE, ACTION\_COLLECTION\_MANYTOMANY\_UPDATE, and ACTION\_COLLECTION\_MANYTOMANY\_DELETE to support many-to-many collection events.
Base Formatter Enhancements
app/Audit/AbstractAuditLogFormatter.php
Added protected helpers processCollection, extractCollectionEntityIds, buildManyToManyDetailedMessage, and static formatManyToManyDetailedMessage to extract and format collection changes with metadata (field name, target entity, added/removed IDs). Added imports for PersistentCollection and Log.
Event Listener Updates
app/Audit/AuditEventListener.php
Centralized entity manager as class property (\$em), added auditCollection helper to process collection changes and determine event type, and helpers fetchManyToManyIds and fetchDeletedIdsFromInitializedCollection to compute affected IDs for many-to-many relationships. Expanded onFlush to delegate collection auditing.
Strategy & Factory Updates
app/Audit/AuditLogFormatterFactory.php, app/Audit/AuditLogOtlpStrategy.php
Factory now routes EVENT\_COLLECTION\_MANYTOMANY\_UPDATE and EVENT\_COLLECTION\_MANYTOMANY\_DELETE to new default formatters with context fallback. Strategy extended to record collection type, counts, and dirty state for many-to-many events; refactored getCollectionType for better error messaging.
New Formatter Implementations
app/Audit/ConcreteFormatters/DefaultEntityManyToManyCollectionUpdateAuditLogFormatter.php, app/Audit/ConcreteFormatters/DefaultEntityManyToManyCollectionDeleteAuditLogFormatter.php
Two new concrete formatter classes implementing format() to produce detailed or delegated child-formatter messages for many-to-many collection updates and deletions, with error handling and fallback logic.
Existing Formatter Extensions
app/Audit/ConcreteFormatters/SummitAttendeeAuditLogFormatter.php
Added switch branches for EVENT\_COLLECTION\_MANYTOMANY\_UPDATE and EVENT\_COLLECTION\_MANYTOMANY\_DELETE delegating to new handleManyToManyCollection, formatManyToManyUpdate, and formatManyToManyDelete helpers. Updated exception handling to catch Throwable.
Test Infrastructure
tests/OpenTelemetry/Formatters/Support/PersistentCollectionTestHelper.php
New test helper class providing buildManyToManyCollection() to construct Doctrine PersistentCollection instances with snapshot and current state for many-to-many association testing.
Comprehensive Test Coverage
tests/OpenTelemetry/Formatters/AuditEventListenerTest.php, tests/OpenTelemetry/Formatters/DefaultEntityManyToManyCollectionUpdateAuditLogFormatterTest.php, tests/OpenTelemetry/Formatters/DefaultEntityManyToManyCollectionDeleteAuditLogFormatterTest.php, tests/OpenTelemetry/Formatters/SummitAttendeeAuditLogFormatterManyToManyTest.php, tests/OpenTelemetry/Formatters/AllFormattersIntegrationTest.php
Added test suites covering AuditEventListener collection auditing, both new formatter classes with various diff scenarios and child formatter interactions, SummitAttendee many-to-many handling, and integration tests including new event types. Uses Mockery and reflection for private method access.
Test Migration
tests/SummitRSVPServiceTest.php
Migrated PHPUnit coverage annotation from docblock to PHP 8 Attributes syntax using CoversClass attribute.

Sequence Diagram

sequenceDiagram
    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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Poem

🐰 Hop, hop! Collections now dance true,
Many-to-many changes we track anew,
With formatters wise and listeners keen,
The finest audit logs you've seen!
No rabbit's burrow could be more neat,
This audit system's quite the treat! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.45% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title directly and clearly describes the main feature being added: support for many-to-many relationship events in the audit system.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/new-event-for-formatters

Comment @coderabbitai help to get the list of available commands and usage tips.

@andrestejerina97
Copy link
Contributor Author

@martinquiroga-exo ready to review

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 | 🟠 Major

Fail on unsupported event types.

$unsupported is populated but never asserted. Any formatter that still rejects EVENT_COLLECTION_MANYTOMANY_UPDATE or EVENT_COLLECTION_MANYTOMANY_DELETE through 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 $subject parameter.

The $subject parameter is declared but never used within handleManyToManyCollection. Since $id and $name are 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 deletedIds array 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 $addedIds and $removedIds are empty arrays, formatManyToManyDetailedMessage will 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 via fetchDeletedIdsFromInitializedCollection(), but this file only exercises the non-many-to-many branch and the raw ID fetch helper. That leaves the new diff-based deleted_ids payload untested at the listener level. PersistentCollectionTestHelper should 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

📥 Commits

Reviewing files that changed from the base of the PR and between 257cc38 and 1e069fd.

📒 Files selected for processing (15)
  • app/Audit/AbstractAuditLogFormatter.php
  • app/Audit/AuditEventListener.php
  • app/Audit/AuditLogFormatterFactory.php
  • app/Audit/AuditLogOtlpStrategy.php
  • app/Audit/ConcreteFormatters/DefaultEntityManyToManyCollectionDeleteAuditLogFormatter.php
  • app/Audit/ConcreteFormatters/DefaultEntityManyToManyCollectionUpdateAuditLogFormatter.php
  • app/Audit/ConcreteFormatters/SummitAttendeeAuditLogFormatter.php
  • app/Audit/Interfaces/IAuditStrategy.php
  • tests/OpenTelemetry/Formatters/AllFormattersIntegrationTest.php
  • tests/OpenTelemetry/Formatters/AuditEventListenerTest.php
  • tests/OpenTelemetry/Formatters/DefaultEntityManyToManyCollectionDeleteAuditLogFormatterTest.php
  • tests/OpenTelemetry/Formatters/DefaultEntityManyToManyCollectionUpdateAuditLogFormatterTest.php
  • tests/OpenTelemetry/Formatters/SummitAttendeeAuditLogFormatterManyToManyTest.php
  • tests/OpenTelemetry/Formatters/Support/PersistentCollectionTestHelper.php
  • tests/SummitRSVPServiceTest.php

}

//$ui = app()->bound('ui.context') ? app('ui.context') : [];
$ui = [];
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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

Comment on lines +216 to +285
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;
}
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Comment on lines +96 to +109
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);
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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)).

Comment on lines +109 to +112
} catch (\Throwable $ex) {
Log::warning("SummitAttendeeAuditLogFormatter::formatManyToManyUpdate error: " . $ex->getMessage());
return sprintf("Attendee (%s) '%s' association updated by user %s", $id, $name, $this->getUserInfo());
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Comment on lines +250 to +264
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);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.

Comment on lines +70 to +79
#[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);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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().

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.

4 participants