From acff6ab650f82a0ae50806673fcd4cb0af49a122 Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Wed, 29 Jul 2026 18:52:48 +0600 Subject: [PATCH] fix(email): avoid nested site_start link tags in unsubscribe URLs Provide [[+unsubscribe_url]] at send time and flatten residual [[~[[N]]]] patterns so the MODX parser no longer logs Bad link tag. --- README.md | 2 +- core/components/sendex/docs/changelog.txt | 1 + .../elements/templates/template.sendex.tpl | 16 ++--- .../sendex/sxqueuebodyrenderer.class.php | 64 ++++++++++++++++++- tests/Stubs/FakeModX.php | 23 +++++++ tests/Unit/QueueBodyRendererTest.php | 59 +++++++++++++++++ tests/Unit/UnsubscribeResolveTest.php | 6 +- 7 files changed, 158 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 2bd1bec..09126a3 100644 --- a/README.md +++ b/README.md @@ -135,7 +135,7 @@ The page must call `[[!Sendex? &id=...]]` (any newsletter id is fine). Query par | `code` | yes | `sxSubscriber.code` | | `newsletter_id` | no | Same as newsletter id; avoids confusion with MODX resource `id`. Snippet resolves the owner newsletter from `code` if the snippet `&id` differs. | -Default letter template links to `site_start` with `sx_action`, `newsletter_id`, and `code`. +Default letter template uses `[[+unsubscribe_url]]` (built at send time via `makeUrl` on `sendex_unsubscribe_page` or `site_start`). Do **not** nest `[[++site_start]]` inside `[[~…]]` — that becomes `[[~[[57]]]]` and logs "Bad link tag". ## Cron diff --git a/core/components/sendex/docs/changelog.txt b/core/components/sendex/docs/changelog.txt index 4ff2c16..6fea2e6 100644 --- a/core/components/sendex/docs/changelog.txt +++ b/core/components/sendex/docs/changelog.txt @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [2.0.1-pl] - 2026-07-29 ### Fixed +- Email templates: nested `[[~[[++site_start]]]]` unsubscribe links became `[[~[[57]]]]` and logged "Bad link tag". Queue body render now provides `[[+unsubscribe_url]]`, flattens residual nested `[[~[[N]]]]` before parse, and documents `sendex_unsubscribe_page` (fallback: `site_start`). - [#119] Mgr row-action icon buttons (edit/disable/send/remove) did nothing when the click hit the inner ``: shared `SelectionMixin.onClick` now finds the button via `getTarget('button')`, resolves the row via `findRowIndex`, and reads the action from `data-action`. - [#114] Mgr newsletter create appeared to hang on «Загружается…» and the grid stayed empty after reload (row was saved; duplicate-name error on retry). Newsletter `getlist` no longer uses JOIN/subquery SQL (subscriber count and template name are added in `prepareRow`); grid refresh after save is deferred so the create window can close first; `getlist`/`get` accept `view_sendex` as well as `view_document`; create `beforeSet()` returns strict `true` for MODX 3. Image column renderer uses `Sendex.utils.escapeHtmlAttr` (ExtJS does not keep grid scope for column renderers). - [#111] Mgr row-action and menu icons no longer force `font-family: "Font Awesome 5 Free"` (Sendex does not load FA5); icons inherit the mgr icon font on MODX 2.3+/3.x or bundled FA4 on older MODX. diff --git a/core/components/sendex/elements/templates/template.sendex.tpl b/core/components/sendex/elements/templates/template.sendex.tpl index aa355ac..486be1f 100644 --- a/core/components/sendex/elements/templates/template.sendex.tpl +++ b/core/components/sendex/elements/templates/template.sendex.tpl @@ -52,12 +52,12 @@

Link for unsubscribe

-Link must lead to a page that calls the Sendex snippet. Required query params: -sx_action=unsubscribe, code (subscriber code). Optional: -newsletter_id (same as [[+newsletter.id]]; the snippet also resolves the newsletter from code if the snippet &id differs). -
-
[[~id_of_resource?scheme=`full`&sx_action=`unsubscribe`&newsletter_id=`[[+newsletter.id]]`&code=`[[+subscriber.code]]`]]
+

Prefer the ready-made placeholder (built in PHP — avoids nested link tags):

+
[[+unsubscribe_url]]
+

Example:

+Unsubscribe from this newsletter -

-For example (works on site_start even when the snippet &id is another newsletter):
-Unsubscribe from this newsletter +

If you build the URL yourself, use a numeric resource id (not [[++site_start]] inside [[~…]]). +Required query params: sx_action=unsubscribe, code. Optional: newsletter_id.

+
[[~id_of_resource?scheme=`full`&sx_action=`unsubscribe`&newsletter_id=`[[+newsletter.id]]`&code=`[[+subscriber.code]]`]]
+

Optional system setting sendex_unsubscribe_page overrides site_start for [[+unsubscribe_url]].

diff --git a/core/components/sendex/model/sendex/sxqueuebodyrenderer.class.php b/core/components/sendex/model/sendex/sxqueuebodyrenderer.class.php index a803adf..45b2ee3 100644 --- a/core/components/sendex/model/sendex/sxqueuebodyrenderer.class.php +++ b/core/components/sendex/model/sendex/sxqueuebodyrenderer.class.php @@ -62,8 +62,9 @@ public static function render($xpdo, $newsletter, $subscriber) } $scriptProperties = array( - 'newsletter' => $newsletter->toArray(), - 'subscriber' => $subscriber->toArray(), + 'newsletter' => $newsletter->toArray(), + 'subscriber' => $subscriber->toArray(), + 'unsubscribe_url' => self::buildUnsubscribeUrl($xpdo, $newsletter, $subscriber), ); $userId = (int) $subscriber->get('user_id'); @@ -82,6 +83,9 @@ public static function render($xpdo, $newsletter, $subscriber) $template->_output = ''; $body = $template->process($scriptProperties); + // Nested [[~[[++site_start]]]] becomes [[~[[57]]]] after ++ expands — invalid link tag. + $body = self::flattenNestedResourceLinks($body, (int) $xpdo->getOption('site_start')); + /** @var modParser|null $parser */ $parser = sxModxCompat::getParser($xpdo); if ($parser && $parser instanceof modParser) { @@ -92,6 +96,62 @@ public static function render($xpdo, $newsletter, $subscriber) return $body; } + /** + * Absolute unsubscribe URL for email templates (avoids nested [[~[[++site_start]]]]). + * + * @param object $xpdo + * @param object $newsletter + * @param object $subscriber + * @return string + */ + public static function buildUnsubscribeUrl($xpdo, $newsletter, $subscriber) + { + $resourceId = (int) $xpdo->getOption('sendex_unsubscribe_page', null, 0); + if ($resourceId <= 0) { + $resourceId = (int) $xpdo->getOption('site_start'); + } + if ($resourceId <= 0 || !method_exists($xpdo, 'makeUrl')) { + return ''; + } + + $params = array( + 'sx_action' => 'unsubscribe', + 'newsletter_id' => (int) $newsletter->get('id'), + 'code' => (string) $subscriber->get('code'), + ); + + $url = $xpdo->makeUrl($resourceId, '', $params, 'full'); + + return is_string($url) ? $url : ''; + } + + /** + * Rewrite [[~[[++site_start]]…]] / residual [[~[[123]]…]] into [[~123…]]. + * + * @param string $body + * @param int $siteStart + * @return string + */ + public static function flattenNestedResourceLinks($body, $siteStart) + { + $siteStart = (int) $siteStart; + if ($siteStart <= 0 || !is_string($body) || $body === '') { + return $body; + } + + $body = preg_replace( + '/\[\[~\s*\[\[\+\+site_start\]\]/', + '[[~' . $siteStart, + $body + ); + + return preg_replace( + '/\[\[~\s*\[\[(\d+)\]\]/', + '[[~$1', + $body + ); + } + /** * @param object $xpdo * @param object $queue sxQueue-like diff --git a/tests/Stubs/FakeModX.php b/tests/Stubs/FakeModX.php index b303afc..1be77cf 100644 --- a/tests/Stubs/FakeModX.php +++ b/tests/Stubs/FakeModX.php @@ -565,6 +565,29 @@ public function getTableName($class) return $class; } + /** + * @param int|string $id + * @param string $context + * @param array|string $args + * @param mixed $scheme + * @return string + */ + public function makeUrl($id, $context = '', $args = array(), $scheme = -1) + { + if (is_array($args)) { + $query = http_build_query($args); + } else { + $query = (string) $args; + } + + $url = 'https://example.com/index.php?id=' . (int) $id; + if ($query !== '') { + $url .= '&' . $query; + } + + return $url; + } + /** * @return FakePdoConnection */ diff --git a/tests/Unit/QueueBodyRendererTest.php b/tests/Unit/QueueBodyRendererTest.php index 51abf03..d2b6521 100644 --- a/tests/Unit/QueueBodyRendererTest.php +++ b/tests/Unit/QueueBodyRendererTest.php @@ -113,4 +113,63 @@ public function testDeliverMailRendersCompactBodyAtSendTime() $this->assertTrue(sxQueueSender::deliverMail($queue)); $this->assertSame('Body for send@example.com', $mail->sets[modMail::MAIL_BODY]); } + + public function testFlattenNestedResourceLinksRewritesSiteStartAndNumericNesting() + { + $nested = 'x'; + $this->assertSame( + 'x', + sxQueueBodyRenderer::flattenNestedResourceLinks($nested, 57) + ); + + $residual = '[[~[[57]]?code=`abc`]]'; + $this->assertSame( + '[[~57?code=`abc`]]', + sxQueueBodyRenderer::flattenNestedResourceLinks($residual, 57) + ); + } + + public function testBuildUnsubscribeUrlUsesSiteStartAndParams() + { + $this->modx->options['site_start'] = 57; + + $newsletter = new TestableNewsletter($this->modx); + $newsletter->set('id', 1); + + $subscriber = new sxSubscriber($this->modx); + $subscriber->fromArray(array( + 'id' => 2, + 'newsletter_id' => 1, + 'code' => 'deadbeef', + 'email' => 'a@example.com', + )); + + $url = sxQueueBodyRenderer::buildUnsubscribeUrl($this->modx, $newsletter, $subscriber); + + $this->assertStringContainsString('id=57', $url); + $this->assertStringContainsString('sx_action=unsubscribe', $url); + $this->assertStringContainsString('newsletter_id=1', $url); + $this->assertStringContainsString('code=deadbeef', $url); + } + + public function testBuildUnsubscribeUrlPrefersSendexUnsubscribePage() + { + $this->modx->options['site_start'] = 57; + $this->modx->options['sendex_unsubscribe_page'] = 12; + + $newsletter = new TestableNewsletter($this->modx); + $newsletter->set('id', 3); + + $subscriber = new sxSubscriber($this->modx); + $subscriber->fromArray(array( + 'id' => 4, + 'newsletter_id' => 3, + 'code' => 'c0de', + 'email' => 'b@example.com', + )); + + $url = sxQueueBodyRenderer::buildUnsubscribeUrl($this->modx, $newsletter, $subscriber); + $this->assertStringContainsString('id=12', $url); + $this->assertStringNotContainsString('id=57', $url); + } } diff --git a/tests/Unit/UnsubscribeResolveTest.php b/tests/Unit/UnsubscribeResolveTest.php index 0f3aef5..698625f 100644 --- a/tests/Unit/UnsubscribeResolveTest.php +++ b/tests/Unit/UnsubscribeResolveTest.php @@ -78,9 +78,11 @@ public function testTemplateIncludesNewsletterIdQueryParam() dirname(__DIR__, 2) . '/core/components/sendex/elements/templates/template.sendex.tpl' ); + $this->assertStringContainsString('[[+unsubscribe_url]]', $template); $this->assertStringContainsString('sx_action=`unsubscribe`', $template); - $this->assertStringContainsString('newsletter_id=`[[+newsletter.id]]`', $template); - $this->assertStringContainsString('code=`[[+subscriber.code]]`', $template); + $this->assertStringContainsString('newsletter_id=`[[+newsletter.id]]`', $template); + $this->assertStringContainsString('code=`[[+subscriber.code]]`', $template); + $this->assertStringNotContainsString('[[~[[++site_start]]', $template); } public function testSnippetResolvesByCodeBeforeUnsubscribe()