Email adapter#28247
Open
andreascreten wants to merge 2 commits into
Open
Conversation
Contributor
WalkthroughThis PR introduces a pluggable email adapter framework that decouples Ghost core from Mailgun-specific implementation details. It defines an 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
ghost/core/core/server/services/email-service/email-service-wrapper.js (1)
78-81: 💤 Low valueProvider-specific branch leaks Mailgun details into the generic wrapper.
Hardcoding
if (emailProvider === 'mailgun') { adapterConfig.labs = labs; }couples this generic adapter-loading path to one provider, partially defeating the abstraction. Consider always passinglabsinadapterConfigand letting each adapter pick what it needs, so adding a new provider requires no edits here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/core/server/services/email-service/email-service-wrapper.js` around lines 78 - 81, The current provider-specific branch leaks Mailgun details by only setting adapterConfig.labs when emailProvider === 'mailgun'; instead, always include labs on the adapterConfig so adapters can opt-in, i.e. assign adapterConfig.labs = labs unconditionally (remove the if-check referencing emailProvider), and let each adapter implementation read adapterConfig.labs if needed; update any surrounding adapter-loading logic that constructs adapterConfig to ensure labs is passed through.ghost/core/core/server/services/email-analytics/email-analytics-service-wrapper.js (1)
69-72: 💤 Low valueProvider-specific
=== 'mailgun'branch leaks provider knowledge into generic loading code.Hard-coding which provider gets
labshere couples this generic adapter-loading path to Mailgun and won't scale to community providers that may (or may not) needlabs. Passinglabsunconditionally is harmless for adapters that ignore it, or the per-provider dependency selection should live behind the adapter/factory.♻️ Drop the provider-specific branch
const adapterConfig = { configService: config, - settingsCache: settings + settingsCache: settings, + labs }; - - // Add labs for Mailgun - if (emailProvider === 'mailgun') { - adapterConfig.labs = labs; - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/core/server/services/email-analytics/email-analytics-service-wrapper.js` around lines 69 - 72, The code currently special-cases emailProvider === 'mailgun' and sets adapterConfig.labs only for Mailgun; remove that provider-specific branch and instead attach labs to adapterConfig unconditionally (i.e., ensure adapterConfig.labs = labs before adapter loading) so adapters that need it receive it and others ignore it; reference adapterConfig, emailProvider, and labs when making the change so you update the generic adapter-loading path rather than coupling it to Mailgun.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ghost/core/core/server/adapters/email/index.js`:
- Around line 9-17: getEmailAdapter currently builds an adapterName and calls
adapterManager.getAdapter(adapterName) with no way to pass runtime dependencies;
modify getEmailAdapter to accept a second parameter (e.g., deps or options) and
thread that through to adapterManager.getAdapter so runtime deps like
configService, settingsCache, labs and errorHandler can be injected by callers
(see email-service-wrapper.js); update the function signature
(getEmailAdapter(feature, deps)) and ensure adapterManager.getAdapter is invoked
with those deps/options so callers can avoid reconstructing adapters.
In
`@ghost/core/core/server/services/email-analytics/email-analytics-service-wrapper.js`:
- Around line 58-80: The analytics wrapper currently builds adapterConfig from
config/settings and labs but omits provider-specific options and the error
handler used by the sending adapter; fix this by merging
bulkEmailConfig[emailProvider] into adapterConfig (so provider-specific keys are
preserved) and include the same errorHandler that the sending path provides
(mirror what email-service-wrapper.js does) before instantiating AdapterClass
(from emailAdapter.getEmailAdapter().constructor), keeping the existing
conditional labs injection and then creating the new instance with the merged
adapterConfig.
In `@ghost/core/core/server/services/email-service/email-service-wrapper.js`:
- Around line 84-86: The code dereferences bulkEmailConfig[emailProvider]
without guarding against bulkEmailConfig being undefined; update the block
around emailProvider and adapterConfig so you only access
bulkEmailConfig[emailProvider] when bulkEmailConfig is present — e.g., change
the if to check bulkEmailConfig && bulkEmailConfig[emailProvider] (or use
optional chaining bulkEmailConfig?.[emailProvider]) before calling
Object.assign(adapterConfig, ...), ensuring you reference the existing symbols
bulkEmailConfig, emailProvider and adapterConfig.
In `@ghost/core/test/unit/server/adapters/email/Mailgun.test.js`:
- Line 55: The tests are attempting to replace Mailgun's native private fields
by setting _Mailgun__emailProvider/_Mailgun__analyticsProvider which only create
public properties and do not override the class private fields
(`#emailProvider/`#analyticsProvider); update the tests to inject stubbed
providers via Mailgun's public constructor/config seam (or add a supported
injection seam) so that the instance uses the provided email and analytics
providers when calling send() and fetchLatest(); specifically, modify
Mailgun.test.js to construct Mailgun with the stubbed provider instances (or
adjust Mailgun's constructor to accept an options object with
emailProvider/analyticsProvider) rather than assigning to
_Mailgun__emailProvider/_Mailgun__analyticsProvider, so send() and fetchLatest()
will delegate to the stubs.
---
Nitpick comments:
In
`@ghost/core/core/server/services/email-analytics/email-analytics-service-wrapper.js`:
- Around line 69-72: The code currently special-cases emailProvider ===
'mailgun' and sets adapterConfig.labs only for Mailgun; remove that
provider-specific branch and instead attach labs to adapterConfig
unconditionally (i.e., ensure adapterConfig.labs = labs before adapter loading)
so adapters that need it receive it and others ignore it; reference
adapterConfig, emailProvider, and labs when making the change so you update the
generic adapter-loading path rather than coupling it to Mailgun.
In `@ghost/core/core/server/services/email-service/email-service-wrapper.js`:
- Around line 78-81: The current provider-specific branch leaks Mailgun details
by only setting adapterConfig.labs when emailProvider === 'mailgun'; instead,
always include labs on the adapterConfig so adapters can opt-in, i.e. assign
adapterConfig.labs = labs unconditionally (remove the if-check referencing
emailProvider), and let each adapter implementation read adapterConfig.labs if
needed; update any surrounding adapter-loading logic that constructs
adapterConfig to ensure labs is passed through.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fabfa5dc-d43c-4edb-84ee-b87ed95ae7be
📒 Files selected for processing (9)
ghost/core/core/server/adapters/email/EmailProviderBase.jsghost/core/core/server/adapters/email/Mailgun.jsghost/core/core/server/adapters/email/index.jsghost/core/core/server/services/adapter-manager/index.jsghost/core/core/server/services/email-analytics/email-analytics-service-wrapper.jsghost/core/core/server/services/email-service/email-service-wrapper.jsghost/core/core/shared/config/defaults.jsonghost/core/test/unit/server/adapters/email/EmailProviderBase.test.jsghost/core/test/unit/server/adapters/email/Mailgun.test.js
Comment on lines
+9
to
+17
| function getEmailAdapter(feature) { | ||
| let adapterName = 'email'; | ||
|
|
||
| if (feature) { | ||
| adapterName += `:${feature}`; | ||
| } | ||
|
|
||
| return adapterManager.getAdapter(adapterName); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
getEmailAdapter provides no way to inject runtime dependencies.
The factory returns a fully-constructed instance from the adapter manager using only the static config from defaults.json (Mailgun: {}). Runtime deps like configService, settingsCache, labs, and errorHandler cannot be passed in, which forces consumers (the email-service wrapper) to construct the adapter a second time. Consider accepting a config/deps argument and threading it through to the adapter manager. See the related comment in email-service-wrapper.js.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ghost/core/core/server/adapters/email/index.js` around lines 9 - 17,
getEmailAdapter currently builds an adapterName and calls
adapterManager.getAdapter(adapterName) with no way to pass runtime dependencies;
modify getEmailAdapter to accept a second parameter (e.g., deps or options) and
thread that through to adapterManager.getAdapter so runtime deps like
configService, settingsCache, labs and errorHandler can be injected by callers
(see email-service-wrapper.js); update the function signature
(getEmailAdapter(feature, deps)) and ensure adapterManager.getAdapter is invoked
with those deps/options so callers can avoid reconstructing adapters.
Comment on lines
+58
to
+80
| try { | ||
| // Get unified email adapter instance (same one used for email sending) | ||
| const adapterInstance = emailAdapter.getEmailAdapter(); | ||
|
|
||
| // Inject dependencies needed by the adapter | ||
| const AdapterClass = adapterInstance.constructor; | ||
| const adapterConfig = { | ||
| configService: config, | ||
| settingsCache: settings | ||
| }; | ||
|
|
||
| // Add labs for Mailgun | ||
| if (emailProvider === 'mailgun') { | ||
| adapterConfig.labs = labs; | ||
| } | ||
|
|
||
| // Create a new instance for analytics (the email service has its own instance) | ||
| providers.push(new AdapterClass(adapterConfig)); | ||
| } catch (error) { | ||
| logging.error(`[EmailAnalytics] Failed to load ${emailProvider} adapter: ${error.message}`); | ||
| logging.error(error.stack); | ||
| throw error; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Inspect the email adapter factory (getEmailAdapter)
fd -t f 'index.js' | rg -n 'adapters/email/index.js$'
rg -nP -C4 'getEmailAdapter'
# 2) Inspect the EmailProviderBase + Mailgun adapter constructors / destructured config keys
fd -t f 'EmailProviderBase.js' --exec cat {}
fd -t f 'Mailgun.js' -p 'adapters/email' --exec cat {}Repository: TryGhost/Ghost
Length of output: 6440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the Mailgun adapter file(s)
fd -t f -i 'mailgun\.js$' . || true
fd -t f -i '*mailgun*' ghost/core/core/server/adapters || true
# Inspect the actual Mailgun adapter implementation (whatever file exists)
# If multiple matches exist, print the first few.
matches=( $(fd -t f -i '*mailgun*' ghost/core/core/server/adapters -a 2>/dev/null | head -n 20) )
echo "MAILGUN_MATCHES=${`#matches`[@]}"
for f in "${matches[@]}"; do
echo "---- $f (head) ----"
sed -n '1,160p' "$f"
echo "---- $f (tail) ----"
tail -n 80 "$f"
done
# Inspect email adapter factory / adapterManager usage
sed -n '1,120p' ghost/core/core/server/adapters/email/index.js
# Inspect email-service wrapper's adapter loading block for comparison
sed -n '1,140p' ghost/core/core/server/services/email-service/email-service-wrapper.js
# Inspect email-analytics wrapper around the relevant lines (for exact config construction)
sed -n '1,140p' ghost/core/core/server/services/email-analytics/email-analytics-service-wrapper.js
# Search for "labs" destructuring in adapters/providers
rg -n "labs" ghost/core/core/server/adapters/email ghost/core/core/server/services/email-analytics ghost/core/core/server/adapters -S || true
rg -nP "constructor\((\{)?\s*[^)]*labs" ghost/core/core/server/adapters -S || true
# Search for where getEmailAdapter is called with/without args
rg -n "getEmailAdapter\(" ghost/core/core/server/services/email-analytics ghost/core/core/server/services/email-service ghost/core/core/server/adapters -S || true
# Inspect adapterManager.getAdapter contract (if present)
fd -t f -i 'adapterManager\.js$' ghost/core/core/server || true
fd -t f -i '*adapter*manager*' ghost/core/core/server | head -n 20Repository: TryGhost/Ghost
Length of output: 541
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate Mailgun adapter(s) deterministically
fd -t f -i '*mailgun*.js' ghost/core/core/server/adapters/email -a
fd -t f -i '*mailgun*.js' ghost/core/core/server/adapters -a
# 2) Print adapter factory + adapterManager usage
sed -n '1,140p' ghost/core/core/server/adapters/email/index.js || true
fd -t f -i '*adapter*manager*.js' ghost/core/core/server | head -n 50
# 3) Inspect Mailgun adapter implementation
MAILGUN_FILE="$(fd -t f -i 'Mailgun\.js$' ghost/core/core/server/adapters/email | head -n 1)"
echo "MAILGUN_FILE=$MAILGUN_FILE"
sed -n '1,240p' "$MAILGUN_FILE"
tail -n 120 "$MAILGUN_FILE"
# 4) Inspect EmailProviderBase (to see constructor shape and config expectations)
EMAIL_BASE_FILE="$(fd -t f -i 'EmailProviderBase.*\.js$' ghost/core/core/server/adapters/email ghost/core/core/server/adapters -a | head -n 1)"
echo "EMAIL_BASE_FILE=$EMAIL_BASE_FILE"
if [[ -n "${EMAIL_BASE_FILE:-}" ]]; then
sed -n '1,220p' "$EMAIL_BASE_FILE"
fi
# 5) Inspect email-analytics wrapper and the mailgun-specific analytics provider
sed -n '1,220p' ghost/core/core/server/services/email-analytics/email-analytics-service-wrapper.js
sed -n '1,220p' ghost/core/core/server/services/email-analytics/email-analytics-provider-mailgun.js
# 6) Inspect email-service wrapper for comparison of how adapters are instantiated/configured
sed -n '1,220p' ghost/core/core/server/services/email-service/email-service-wrapper.js
# 7) Search for exact usage of adapterConfig keys inside Mailgun adapter
rg -n --fixed-strings "configService" ghost/core/core/server/adapters/email/Mailgun.js || true
rg -n --fixed-strings "settingsCache" ghost/core/core/server/adapters/email/Mailgun.js || true
rg -n --fixed-strings "labs" ghost/core/core/server/adapters/email/Mailgun.js || true
rg -n --fixed-strings "constructor" ghost/core/core/server/adapters/email/Mailgun.js || true
# 8) Search for destructuring patterns in Email adapter constructor calls
rg -nP "getEmailAdapter\\(([^)]*)\\)" ghost/core/core/server/services/email-analytics ghost/core/core/server/services/email-service ghost/core/core/server/adapters/email/index.js || true
rg -nP "new .*Adapter\\((\\{)?[^}]*\\}" ghost/core/core/server/services/email-analytics ghost/core/core/server/services/email-service ghost/core/core/server/adapters/email -S || trueRepository: TryGhost/Ghost
Length of output: 350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Cleanly locate Mailgun adapter(s) via safe glob
MAILGUN_FILES=( )
while IFS= read -r f; do MAILGUN_FILES+=("$f"); done < <(fd -t f --glob '*mailgun*.js' ghost/core/core/server/adapters/email)
echo "MAILGUN_FILES_COUNT=${`#MAILGUN_FILES`[@]}"
printf '%s\n' "${MAILGUN_FILES[@]}"
# Email adapter factory
echo "---- adapters/email/index.js ----"
sed -n '1,220p' ghost/core/core/server/adapters/email/index.js
# Find EmailProviderBase (exact file by name)
EMAIL_BASE_FILE="$(fd -t f --glob '*EmailProviderBase*.js' ghost/core/core/server/adapters/email ghost/core/core/server/adapters -a | head -n 1 || true)"
echo "EMAIL_BASE_FILE=$EMAIL_BASE_FILE"
if [[ -n "${EMAIL_BASE_FILE:-}" ]]; then
sed -n '1,260p' "$EMAIL_BASE_FILE"
fi
# Inspect Mailgun adapter(s) + search for config keys
for f in "${MAILGUN_FILES[@]}"; do
echo "---- $f (head) ----"
sed -n '1,260p' "$f"
echo "---- $f (search config keys) ----"
rg -n --fixed-strings 'configService' "$f" || true
rg -n --fixed-strings 'settingsCache' "$f" || true
rg -n --fixed-strings 'labs' "$f" || true
rg -n 'constructor\s*\(' "$f" || true
done
# Inspect analytics wrapper + email-analytics-provider-mailgun
echo "---- email-analytics-service-wrapper.js (relevant) ----"
sed -n '1,220p' ghost/core/core/server/services/email-analytics/email-analytics-service-wrapper.js
echo "---- email-analytics-provider-mailgun.js (relevant) ----"
sed -n '1,260p' ghost/core/core/server/services/email-analytics/email-analytics-provider-mailgun.js
# Inspect email-service wrapper for how it passes config
echo "---- email-service-wrapper.js (relevant) ----"
sed -n '1,220p' ghost/core/core/server/services/email-service/email-service-wrapper.js
# Search for getEmailAdapter invocation patterns in wrappers
echo "---- getEmailAdapter usage in wrappers ----"
rg -n "getEmailAdapter\s*\(" ghost/core/core/server/services/email-analytics ghost/core/core/server/services/email-service ghost/core/core/server/adapters/email -S || trueRepository: TryGhost/Ghost
Length of output: 115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) List Mailgun adapter files (use glob, not regex)
TMP1=/tmp/mailgun_files.txt
rm -f "$TMP1"
fd -t f --glob '*mailgun*.js' ghost/core/core/server/adapters/email > "$TMP1" || true
echo "MAILGUN_FILES_COUNT=$(wc -l < "$TMP1" 2>/dev/null || echo 0)"
echo "---- mailgun files ----"
cat "$TMP1" 2>/dev/null || true
# 2) Print adapter factory
echo "---- adapters/email/index.js ----"
sed -n '1,220p' ghost/core/core/server/adapters/email/index.js
# 3) Locate EmailProviderBase
TMP2=/tmp/email_base_files.txt
rm -f "$TMP2"
fd -t f --glob '*EmailProviderBase*.js' ghost/core/core/server/adapters > "$TMP2" || true
EMAIL_BASE_FILE="$(head -n 1 "$TMP2" 2>/dev/null || true)"
echo "EMAIL_BASE_FILE=${EMAIL_BASE_FILE:-<none>}"
if [[ -n "${EMAIL_BASE_FILE:-}" ]]; then
sed -n '1,260p' "$EMAIL_BASE_FILE"
fi
# 4) Inspect Mailgun adapter(s)
while IFS= read -r f; do
[[ -z "$f" ]] && continue
echo "==================== $f ===================="
sed -n '1,260p' "$f"
echo "---- search config keys in $f ----"
rg -n --fixed-strings 'configService' "$f" || true
rg -n --fixed-strings 'settingsCache' "$f" || true
rg -n --fixed-strings 'labs' "$f" || true
echo "---- search constructor/config destructuring in $f ----"
rg -nP "constructor\s*\(\s*\{\s*[^}]*\}\s*\)" "$f" || true
rg -nP "constructor\s*\(\s*[^)]*\)" "$f" || true
done < "$TMP1"
# 5) Inspect wrappers
echo "---- email-analytics-service-wrapper.js ----"
sed -n '1,240p' ghost/core/core/server/services/email-analytics/email-analytics-service-wrapper.js
echo "---- email-analytics-provider-mailgun.js ----"
sed -n '1,260p' ghost/core/core/server/services/email-analytics/email-analytics-provider-mailgun.js
echo "---- email-service-wrapper.js ----"
sed -n '1,240p' ghost/core/core/server/services/email-service/email-service-wrapper.js
# 6) Search getEmailAdapter usage patterns
echo "---- getEmailAdapter usage in wrappers ----"
rg -n "getEmailAdapter\s*\(" ghost/core/core/server/services/email-analytics ghost/core/core/server/services/email-service ghost/core/core/server/adapters/email/index.js -S || trueRepository: TryGhost/Ghost
Length of output: 27507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List all email adapters under adapters/email
echo "---- adapters/email/*.js ----"
fd -t f ghost/core/core/server/adapters/email | sort
# Inspect adapter-manager implementation for getAdapter contract
ADAPTER_MANAGER_FILE="$(fd -t f -i 'adapter-manager\.js$' ghost/core/core/server/services | head -n 1 || true)"
if [[ -n "${ADAPTER_MANAGER_FILE:-}" ]]; then
echo "---- $ADAPTER_MANAGER_FILE (head) ----"
sed -n '1,220p' "$ADAPTER_MANAGER_FILE"
echo "---- $ADAPTER_MANAGER_FILE (search getAdapter) ----"
rg -n "getAdapter" "$ADAPTER_MANAGER_FILE" || true
else
echo "adapter-manager.js not found under services; searching broader..."
ADAPTER_MANAGER_FILE="$(fd -t f -i '*adapter*manager*.js' ghost/core/core/server | head -n 1 || true)"
echo "---- $ADAPTER_MANAGER_FILE (head) ----"
sed -n '1,240p' "$ADAPTER_MANAGER_FILE"
rg -n "getAdapter" "$ADAPTER_MANAGER_FILE" || true
fi
# Inspect analytics provider wrapper usage of errorHandler/labs (if any)
rg -n "errorHandler" ghost/core/core/server/services/email-analytics -S || true
rg -n "new AdapterClass" ghost/core/core/server/services/email-analytics -S || trueRepository: TryGhost/Ghost
Length of output: 560
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) List adapter files in the adapters/email directory
echo "---- ls ghost/core/core/server/adapters/email ----"
ls -la ghost/core/core/server/adapters/email || true
echo "---- fd files in ghost/core/core/server/adapters/email ----"
fd -t f . ghost/core/core/server/adapters/email | sort
# 2) Locate adapter-manager
echo "---- locate adapter-manager ----"
ADAPTER_MANAGER_FILES=$(fd -t f --full-path -i '*adapter*manager*.js' ghost/core/core/server || true)
echo "$ADAPTER_MANAGER_FILES" | head -n 20
ADAPTER_MANAGER_FILE=$(echo "$ADAPTER_MANAGER_FILES" | head -n 1 || true)
if [[ -n "${ADAPTER_MANAGER_FILE:-}" ]]; then
echo "---- $ADAPTER_MANAGER_FILE (head) ----"
sed -n '1,240p' "$ADAPTER_MANAGER_FILE"
echo "---- $ADAPTER_MANAGER_FILE (getAdapter search) ----"
rg -n "getAdapter" "$ADAPTER_MANAGER_FILE" || true
fi
# 3) Also inspect any nearby adapter-manager imports
echo "---- references to '../../services/adapter-manager' ----"
rg -n --fixed-strings "services/adapter-manager" ghost/core/core/server/adapters/email ghost/core/core/server/services | head -n 50
# 4) Inspect the email-service adapter factory (already known) for getEmailAdapter(feature) contract
echo "---- adapters/email/index.js (full) ----"
sed -n '1,120p' ghost/core/core/server/adapters/email/index.jsRepository: TryGhost/Ghost
Length of output: 1686
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate adapter-manager files safely (glob, not regex)
fd -t f --glob '*adapter*manager*.js' ghost/core/core/server/services -a | sort | head -n 20
fd -t f --glob '*adapter*manager*.js' ghost/core/core/server -a | sort | head -n 50
ADAPTER_MANAGER_FILE="$(fd -t f --glob '*adapter*manager*.js' ghost/core/core/server/services -a | sort | head -n 1 || true)"
echo "ADAPTER_MANAGER_FILE=${ADAPTER_MANAGER_FILE:-<none>}"
if [[ -n "${ADAPTER_MANAGER_FILE:-}" ]]; then
sed -n '1,260p' "$ADAPTER_MANAGER_FILE"
rg -n "getAdapter" "$ADAPTER_MANAGER_FILE" || true
rg -n "email" "$ADAPTER_MANAGER_FILE" || true
fi
# Search for any other adapters extending EmailProviderBase (beyond adapters/email/Mailgun.js)
rg -n --glob '**/*.js' --glob '**/*.ts' "extends EmailProviderBase" ghost/core/core/server/adapters/email ghost/core/core/server | head -n 50 || true
# Search for adapterName usage like 'email:' or adapter registry keys
rg -n --fixed-strings "email:" ghost/core/core/server | head -n 50 || true
rg -n --fixed-strings "getAdapter('email" ghost/core/core/server | head -n 50 || true
rg -n --fixed-strings "getAdapter(\"email" ghost/core/core/server | head -n 50 || trueRepository: TryGhost/Ghost
Length of output: 16463
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect adapter-manager module export from services/adapter-manager
ls -la ghost/core/core/server/services/adapter-manager || true
fd -t f --glob 'ghost/core/core/server/services/adapter-manager/*' ghost/core/core/server/services/adapter-manager -a | sort
# Print entrypoints (index.js or main file)
for f in \
ghost/core/core/server/services/adapter-manager/index.js \
ghost/core/core/server/services/adapter-manager.js \
ghost/core/core/server/services/adapter-manager/*.js
do
if [[ -f "$f" ]]; then
echo "---- $f ----"
sed -n '1,260p' "$f"
fi
done
# Search for adapterManager.getAdapter wrappers or adapters registration
rg -n "registerAdapter\\(" ghost/core/core/server | head -n 50
rg -n "new AdapterManager" ghost/core/core/server | head -n 50
rg -n "adapterManager\\.getAdapter" ghost/core/core/server | head -n 50
# Specifically find where adapter type 'email' is registered
rg -n "registerAdapter\\(['\"]email['\"]" ghost/core/core/server | head -n 50 || true
rg -n "registerAdapter\\(\"email\"" ghost/core/core/server | head -n 50 || trueRepository: TryGhost/Ghost
Length of output: 16749
Align email analytics adapter instantiation/config with email sending
The adapters/email/Mailgun.js constructor expects {configService, settingsCache, labs, errorHandler}, and this analytics wrapper passes configService/settingsCache and conditionally labs for mailgun—so the key names match for Mailgun.
However, the wrapper still recreates the adapter via emailAdapter.getEmailAdapter().constructor and (unlike email-service-wrapper.js) does not provide an errorHandler and does not merge bulkEmailConfig[emailProvider] into adapterConfig. For any provider/adaptor that relies on provider-specific options (or error handling) this can leave the analytics adapter misconfigured even while sending works.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ghost/core/core/server/services/email-analytics/email-analytics-service-wrapper.js`
around lines 58 - 80, The analytics wrapper currently builds adapterConfig from
config/settings and labs but omits provider-specific options and the error
handler used by the sending adapter; fix this by merging
bulkEmailConfig[emailProvider] into adapterConfig (so provider-specific keys are
preserved) and include the same errorHandler that the sending path provides
(mirror what email-service-wrapper.js does) before instantiating AdapterClass
(from emailAdapter.getEmailAdapter().constructor), keeping the existing
conditional labs injection and then creating the new instance with the merged
adapterConfig.
Comment on lines
+84
to
+86
| if (bulkEmailConfig[emailProvider]) { | ||
| Object.assign(adapterConfig, bulkEmailConfig[emailProvider]); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard bulkEmailConfig access for consistency.
Line 52 reads bulkEmailConfig?.provider (treating an absent bulkEmail config as valid), but line 84 dereferences bulkEmailConfig[emailProvider] unguarded, which would throw a TypeError if bulkEmail is undefined. Although defaults.json always defines bulkEmail, the inconsistency is a latent footgun.
🛡️ Proposed guard
- if (bulkEmailConfig[emailProvider]) {
+ if (bulkEmailConfig?.[emailProvider]) {
Object.assign(adapterConfig, bulkEmailConfig[emailProvider]);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ghost/core/core/server/services/email-service/email-service-wrapper.js`
around lines 84 - 86, The code dereferences bulkEmailConfig[emailProvider]
without guarding against bulkEmailConfig being undefined; update the block
around emailProvider and adapterConfig so you only access
bulkEmailConfig[emailProvider] when bulkEmailConfig is present — e.g., change
the if to check bulkEmailConfig && bulkEmailConfig[emailProvider] (or use
optional chaining bulkEmailConfig?.[emailProvider]) before calling
Object.assign(adapterConfig, ...), ensuring you reference the existing symbols
bulkEmailConfig, emailProvider and adapterConfig.
| }); | ||
|
|
||
| // Replace the internal provider with our mock | ||
| adapter._Mailgun__emailProvider = emailProvider; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any babel config that would transform server code
fd -H -t f -i 'babel.config*'
fd -H -t f -i '.babelrc*'
# Check whether the mangled private-field convention is used anywhere else in the repo
rg -nP '_[A-Za-z]+__[A-Za-z]' -g '!**/node_modules/**' | head -50
# Inspect mocharc / test setup for ghost/core to confirm no transpilation hook
fd -H -t f -i '.mocharc*' ghost/coreRepository: TryGhost/Ghost
Length of output: 50371
Fix Mailgun unit test seam: _Mailgun__emailProvider/_Mailgun__analyticsProvider don’t override native private fields.
ghost/core/core/server/adapters/email/Mailgun.js delegates send()/fetchLatest() to native ES private fields #emailProvider and #analyticsProvider. In ghost/core/test/unit/server/adapters/email/Mailgun.test.js the test sets _Mailgun__emailProvider (lines 55/79/94) and _Mailgun__analyticsProvider (line 109), which only creates unrelated public properties and won’t replace this.#emailProvider/this.#analyticsProvider, so the delegation assertions won’t be exercising the stubs. Inject the provider instances via the constructor/config seam (or add a supported seam) instead of mutating _Mailgun__*.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ghost/core/test/unit/server/adapters/email/Mailgun.test.js` at line 55, The
tests are attempting to replace Mailgun's native private fields by setting
_Mailgun__emailProvider/_Mailgun__analyticsProvider which only create public
properties and do not override the class private fields
(`#emailProvider/`#analyticsProvider); update the tests to inject stubbed
providers via Mailgun's public constructor/config seam (or add a supported
injection seam) so that the instance uses the provided email and analytics
providers when calling send() and fetchLatest(); specifically, modify
Mailgun.test.js to construct Mailgun with the stubbed provider instances (or
adjust Mailgun's constructor to accept an options object with
emailProvider/analyticsProvider) rather than assigning to
_Mailgun__emailProvider/_Mailgun__analyticsProvider, so send() and fetchLatest()
will delegate to the stubs.
|
I really hope this gets merged this time. I do admin for a friend and we went with self-hosting Ghost as a Patreon alternative. That was 2 years ago, and we’ve never had a newsletter go out once to our ~25 recipients because Mailgun limits free accounts to less than 10 recipients at a time. For the past 2 days I have been trying to upgrade the Mailgun account from numerous devices, browsers, and credit cards, and their system fails to submit the purchase every time. I filed a ticket and their customer service is abysmal, replying with the FAQ link and telling me to try things I documented in the ticket already. I started looking at alternatives to Ghost this morning because Mailgun clearly isn’t interested in resolving my issue, and a friend I’d vented to ended up seeing this and sending it my way. If this gets merged it will save a lot of frustration and work for us and our subscribers. Please consider it. |
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.
After a couple of tries in the past years, I am giving this one more go.
I understand and respect that the core team has no interest in maintaining other mail providers than Mailgun. But in the interest of the community, it would be great to allow other providers via 3rd party packages. We, for example, have been banned from Mailgun without any explanation and cannot restore our account.
In this implementation, I try to make as few changes as possible (even not moving the email-provider-mailgun and email-analytics-provider-mailgun; ideally, they move into the adapter as well). This way, I hope to reduce the load on the core team from this change while enabling anyone else to implement new email providers.
The implementation of the Postmark provider can be found at madewithlove/ghost-email-postmark.