Open
Conversation
Reviewer's GuideMigrates the application from SwiftMailer to Symfony Mailer, simplifies email settings to only manage from/to/reply-to addresses, wires Symfony Mailer + Twig extra bundles, and introduces a Twig Sequence diagram for creating and sending an email with EmailFactory and Symfony MailersequenceDiagram
actor BackendController
participant EmailFactory as EmailFactory
participant ModulesSettings as ModulesSettings
participant Mailer as MailerInterface
participant SmtpServer as Smtp_server
BackendController->>EmailFactory: create()
activate EmailFactory
EmailFactory->>ModulesSettings: get(Core, mailer_from)
ModulesSettings-->>EmailFactory: mailer_from[name,email]
EmailFactory->>ModulesSettings: get(Core, mailer_to)
ModulesSettings-->>EmailFactory: mailer_to[name,email]
EmailFactory->>ModulesSettings: get(Core, mailer_reply_to)
ModulesSettings-->>EmailFactory: mailer_reply_to[name,email]
EmailFactory-->>BackendController: TemplatedEmail
deactivate EmailFactory
BackendController->>BackendController: configure subject,to,template,context
BackendController->>Mailer: send(TemplatedEmail)
activate Mailer
Mailer->>SmtpServer: open_connection_and_deliver
SmtpServer-->>Mailer: delivery_status
Mailer-->>BackendController: void
deactivate Mailer
Updated class diagram for EmailFactory and UtmExtensionclassDiagram
class ModulesSettings {
}
class TemplatedEmail {
}
class Address {
}
class EmailFactory {
- ModulesSettings modulesSettings
+ __construct(ModulesSettings modulesSettings)
+ create() TemplatedEmail
- createDefaultEmailInstance() TemplatedEmail
}
EmailFactory --> ModulesSettings : uses
EmailFactory --> TemplatedEmail : creates
EmailFactory --> Address : uses
class AbstractExtension {
}
class TwigFilter {
}
class UtmExtension {
+ getFilters() array
+ injectUtmTags(string html, string source, string medium, string campaign) string
}
UtmExtension --|> AbstractExtension
UtmExtension --> TwigFilter : returns
class MailerInterface {
}
BackendController o--> EmailFactory : obtains
BackendController o--> MailerInterface : obtains
class BackendController {
+ sendRegistrationEmail()
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
UtmExtension::injectUtmTagsimplementation relies on a simple regex (href="(https?:[^"]+)"), so it will miss links in single-quoted attributes or with unusual formatting; consider using a more robust HTML/DOM-based approach if you expect varied markup. - When injecting UTM parameters, existing
utm_*query parameters on the URL are not checked or merged, so the filter may append duplicate or conflicting UTM parameters; you may want to detect and preserve/override existing UTM values instead of always appending a new query string.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `UtmExtension::injectUtmTags` implementation relies on a simple regex (`href="(https?:[^"]+)"`), so it will miss links in single-quoted attributes or with unusual formatting; consider using a more robust HTML/DOM-based approach if you expect varied markup.
- When injecting UTM parameters, existing `utm_*` query parameters on the URL are not checked or merged, so the filter may append duplicate or conflicting UTM parameters; you may want to detect and preserve/override existing UTM values instead of always appending a new query string.
## Individual Comments
### Comment 1
<location path="src/Common/Core/Twig/Extensions/UtmExtension.php" line_range="30-46" />
<code_context>
+
+
+ return preg_replace_callback(
+ '/href="(https?:[^"]+)"/i',
+ function (array $matches) use ($campaign, $source, $medium): string {
+ $href = $matches[1];
+
+ $utmParams = http_build_query([
+ 'utm_source' => $source,
+ 'utm_medium' => $medium,
+ 'utm_campaign' => $campaign,
+ ]);
+
+ $separator = str_contains($href, '?') ? '&' : '?';
+
+ return 'href="' . $href . $separator . $utmParams . '"';
+ },
+ $html
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid appending UTM parameters to links that already contain UTM query params to prevent duplication.
This will append `utm_*` params to every `http(s)` href, even when the URL already has UTM tags, leading to duplicates or conflicts (e.g. `?utm_source=a&utm_source=b`). Before appending, either skip URLs where `preg_match('/[?&]utm_(source|medium|campaign)=/i', $href)` matches, or only add missing UTM keys. This will keep URLs consistent and avoid analytics issues.
```suggestion
return preg_replace_callback(
'/href="(https?:[^"]+)"/i',
function (array $matches) use ($campaign, $source, $medium): string {
$href = $matches[1];
// Skip if URL already contains any UTM parameter to avoid duplication
if (preg_match('/[?&]utm_(source|medium|campaign)=/i', $href) === 1) {
return $matches[0];
}
$utmParams = http_build_query([
'utm_source' => $source,
'utm_medium' => $medium,
'utm_campaign' => $campaign,
]);
$separator = str_contains($href, '?') ? '&' : '?';
return 'href="' . $href . $separator . $utmParams . '"';
},
$html
);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+30
to
+46
| return preg_replace_callback( | ||
| '/href="(https?:[^"]+)"/i', | ||
| function (array $matches) use ($campaign, $source, $medium): string { | ||
| $href = $matches[1]; | ||
|
|
||
| $utmParams = http_build_query([ | ||
| 'utm_source' => $source, | ||
| 'utm_medium' => $medium, | ||
| 'utm_campaign' => $campaign, | ||
| ]); | ||
|
|
||
| $separator = str_contains($href, '?') ? '&' : '?'; | ||
|
|
||
| return 'href="' . $href . $separator . $utmParams . '"'; | ||
| }, | ||
| $html | ||
| ); |
There was a problem hiding this comment.
suggestion (bug_risk): Avoid appending UTM parameters to links that already contain UTM query params to prevent duplication.
This will append utm_* params to every http(s) href, even when the URL already has UTM tags, leading to duplicates or conflicts (e.g. ?utm_source=a&utm_source=b). Before appending, either skip URLs where preg_match('/[?&]utm_(source|medium|campaign)=/i', $href) matches, or only add missing UTM keys. This will keep URLs consistent and avoid analytics issues.
Suggested change
| return preg_replace_callback( | |
| '/href="(https?:[^"]+)"/i', | |
| function (array $matches) use ($campaign, $source, $medium): string { | |
| $href = $matches[1]; | |
| $utmParams = http_build_query([ | |
| 'utm_source' => $source, | |
| 'utm_medium' => $medium, | |
| 'utm_campaign' => $campaign, | |
| ]); | |
| $separator = str_contains($href, '?') ? '&' : '?'; | |
| return 'href="' . $href . $separator . $utmParams . '"'; | |
| }, | |
| $html | |
| ); | |
| return preg_replace_callback( | |
| '/href="(https?:[^"]+)"/i', | |
| function (array $matches) use ($campaign, $source, $medium): string { | |
| $href = $matches[1]; | |
| // Skip if URL already contains any UTM parameter to avoid duplication | |
| if (preg_match('/[?&]utm_(source|medium|campaign)=/i', $href) === 1) { | |
| return $matches[0]; | |
| } | |
| $utmParams = http_build_query([ | |
| 'utm_source' => $source, | |
| 'utm_medium' => $medium, | |
| 'utm_campaign' => $campaign, | |
| ]); | |
| $separator = str_contains($href, '?') ? '&' : '?'; | |
| return 'href="' . $href . $separator . $utmParams . '"'; | |
| }, | |
| $html | |
| ); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace swiftmailer/swiftmailer with symfony/mailer
Todo
Sending an email is as simple as:
$this->get(EmailFactory::class)->create(). This wil return a preconfiguredTemplatedEmailinstance.$this->get(MailerInterface::class)>send($message);The preconfigured
TemplatedEmailinstance has the default from, to and reply-to configured.Full example:
Good to know:
parameters.ymlinstead of via the CMSinline_cssSummary by Sourcery
Migrate the application to Symfony Mailer with Twig extras, simplifying email configuration and enhancing email templating capabilities.
Enhancements:
Build:
Tests: