fix(DynamicContentSubscriber): complete Mautic 5 migration of filter evaluation by edouard-mangel · Pull Request #381 · acquia/mc-cs-plugin-custom-objects

edouard-mangel · 2026-03-09T10:26:37Z

Context

This PR fixes a fatal TypeError that occurs when evaluating Dynamic Web Content with custom object filters in Mautic 5.

Note: This replaces #380, which was accidentally opened from WebAnyOne:staging (346 commits, 382 files changed). This PR is a single focused change.

Root cause

DynamicContentSubscriber used DbalQueryTrait::executeSelect() which expects a Doctrine\DBAL\Query\QueryBuilder. But QueryFilterFactory::configureQueryBuilderFromSegmentFilter() can return a UnionQueryContainer for custom field filters — causing a fatal TypeError at runtime.

The branch also had three diverging copies of matchFilterForLead() logic (polyfill trait, TokenSubscriber, ContactFilterMatcher), none of which correctly handled multi-value custom object fields where $lead['cmf_X'] is an array of values (one per linked item).

Changes

`Helper/FilterEvaluator` (new service)

Standalone service owning all DWC filter evaluation logic:

Groups filters by AND/OR glue
For custom_object fields: iterates the array of linked item values — a filter matches if any item satisfies the condition
All operators: =, !=, gt, gte, lt, lte, empty, !empty, like, !like, in, !in, regexp, !regexp, startsWith, endsWith, contains
Type coercions: boolean, datetime/time, tags/select/multiselect, number/int
Bug fixed: previous any-match loop broke on the first non-matching item, preventing evaluation of subsequent linked items

`Helper/ContactFilterMatcher`

Removed MatchFilterForLeadTraitPolyfill and dead transformFilterDataForLead()
Removed LeadListRepository dependency (was only used by the now-deleted trait)
Injects FilterEvaluator and delegates evaluation to it

`EventListener/DynamicContentSubscriber`

Wires ContactFilterMatcher::match() as the delegate in evaluateFilters()
Passes array_merge(['id' => $contact->getId()], $contact->getProfileFields()) — getProfileFields() does not include the entity id

`EventListener/TokenSubscriber`

Removed local matchFilterForLeadInCustomObject() — delegates to injected FilterEvaluator

`Polyfill/EventListener/MatchFilterForLeadTrait` — deleted

`phpstan-baseline.neon`

Removed 5 dead-code suppressions that were added by the previous iteration

Tests

`Tests/Unit/Helper/FilterEvaluatorTest` (new — 36 tests, no DB)

All 14 operators with match and no-match assertions
All type coercions (boolean, datetime, multiselect, int/number)
AND/OR group logic — both directions (first group passes, first group fails)
Custom object any-match semantics, scalar normalisation, empty array edge cases
Guard conditions (missing id, empty filter list, unknown field)
OperatorsNotFoundException for unknown operators

`Tests/Functional/EventListener/DynamicContentSubscriberTest` (rewritten — 10 tests, MySQL)

testVariousOperators — all string/date operators against real DB data (20+ assertions)
testAndFiltersAllMustMatch / testAndFiltersOneFailsNoMatch
testOrFiltersSecondGroupMatches / testOrFiltersFirstGroupPassesSecondFails
testMultipleLinkedItemsAnyMatchWins
testEmptyOperatorMatchesContactWithNoLinkedItems
testRegexpOperator
testNumberFieldComparisons — gt/gte/lt/lte/= on int fields
testAlreadyEvaluatedEventIsSkipped
testEvaluateFiltersSkipsEventWithNoCustomObjectFilters

Verification

CI passes on PHP 8.0 / MySQL / Mautic 5.1: https://github.com/WebAnyOne/mc-cs-plugin-custom-objects/actions/runs/24459717499

Copilot

Pull request overview

This PR completes the Mautic 5 migration for dynamic content filter evaluation by removing Mautic 4-era trait/query-builder logic from DynamicContentSubscriber and delegating evaluation to a matcher, with updated unit test coverage and an ADR documenting the migration approach.

Changes:

Refactors DynamicContentSubscriber::evaluateFilters() to remove trait/DBAL-based evaluation and delegate matching to a ContactFilterMatcher.
Rewrites DynamicContentSubscriberTest to cover the new collaborator-driven behavior and updated early-exit conditions.
Adds an ADR documenting the iterative migration strategy and local DDEV constraints.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
`EventListener/DynamicContentSubscriber.php`	Removes old trait-based evaluation and routes dynamic content filter evaluation through a matcher + CO-filter detection.
`Tests/Unit/EventListener/DynamicContentSubscriberTest.php`	Updates unit tests to reflect the new control flow and collaborator usage.
`docs/adr/0001-mautic5-migration-strategy.md`	Documents migration decisions, rationale, and DDEV/symlink constraints.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

…evaluation The 5.x branch left DynamicContentSubscriber in the old Mautic 4 state: traits (MatchFilterForLeadTrait, DbalQueryTrait) that no longer exist in mautic/core-lib ^5.0, and a QueryBuilder-based filter evaluation path. Changes: - Remove MatchFilterForLeadTrait and DbalQueryTrait (removed from core in 5.x) - Remove QueryFilterHelper and LoggerInterface from constructor - Wire ContactFilterMatcher as the delegate in evaluateFilters() - Simplify hasCustomObjectFilters() — returns true on first non-throwing filter - Rewrite DynamicContentSubscriberTest: correct collaborators, 6 tests / 21 assertions - Add docs/adr/0001-mautic5-migration-strategy.md Verified on Mautic 5.2.9 / Symfony 5.4 / PHP 8.3 via DDEV: - cache:clear succeeds - mautic:plugins:reload installs the plugin (13 DB tables created) - Custom Objects UI fully functional at /s/custom/object/ Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…ill trait directly The class_alias trick (selecting between Mautic core MatchFilterForLeadTrait and the polyfill at runtime) is not statically analysable by PHPStan, causing 8 false-positive errors. Since this branch targets Mautic 5 where the core trait no longer has transformFilterDataForLead(), the polyfill branch always executes — making the class_alias logic dead code. - Remove class_alias block and Mautic\EmailBundle\EventListener\MatchFilterForLeadTrait import - Use MatchFilterForLeadTraitPolyfill directly with a named alias - Add 2 legitimate PHPStan baseline entries (MAUTIC_TABLE_PREFIX global constant and transformFilterDataForLead trait-override detection limitation) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

escopecz

Can you mention in the PR description what error is this fixing? I'm not sure I understand the changes. The error message would help.

DynamicContentSubscriber and ContactFilterMatcher are already discovered by Config/services.php via ->load(). Add ->bind() for the scalar $leadCustomItemFetchLimit parameter so autowiring can resolve it without explicit config.php entries. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Replace global ->bind() in services.php defaults with a per-service #[Autowire(param: ...)] attribute on ContactFilterMatcher, following the pattern used in Mautic 5 core (JsController, PluginDatabase, etc.).

Covers: no CO filters, NotFoundException/InvalidCustomObjectFormatListException handling, no linked items, name match/mismatch, multi-item match, custom item caching per object+lead, company enrichment, tag enrichment.

…cuteQuery()

…ems linked When a contact has no linked custom items, the lead value array is empty and the foreach never executes, leaving the group result as false. For the 'empty' operator this is semantically wrong — no items means the field IS empty, so the condition should match.

…ing in services.php The #[Autowire] attribute is not reliably processed in Mautic 5.1 environments, causing a container compilation failure. Use a specific service definition with ->arg() in services.php instead — this is scoped to ContactFilterMatcher only and works across all supported Mautic/Symfony versions.

…rsection type

… in bootstrap)

…ll return type

… non-capturing catches)

…ataForLead methods

edouard-mangel · 2026-03-14T15:06:27Z

Hi @escopecz — just a heads-up that the PR description has been updated to reflect the accurate root cause (the TypeError from executeSelect() receiving a UnionQueryContainer instead of a QueryBuilder, not a removed trait). Happy to clarify anything further if needed. Thanks for the thorough review!

acquia-stalebot-platauto · 2026-04-06T08:38:16Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please remove the stale label to avoid it being closed. Thank you for your contributions. More info: https://github.com/acquia/devops-github-administration/blob/main/docs/operations_related_to_repositories.md#acquia-stale-bot

edouard-mangel · 2026-04-10T09:58:25Z

Hello Team :)

I'm sorry, I got the notification that this MR is stale. Is there any modification still needed ? I had the impression that I had done all you asked.

Do you need anything from me ?

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-10T10:49:18Z

+        $event->stopPropagation();
+        $event->setIsMatched($this->contactFilterMatcher->match(
+            $event->getFilters(),
+            $event->getContact()->getProfileFields()


ContactFilterMatcher::match() expects the lead array to contain an id (it immediately reads $lead['id']). Lead::getProfileFields() typically does not include the entity id, so this call risks an undefined-index notice (and subsequent fatal errors) during dynamic content evaluation. Pass the id explicitly (e.g., merge ['id' => $event->getContact()->getId()] into the profile fields) so matching and tag/company preloading can work reliably.

Suggested change

$event->getContact()->getProfileFields()

array_merge(

['id' => $event->getContact()->getId()],

$event->getContact()->getProfileFields()

)

Copilot · 2026-04-10T10:49:18Z

+    public function match(array $filters, array $lead, bool &$hasCustomFields = false): bool
+    {
+        $leadId            = (string) $lead['id'];
+        $customFieldValues = $this->getCustomFieldDataForLead($filters, $leadId);
+
+        if (!$customFieldValues) {
+            return false;
+        }


match() reads $lead['id'] unconditionally. If the lead array comes from profile fields/preview data and lacks an id, this will raise an undefined-index notice and can break filter evaluation. Add an early guard (similar to the trait’s empty($lead['id']) check) and return false when no lead id is available.

Copilot · 2026-04-10T10:49:19Z

+    private function buildFilters(): array
    {
-        return new ContactFiltersEvaluateEvent(
-            [
-                'custom_field_1' => [
-                    'type'          => CustomFieldFilterQueryBuilder::getServiceId(),
-                    'table'         => 'custom_field_text',
-                    'field'         => 'cfwq_1',
-                    'foreign_table' => 'custom_objects',
-                ],
-                'custom_item_1'  => [
-                    'type'          => CustomItemNameFilterQueryBuilder::getServiceId(),
-                    'table'         => 'custom_field_text',
-                    'field'         => 'cowq_2',
-                    'foreign_table' => 'custom_objects',
-                ],
+        return [
+            'custom_field_1' => [
+                'type'          => CustomFieldFilterQueryBuilder::getServiceId(),
+                'table'         => 'custom_field_text',
+                'field'         => 'cfwq_1',
+                'foreign_table' => 'custom_objects',
            ],
-            new Lead()
-        );
+            'custom_item_1' => [
+                'type'          => CustomItemNameFilterQueryBuilder::getServiceId(),
+                'table'         => 'custom_field_text',
+                'field'         => 'cowq_2',
+                'foreign_table' => 'custom_objects',
+            ],
+        ];


buildFilters() constructs filter entries with keys like table/foreign_table/cfwq_1, but ContactFilterMatcher (and the segment matching logic) expects segment-style filters containing keys such as object, field (e.g., cmf_1/cmo_1), operator, filter, and glue (see TokenSubscriber’s matching logic). As written, this unit test can’t catch real integration issues because the filter shape differs from what the matcher can actually process.

Copilot · 2026-04-10T10:49:19Z

+        $this->contactFilterMatcher->expects($this->once())
+            ->method('match')
+            ->with($this->buildFilters(), ['email' => 'test@example.com'])
+            ->willReturn(true);


This expectation asserts that DynamicContentSubscriber calls the matcher with only ['email' => ...]. However, ContactFilterMatcher::match() requires id in the lead array (it uses it to load custom items/tags/companies). Update the test to include the contact id in the lead data passed to match() once the subscriber is fixed to provide it.

Copilot · 2026-04-10T10:49:19Z

+<?php
+
+namespace MauticPlugin\CustomObjectsBundle\Polyfill\EventListener;
+
+use Mautic\LeadBundle\Entity\LeadListRepository;
+use Mautic\LeadBundle\Exception\OperatorsNotFoundException;
+use Mautic\LeadBundle\Segment\OperatorOptions;


This new polyfill file is missing declare(strict_types=1);, while the rest of the codebase appears to consistently enable strict types. Adding it helps keep type behavior consistent across the bundle (especially important here given extensive scalar comparisons/casts).

Copilot · 2026-04-10T10:49:19Z

+    protected function matchFilterForLead(array $filter, array $lead): bool
+    {
+        if (empty($lead['id'])) {
+            // Lead in generated for preview with faked data


Typo in comment: "Lead in generated for preview with faked data" → "Lead is generated for preview with faked data".

Suggested change

// Lead in generated for preview with faked data

// Lead is generated for preview with faked data

escopecz · 2026-04-10T10:49:23Z

Hi, sorry for the delay. We'll need to get some reviewers and testers to this PR. The struggle to perform the quality checks was always a struggle for OSS projects. It's been multiplied in the recent years with AI where writing the code is cheap but assessing the quality isn't something that AI can do so it would be trustworthy.

You can make maintainers to trust your changes more if your changes are covered by functional tests. There is only a unit test in this PR. Also, there shouldn't be any additions to phpstan-baseline.neon as it means we are adding on top of existing tech debt.

I'm still not really sure about duplicating the trait from Mautic. But that would require me to do an extensive research that I cannot spend the time on. I'd look if we could update the trait in Mautic so it would also work for this plugin. Or fire a new event that this plugin would enable to hook into the logic and update it. Because with this duplication if a change is made in the original then it won't be made in this copy and more bugs will happen.

…ith FilterEvaluator service - Add Helper/FilterEvaluator.php: standalone service consolidating three divergent copies of filter evaluation logic (TokenSubscriber, ContactFilterMatcher, and the polyfill trait) into one authoritative implementation with multi-value any-match semantics for custom_object fields - Remove Polyfill/EventListener/MatchFilterForLeadTrait.php (236 lines deleted) - Remove dead transformFilterDataForLead() from ContactFilterMatcher - Inject FilterEvaluator into ContactFilterMatcher and TokenSubscriber - Register ContactFilterMatcher explicitly in config.php to resolve int $leadCustomItemFetchLimit DI argument (excluded from services.php auto-discovery to avoid conflict) - Remove 5 phpstan-baseline suppressions that covered dead code - Replace trivial DynamicContentSubscriberTest with 8 MauticMysqlTestCase functional tests covering operators, AND/OR glue, multi-item any-match, empty operator, and subscriber skip conditions - Update TokenSubscriberTest to include FilterEvaluator mock

…ture

…Matcher in config.php, add class-name aliases in services.php for autowiring

…tch loop - DynamicContentSubscriber: pass array_merge(['id' => $contact->getId()], $contact->getProfileFields()) so ContactFilterMatcher can find 'id' (Lead::getProfileFields() does not include the entity id property) - FilterEvaluator: fix any-match semantics — previous loop broke on first non-match (null !== false → break), preventing evaluation of subsequent linked custom items

…or DynamicContentSubscriber

- Add Tests/Unit/Helper/FilterEvaluatorTest: 36 tests covering all operators, type coercions, AND/OR group logic, custom-object any-match semantics, and edge cases (missing id, empty filters, scalar-to-array normalisation) - DynamicContentSubscriberTest: add testRegexpOperator, testNumberFieldComparisons, testOrFiltersFirstGroupPassesSecondFails - DynamicContentSubscriberTest: simplify testAlreadyEvaluatedEventIsSkipped — removes unnecessary DB fixture creation (subscriber bails out before DB access)

…mber)

…unctional test - FilterEvaluator: add 'int' case alongside 'number' in coerceTypes switch (CustomObjectsBundle IntType uses key 'int', Mautic lead fields use 'number') - Functional test: use 'int' as the type for int field filters (matches the actual type key registered by IntType, which QueryFilterFactory validates) - Unit test: add 'int' coercion assertion alongside existing 'number' test

edouard-mangel · 2026-04-15T15:31:34Z

Hi @escopecz — thank you for the detailed feedback, and for taking the time despite being stretched on reviewer capacity. I really appreciate the transparency about the challenges OSS maintenance brings, especially in the current AI-assisted landscape.

I've addressed all three concerns:

1. Functional tests — The PR now includes a full MauticMysqlTestCase functional test suite (Tests/Functional/EventListener/DynamicContentSubscriberTest) with 10 tests hitting a real MySQL database: all string/date operators, AND/OR group logic (both directions), multi-item any-match semantics, regexp, number field comparisons, and the empty-operator edge case. There is also a new Tests/Unit/Helper/FilterEvaluatorTest with 36 unit tests covering every operator and type coercion in isolation.

2. phpstan-baseline additions — The 5 baseline suppressions from the previous iteration have been removed. The baseline is now smaller than before this PR.

3. Trait duplication — The polyfill trait has been deleted entirely. It is replaced by a standalone Helper/FilterEvaluator service with no inheritance from Mautic's trait. The evaluator is ~190 lines of self-contained logic with no coupling to Mautic internals, so there is no drift risk.

The fix for the TypeError (passing a UnionQueryContainer where a QueryBuilder was expected) itself didn't require the trait at all — the trait was pre-existing tech debt that I removed as part of the cleanup.

CI is green on PHP 8.0 / MySQL / Mautic 5.1: https://github.com/WebAnyOne/mc-cs-plugin-custom-objects/actions/runs/24459717499

Happy to make any further adjustments, answer questions, or provide additional context to help reviewers gain confidence in the change. Let me know what would be most useful.

acquia-stalebot-platauto · 2026-05-01T03:25:31Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please remove the stale label to avoid it being closed. Thank you for your contributions. More info: https://github.com/acquia/devops-github-administration/blob/main/docs/operations_related_to_repositories.md#acquia-stale-bot

Copilot AI review requested due to automatic review settings March 9, 2026 10:26

Copilot started reviewing on behalf of edouard-mangel March 9, 2026 10:27 View session

Copilot AI reviewed Mar 9, 2026

View reviewed changes

escopecz reviewed Mar 9, 2026

View reviewed changes

Comment thread docs/adr/0001-mautic5-migration-strategy.md Outdated

edouard-mangel force-pushed the fix/dynamic-content-subscriber-mautic5 branch from a331ffe to f49fc5d Compare March 9, 2026 11:42

escopecz requested changes Mar 10, 2026

View reviewed changes

Comment thread Config/config.php Outdated

Comment thread Polyfill/EventListener/MatchFilterForLeadTrait.php Outdated

escopecz reviewed Mar 11, 2026

View reviewed changes

Comment thread Config/services.php Outdated

edouard-mangel added 11 commits March 11, 2026 14:47

refactor(ContactFilterMatcher): use #[Autowire] for scalar parameter

e49bc8d

Replace global ->bind() in services.php defaults with a per-service #[Autowire(param: ...)] attribute on ContactFilterMatcher, following the pattern used in Mautic 5 core (JsController, PluginDatabase, etc.).

test(ContactFilterMatcher): add unit tests

442f0af

Covers: no CO filters, NotFoundException/InvalidCustomObjectFormatListException handling, no linked items, name match/mismatch, multi-item match, custom item caching per object+lead, company enrichment, tag enrichment.

fix(ContactFilterMatcher): replace deprecated DBAL execute() with exe…

3a9335a

…cuteQuery()

ci: add fix/** branch trigger and workflow_dispatch for local testing

57cf15a

fix(cs): remove extra blank line, extra parentheses, and PHP 8.1 inte…

4878ed9

…rsection type

fix(tests): remove MAUTIC_TABLE_PREFIX define from test file (defined…

ecd58ed

… in bootstrap)

fix(phpstan): add baseline entry for transformFilterDataForLeadPolyfi…

c7302cb

…ll return type

refactor: apply Rector rules (constructor promotion, str_starts_with,…

6002c35

… non-capturing catches)

fix(phpstan): suppress array element type errors for transformFilterD…

99974b5

…ataForLead methods

edouard-mangel requested a review from escopecz March 14, 2026 15:29

acquia-stalebot-platauto Bot added the stale label Apr 6, 2026

escopecz removed the stale label Apr 10, 2026

escopecz requested a review from Copilot April 10, 2026 10:40

Copilot started reviewing on behalf of escopecz April 10, 2026 10:41 View session

Copilot AI reviewed Apr 10, 2026

View reviewed changes

edouard-mangel added 9 commits April 15, 2026 11:58

fix(cs): apply PHP CS Fixer rules for new files and modified classes

528977d

fix(tests): update ContactFilterMatcherTest for new constructor signa…

60fcb53

…ture

fix(di): use string service IDs for FilterEvaluator and ContactFilter…

9fb226e

…Matcher in config.php, add class-name aliases in services.php for autowiring

fix(test): update unit test expectation to include id in lead array f…

803c0cb

…or DynamicContentSubscriber

fix(test): use number-test-field alias for int type (translates to Nu…

b6fdc86

…mber)

acquia-stalebot-platauto Bot added the stale label May 1, 2026

acquia-stalebot-platauto Bot closed this May 8, 2026

escopecz added pinned and removed stale labels May 14, 2026

escopecz reopened this May 14, 2026

	// Lead in generated for preview with faked data
	// Lead is generated for preview with faked data

Conversation

edouard-mangel commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Root cause

Changes

Helper/FilterEvaluator (new service)

Helper/ContactFilterMatcher

EventListener/DynamicContentSubscriber

EventListener/TokenSubscriber

Polyfill/EventListener/MatchFilterForLeadTrait — deleted

phpstan-baseline.neon

Tests

Tests/Unit/Helper/FilterEvaluatorTest (new — 36 tests, no DB)

Tests/Functional/EventListener/DynamicContentSubscriberTest (rewritten — 10 tests, MySQL)

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

escopecz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edouard-mangel commented Mar 14, 2026

Uh oh!

acquia-stalebot-platauto Bot commented Apr 6, 2026

Uh oh!

edouard-mangel commented Apr 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

escopecz commented Apr 10, 2026

Uh oh!

edouard-mangel commented Apr 15, 2026

Uh oh!

acquia-stalebot-platauto Bot commented May 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

edouard-mangel commented Mar 9, 2026 •

edited

Loading

`Helper/FilterEvaluator` (new service)

`Helper/ContactFilterMatcher`

`EventListener/DynamicContentSubscriber`

`EventListener/TokenSubscriber`

`Polyfill/EventListener/MatchFilterForLeadTrait` — deleted

`phpstan-baseline.neon`

`Tests/Unit/Helper/FilterEvaluatorTest` (new — 36 tests, no DB)

`Tests/Functional/EventListener/DynamicContentSubscriberTest` (rewritten — 10 tests, MySQL)