Skip to content

Bugfix/typoscript settings maillog#85

Open
weakbit wants to merge 5 commits into
masterfrom
bugfix/typoscript-settings-maillog
Open

Bugfix/typoscript settings maillog#85
weakbit wants to merge 5 commits into
masterfrom
bugfix/typoscript-settings-maillog

Conversation

@weakbit

@weakbit weakbit commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@weakbit
weakbit requested a review from smirgol July 14, 2026 14:40
@weakbit weakbit self-assigned this Jul 14, 2026
Comment thread composer.json
"require": {
"php": "~8.3.0 || ~8.4.0 || ~8.5.0",
"composer-runtime-api": "^2",
"typo3/cms-core": "^12.4.0 || ^13.4.0",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

needs to change in ext_emconf.php aswell

return $normalizedPaths;
}

private function getRequest(): ServerRequestInterface

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DRY: TemplateBasedMailMessage::getRequest(), ConfigurationUtility::getRequest(), CleanupSettingsService::createBackendRequest()

@smirgol

smirgol commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

One thing I'd clarify before merging: getTransport() unwraps the FileSpool, so Mailer::send() runs directly through FileSpool::send() - the mail is not logged at spool time. The log entry is only created on flush, because getRealTransport() wraps the SMTP transport again into a LoggingTransport. A mail that is spooled but not (yet) flushed (or where the flush fails) therefore never appears in the log.

IMHO the double logging does not come from the shared MailLog, but from the flush transport being wrapped again and LoggingTransport::send being called a second time. If we want to log each mail once and immediately at send time, I would reverse it: keep getTransport() wrapped (logging at spool time), no longer wrap getRealTransport() (flush without re-logging). One entry per mail, at the earliest possible point. The current approach trades this logging guarantee for more precise status information in the log. That may be intentional - but I'd like to see it documented in the PR body so it doesn't pass as an unintended behavior change.

@smirgol

smirgol commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

A caveat about the new spool path that should at least be documented:

Since the core fix for CVE-2026-0859, FileSpool uses a PolymorphicDeserializer with a fixed allowlist (SentMessage, RawMessage, Envelope, Address, AbstractPart, File, Headers, HeaderInterface). TemplateBasedMailMessage extends MailMessage/Email and is not included. With transport_spool_type=file, FileSpool serializes the entire TemplateBasedMailMessage; on flush, deserialization fails → the file is renamed to .invalid → the mail is neither delivered nor logged.

This is not introduced by this PR - it happens whenever the extension is used with file spool. Since LoggingDelayedTransport now explicitly adds spool support, it would be helpful to make this visible in one place (PR body or a short comment at LoggingDelayedTransport): spool in combination with the custom Email subclass currently does not work until the allowlist covers it or the extension handles it separately.

$this->configurationManager->setRequest($this->createBackendRequest());
}

return $this->configurationManager->getConfiguration(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Previously there was a catch for RuntimeException, now there isn't. Intended?
Also, not sure about possible in-process side-effects by setting the request globally

#[Override]
protected function setUp(): void
{
putenv('typo3DatabaseDriver=pdo_sqlite');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

phpunit.xml -> <env name="typo3DatabaseDriver" value="pdo_sqlite"/>?


class LoggingTransport implements TransportInterface
{
protected MailLog $mailLog;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not yet an issue, but accessing this before call to send() will except with TypeError.
How about protected ?MailLog $mailLog = null;?

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.

3 participants