fix(DynamicContentSubscriber): complete Mautic 5 migration of filter evaluation#381
fix(DynamicContentSubscriber): complete Mautic 5 migration of filter evaluation#381edouard-mangel wants to merge 14 commits intoacquia:5.xfrom
Conversation
There was a problem hiding this comment.
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 aContactFilterMatcher. - Rewrites
DynamicContentSubscriberTestto 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>
a331ffe to
f49fc5d
Compare
…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
left a comment
There was a problem hiding this comment.
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.
| /** | ||
| * Polyfill for \Mautic\EmailBundle\EventListener\MatchFilterForLeadTrait. | ||
| */ | ||
| trait MatchFilterForLeadTrait |
There was a problem hiding this comment.
I'm not sure I understand the changes. This trait still exists in M5:
So why is this necessary?
There was a problem hiding this comment.
You're right that \Mautic\EmailBundle\EventListener\MatchFilterForLeadTrait still exists in M5 — and the polyfill actually wraps it directly via use \Mautic\EmailBundle\EventListener\MatchFilterForLeadTrait (line 14).
The override exists for two reasons:
- Multi-value support: custom object fields return an array of values (one per linked custom item), whereas the core trait's
matchFilterForLeadexpects a single scalar per field. The override handles'object' => 'custom_object'by iterating over$leadValuesas an array. - Helper methods:
doFiltersContainCompanyFilter()anddoFiltersContainTagsFilter()don't exist in the core trait — they're added here and used byContactFilterMatcher::match()to lazily pre-load company/tag data before evaluation.
The name "Polyfill" is admittedly misleading — it's really a custom-object extension of the core trait rather than a missing-class shim. Happy to rename it if that would make things clearer.
There was a problem hiding this comment.
Perhaps the PR description should be updated? I got the information that it was removed from it:
Used
MatchFilterForLeadTraitandDbalQueryTrait— both removed frommautic/core-lib ^5.0
There was a problem hiding this comment.
Good catch — the PR description was inaccurate. MatchFilterForLeadTrait still exists in Mautic 5 and was never removed. The real issue was that the old evaluateFilters() called executeSelect(QueryBuilder $qb) on the result of configureQueryBuilderFromSegmentFilter(), which can return a UnionQueryContainer for custom field filters — causing a TypeError at runtime, not a missing class. The description has been updated to reflect this.
There was a problem hiding this comment.
Good catch — the PR description was inaccurate. MatchFilterForLeadTrait still exists in Mautic 5 and was never removed. The real issue was that the old evaluateFilters() called executeSelect(QueryBuilder $qb) on the result of configureQueryBuilderFromSegmentFilter(), which can return a UnionQueryContainer for custom field filters — causing a TypeError at runtime, not a missing class. The description has been updated to reflect this.
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.
…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.
… non-capturing catches)
…ataForLead methods
Context
This PR fixes a fatal error that blocks Mautic 5 from booting with the plugin installed. The `5.x` branch still had `DynamicContentSubscriber` in its Mautic 4 form: it used a `QueryBuilder`-based filter evaluation path via `DbalQueryTrait` that is incompatible with Mautic 5 / Doctrine DBAL 3.x.
Root cause
On the `5.x` branch, `DynamicContentSubscriber`:
Changes
`EventListener/DynamicContentSubscriber.php`
`Helper/ContactFilterMatcher.php` (new)
Brings the `ContactFilterMatcher` class from the `staging` branch into `5.x`. This helper was already written as part of the Mautic 5 migration work but was never merged into the `5.x` branch. It fetches custom field data per contact and delegates filter evaluation to the core `MatchFilterForLeadTrait` (extended with custom-object multi-value support).
`Polyfill/EventListener/MatchFilterForLeadTrait.php` (new)
Wraps `\Mautic\EmailBundle\EventListener\MatchFilterForLeadTrait` (which still exists in M5) and extends it with:
`Config/config.php`
`Config/services.php`
`Tests/Unit/EventListener/DynamicContentSubscriberTest.php`
Rewritten to cover the actual collaborators: 6 tests, 21 assertions.
Architecture decision
Why was ContactFilterMatcher not in `5.x` originally?
The `5.x` branch was created from the merged `pr-338` commit which updated the plugin for Mautic 5. However, the `DynamicContentSubscriber` was left in its Mautic 4 state (with the broken trait references), while `ContactFilterMatcher` and its polyfill were developed in the `staging` branch as part of ongoing Mautic 5 work. This PR ports only those two files needed to make `DynamicContentSubscriber` functional.
Why not just replicate the old QueryBuilder approach?
The old approach fails because `configureQueryBuilderFromSegmentFilter()` returns a `UnionQueryContainer` for custom field filters — not a plain `QueryBuilder`. `ContactFilterMatcher` provides the correct Mautic 5 interface and is already used by `CampaignSubscriber` in `staging`.
Why `configureQueryBuilderFromSegmentFilter()` for CO filter detection?
`QueryFilterFactory::configureQueryBuilderFromSegmentFilter()` throws `InvalidSegmentFilterException` for any filter that is not a custom object filter. This makes it the natural gate for detecting CO-relevant filters, consistent with how `CampaignSubscriber` works in this plugin.
Verification
Tested on Mautic 5.2.9 / Symfony 5.4 / PHP 8.3 via DDEV:
```
bin/console cache:clear ✓ (was fatal before)
bin/console mautic:plugins:reload ✓ (13 DB tables created)
bin/console doctrine:mapping:info ✓ (14 CO entities discovered)
Custom Objects UI at /s/custom/object/ ✓ (fully functional)
PHPUnit DynamicContentSubscriberTest ✓ (6/6 tests pass)
```