Skip to content

Enable HTML emails#1685

Open
fiammybe wants to merge 15 commits into
ImpressCMS:developfrom
fiammybe:html-emails
Open

Enable HTML emails#1685
fiammybe wants to merge 15 commits into
ImpressCMS:developfrom
fiammybe:html-emails

Conversation

@fiammybe
Copy link
Copy Markdown
Member

@fiammybe fiammybe commented Feb 23, 2026

The library we're using PHPMailer, has the ability to send HTML emails, ImpressCMS just never was adapted to do so. This is a minimal change to make it possible to send HTML emails.

  • add the missing attributes to pass to PHPMailer to make HTML emails
    • isHTML
    • priority
    • LineEndingChar
  • Fix a 500 error when XoopsLocalMailer isn't defined for a language
  • The mail template system will look if a .html.tpl variant exists of the chosen template, and if so, will use that and switch over to HTML emails.

Copilot AI and others added 8 commits November 25, 2025 14:17
Co-authored-by: fiammybe <3736946+fiammybe@users.noreply.github.com>
Updated development guidelines for code style and PHP standards.
Auto-detect .html.tpl templates and enable HTML mode. Add isHtml and
altBody properties with setAltBody() and useHtml() methods; skip plain-
text newline normalization for HTML. Fix multimailer fallback to
icms_messaging_EmailHandler to avoid self-instantiation.
@fiammybe fiammybe added this to the 2.1.0 milestone Feb 23, 2026
@qodo-free-for-open-source-projects
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Code cleanup, property declarations, and formatting improvements

✨ Enhancement 🧪 Tests

Grey Divider

Walkthroughs

Description
• Add missing property declarations for Handler class
• Rename $LE to $LineEndingChar for clarity
• Add PluginDir property to EmailHandler class
• Comprehensive code formatting and style improvements
• Fix multimailer instantiation bug in constructor
Diagram
flowchart LR
  Handler["Handler.php<br/>Property Declarations"]
  EmailHandler["EmailHandler.php<br/>PluginDir Property"]
  Formatting["Code Formatting<br/>Style Standardization"]
  BugFix["Constructor Bug Fix<br/>Multimailer Instantiation"]
  Handler --> Formatting
  EmailHandler --> Formatting
  BugFix --> Formatting
Loading

Grey Divider

File Changes

1. htdocs/libraries/icms/messaging/Handler.php ✨ Enhancement +158/-63

Property declarations, variable renaming, and formatting standardization

• Add missing $priority and $LineEndingChar private property declarations
• Rename $LE variable to $LineEndingChar throughout the file for improved readability
• Fix multimailer instantiation bug: change from new icms_messaging_Handler() to `new
 icms_messaging_EmailHandler()` to avoid self-instantiation
• Standardize code formatting: convert single quotes to double quotes, update array syntax from
 array() to [], reformat method signatures and long lines
• Update string literals and function calls to follow consistent style guidelines

htdocs/libraries/icms/messaging/Handler.php


2. htdocs/libraries/icms/messaging/EmailHandler.php ✨ Enhancement +8/-0

Add PluginDir property declaration

• Add missing $PluginDir public property declaration with documentation
• Property initialized as empty string to store PHPMailer plugin directory path

htdocs/libraries/icms/messaging/EmailHandler.php


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown
Contributor

qodo-free-for-open-source-projects Bot commented Feb 23, 2026

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (3) 📎 Requirement gaps (0)

Grey Divider


Action required

1. LineEndingChar not used 📘 Rule violation ✓ Correctness
Description
reset() now initializes $this->LineEndingChar but the class still uses $this->LE for
header/body formatting, so line endings may become null/empty and trigger runtime notices. This
introduces inconsistent naming and effectively dead/incorrect code around line-ending handling.
Code

htdocs/libraries/icms/messaging/Handler.php[143]

+		$this->LineEndingChar = "\n";
Evidence
PR Compliance ID 9 requires consistent naming and removal of dead/incorrect code. The PR adds and
assigns $LineEndingChar, but the class continues to rely on $this->LE for joining headers and
normalizing body line endings, making the new property unused and potentially causing
undefined-property behavior.

htdocs/libraries/icms/messaging/Handler.php[102-106]
htdocs/libraries/icms/messaging/Handler.php[142-143]
htdocs/libraries/icms/messaging/Handler.php[234-234]
htdocs/libraries/icms/messaging/Handler.php[258-258]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`icms_messaging_Handler::reset()` now sets `$this-&amp;gt;LineEndingChar`, but the class still uses `$this-&amp;gt;LE` in multiple places. This can cause undefined-property notices and incorrect email header/body formatting.
## Issue Context
This PR introduced a new property name for line endings but did not update existing usages, creating inconsistent naming and dead/unused state.
## Fix Focus Areas
- htdocs/libraries/icms/messaging/Handler.php[102-106]
- htdocs/libraries/icms/messaging/Handler.php[123-144]
- htdocs/libraries/icms/messaging/Handler.php[234-234]
- htdocs/libraries/icms/messaging/Handler.php[258-258]
- htdocs/libraries/icms/messaging/Handler.php[467-470]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. @fopen hides template errors📘 Rule violation ⛯ Reliability
Description
The template load uses @fopen, which suppresses PHP warnings and prevents capturing actionable
failure context (e.g., permissions, missing file). This can lead to silent failures (especially when
$debug is false) and makes production diagnosis harder.
Code

htdocs/libraries/icms/messaging/Handler.php[R218-221]

+			if (!($fd = @fopen($path, "r"))) {
  			if ($debug) {
  				$this->errors[] = _MAIL_FAILOPTPL;
  			}
Evidence
PR Compliance ID 3 requires robust error handling with meaningful context and avoidance of silent
failures. The modified code explicitly suppresses file-open errors via @fopen and returns false
without any recorded error unless $debug is enabled.

Rule 3: Generic: Robust Error Handling and Edge Case Management
htdocs/libraries/icms/messaging/Handler.php[218-223]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Template loading suppresses file-open warnings using `@fopen` and can fail silently when `$debug` is false.
## Issue Context
Email template loading is a common failure point (missing file, permissions, wrong path) and should provide actionable diagnostics without relying solely on a debug flag.
## Fix Focus Areas
- htdocs/libraries/icms/messaging/Handler.php[210-224]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Recursive mailer instantiation🐞 Bug ⛯ Reliability
Description
If XoopsMailerLocal is not available, the constructor assigns $multimailer to a new
icms_messaging_Handler, which recursively invokes the same constructor. This can crash immediately
(infinite recursion) and also violates the documented expectation that $multimailer is an
EmailHandler/PHPMailer.
Code

htdocs/libraries/icms/messaging/Handler.php[R112-118]

+	public function __construct()
+	{
+		icms_loadLanguageFile("core", "xoopsmailerlocal");
+		if (class_exists("XoopsMailerLocal")) {
  		$this->multimailer = new XoopsMailerLocal();
  	} else {
  		$this->multimailer = new icms_messaging_Handler();
Evidence
The class-level doc says $multimailer should be an icms_messaging_EmailHandler, but the fallback
creates another instance of the wrapper icms_messaging_Handler itself, which repeats the same
logic and can recurse indefinitely if the class isn’t defined/loaded.

htdocs/libraries/icms/messaging/Handler.php[56-60]
htdocs/libraries/icms/messaging/Handler.php[112-120]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`icms_messaging_Handler::__construct()` falls back to `new icms_messaging_Handler()` when `XoopsMailerLocal` is missing, causing potential infinite recursion and assigning the wrong type to `$multimailer`.
### Issue Context
`$multimailer` is documented as an `icms_messaging_EmailHandler` (a PHPMailer subclass). The fallback should create that mailer.
### Fix Focus Areas
- htdocs/libraries/icms/messaging/Handler.php[112-120]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (1)
4. HTML mode not enabled🐞 Bug ✓ Correctness
Description
The messaging layer never selects a .html.tpl template variant and never calls PHPMailer’s
isHTML(true). As a result, this PR does not actually enable HTML email sending; any HTML content
will be sent as text/plain and render as raw markup.
Code

htdocs/libraries/icms/messaging/Handler.php[R387-391]

+	public function sendMail($email, $subject, $body, $headers)
+	{
  	$subject = $this->multimailer->encodeSubject($subject);
  	$this->multimailer->encodeBody($body);
  	$this->multimailer->ClearAllRecipients();
Evidence
send() reads the exact template name without checking for .html.tpl, and sendMail() only sets
Body without switching PHPMailer to HTML mode. PHPMailer explicitly requires calling
isHTML(true) to set the content type to HTML.

htdocs/libraries/icms/messaging/Handler.php[209-225]
htdocs/libraries/icms/messaging/Handler.php[387-405]
htdocs/libraries/phpmailer/src/PHPMailer.php[132-138]
htdocs/libraries/phpmailer/src/PHPMailer.php[948-960]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR claims to enable HTML emails, but the current messaging code neither selects `.html.tpl` templates nor switches PHPMailer into HTML mode via `isHTML(true)`. This means HTML templates (if added) will be delivered as `text/plain`.
### Issue Context
PHPMailer requires `isHTML(true)` to set `ContentType` to `text/html`.
### Fix Focus Areas
- htdocs/libraries/icms/messaging/Handler.php[201-225]
- htdocs/libraries/icms/messaging/Handler.php[387-416]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

5. $icmsConfig['language'] unguarded📘 Rule violation ⛯ Reliability
Description
Template path construction directly indexes $icmsConfig["language"] without checking it exists,
which can produce notices and build an invalid path when the config key is missing. This can break
template loading and email sending in edge environments.
Code

htdocs/libraries/icms/messaging/Handler.php[R213-217]

+					: ICMS_ROOT_PATH .
+						"/language/" .
+						$icmsConfig["language"] .
+						"/mail_template/" .
+						$this->template;
Evidence
PR Compliance ID 7 requires explicit guards/fallbacks for environment/config keys that may be
absent. The new/modified path-building code uses $icmsConfig["language"] directly when building a
filesystem path.

htdocs/libraries/icms/messaging/Handler.php[210-217]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`send()` constructs the template path using `$icmsConfig[&amp;quot;language&amp;quot;]` without checking that the key exists, which can cause notices and invalid paths.
## Issue Context
This code runs during email sending and should be robust even if configuration keys are missing or overridden.
## Fix Focus Areas
- htdocs/libraries/icms/messaging/Handler.php[203-218]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Comment thread htdocs/libraries/icms/messaging/Handler.php
Comment thread htdocs/libraries/icms/messaging/Handler.php Outdated
Comment thread htdocs/libraries/icms/messaging/Handler.php Outdated
Comment thread htdocs/libraries/icms/messaging/Handler.php
When XoopsMailerLocal is not available, the fallback was incorrectly
creating a new icms_messaging_Handler() instead of icms_messaging_EmailHandler().
This caused:
1. Infinite recursion (Handler creating Handler)
2. Wrong type assigned to \ (should be EmailHandler, a PHPMailer subclass)

Changed fallback to correctly instantiate icms_messaging_EmailHandler().
- Use null coalescing operator (??) to safely access icmsConfig['language']
- Default to 'english' if language key is missing
- Refactor template path construction for clarity
- Add HTML template auto-detection logic (checks for .html.tpl variant)
- Improves robustness when configuration keys are missing or overridden
- Remove @ suppression operator from fopen() to expose real errors
- Always capture error messages, not just when debug=true
- Add is_readable() check before attempting to open file
- Include file path in error messages for better diagnostics
- Properly close file handle after reading
- Distinguish between 'file not readable' and 'cannot open' errors

This ensures email template loading failures are always logged with
actionable information, improving debugging and troubleshooting.
- Add \ property declaration to track HTML email mode
- Initialize \ to false in reset() method
- Call \->multimailer->isHTML(\->isHtml) in sendMail() to configure PHPMailer

This ensures that when HTML templates (.html.tpl) are detected and loaded,
PHPMailer is properly configured to send them as text/html instead of text/plain.

The send() method already detects HTML templates and sets \->isHtml = true,
but sendMail() was not using this flag to configure PHPMailer. Now the complete
HTML email flow works:
1. send() detects .html.tpl template and sets \->isHtml = true
2. sendMail() calls isHTML(\->isHtml) to configure PHPMailer
3. PHPMailer sends email with Content-Type: text/html
- Add \ property for plain text fallback in HTML emails
- Initialize \ in reset() method
- Improve sendMail() to properly handle both HTML and plain text modes
- Always call isHTML() explicitly to reset ContentType between sends
- Set AltBody for HTML emails, clear it for plain text emails

This ensures:
1. HTML emails have a plain text fallback (AltBody)
2. ContentType is correctly reset between successive sends
3. Plain text emails don't have leftover HTML configuration
4. Better compatibility with email clients that prefer plain text
- Add setAltBody(\) method to allow setting plain text fallback for HTML emails
- Add useHtml(\ = true) method to allow manual control of HTML mode
- These methods provide a public API for callers to configure HTML email behavior

This allows callers to:
1. Manually set HTML mode via useHtml(true) if not using auto-detection
2. Provide a plain text fallback via setAltBody() for better email client compatibility
3. Have full control over email format when needed
@fiammybe fiammybe requested review from MekDrop and skenow February 23, 2026 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants