Fix nested chunk tags when the chunk name contains a tag

[Ibochkarev]

What changed and why

The three-level repro from #13043 fails because parser merge used sequential str_replace: a shorter tag key (like [[*id]]) could rewrite text that still contained a longer collected key from the same pass.

mergeTagOutput() now uses strtr($content, $tagMap).

This fixes the overlap bug because strtr:

• matches the longest key at each position
• does one-pass substitution and does not re-scan already inserted output

That behavior is exactly what the nested-chunk case needs.

How to test

1. Create chunk chunk-1 with the markup from Parser issue with nested elements #13043 (level placeholder + [[+nestedcontent]]).
2. On resource 1, paste the three-level [[$chunk-[[*id]]]] snippet from the issue.
3. View the resource. You should see three nested wrappers and the inner sentence.

Commands:

• php -l core/src/Revolution/modParser.php → exit 0
• core/vendor/bin/phpunit --enforce-time-limit -c _build/test/phpunit.xml --filter modParserTest → OK (91 tests, 138 assertions)

Related issue(s)/PR(s)

Resolves #13043

Refs #13044 (merged then reverted; that approach changed depth behavior by passing parser_max_iterations through processTag).

Compatibility notes

Core parser merge only. No DB/setup migration.

Breaking change assessment

No public API signature changes.

Behavioral change: with strtr, replacement keys no longer re-apply inside output inserted earlier in the same merge pass. For unresolved nested tags at depth=0, this can defer some substitutions to the next parser iteration instead of resolving them inside one merge pass. Resource rendering remains aligned with default iterative parser flow.

Test coverage

_build/test/Tests/Model/modParserTest.php

• testMergeTagOutputOverlappingNestedTags pins the overlap bug
• testNestedChunkNameContainsTag verifies two and three levels
• testMergedOutputKeepsDeferredUncacheableTags
• testMergedOutputKeepsDeferredUncacheableSnippetExecutions
• providerProcessElementTags keeps both is2 depth=2 rows (processed=1) plus existing is3 depth coverage

Contributors

@christianseel reported the repro and opened #13044. @opengeek validated edge cases and review scenarios.

AI tool use

Cursor assisted with implementation and test drafting; validation/testing was run in the local repository.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 21.82%. Comparing base (5a484de) to head (e14715c).
⚠️ Report is 25 commits behind head on 3.x.

Additional details and impacted files

@@             Coverage Diff              @@
##                3.x   #16991      +/-   ##
============================================
+ Coverage     21.67%   21.82%   +0.15%     
- Complexity    10786    10799      +13     
============================================
  Files           566      566              
  Lines         33149    33205      +56     
============================================
+ Hits           7186     7248      +62     
+ Misses        25963    25957       -6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mkschell]

I tested this branch. The new merge order causes a regression for a documented pattern: cacheable elements that output uncacheable tags.

When a cacheable element renders, the parser does not process the [[!...]] tags in its output (parser_recurse_uncacheable, modParser::processTag()). These tags stay in the content after the merge. The parser processes them on the next iteration, with the state at that time. The uksort breaks this behavior. The longest-key-first sort prevents one problem: a short key can no longer overwrite a longer key in the content (the #13043 failure). But it causes a new problem with the output of longer keys: a shorter tag's replacement now also applies inside output that was merged just before it, in the same str_replace pass.

There are two observable failures. Both occur on the uncached pass (modResource::process()):

1. Stale values. A cacheable snippet sets a placeholder. It returns output that contains the uncacheable tag for that placeholder. The same tag also occurs earlier in the document:

Header shows [[!+greeting]] ... [[SetGreeting]]

At parse start, greeting = BEFORE. The snippet SetGreeting does $modx->setPlaceholder('greeting', 'AFTER'); return 'tpl says [[!+greeting]]';. The results are:

• 3.x: Header shows BEFORE ... tpl says AFTER
• This PR: Header shows BEFORE ... tpl says BEFORE — the shorter key replaces the tag inside the merged snippet output. It uses the value from before the snippet ran.

2. Lost executions. An uncacheable snippet is called standalone. A cacheable chunk also outputs the same snippet tag (<div>[[!uid]]</div>):

[[!uid]] ... [[$wrapper]]

• 3.x: UID1 ... <div>UID2</div> — two executions
• This PR: UID1 ... <div>UID1</div> — the parser copies the output of the standalone call as text. The second execution does not occur. This breaks all per-call values (nonces, unique IDs, counters).

In both cases, the shorter tag must occur earlier in the document than the longer element call. This is the order that the current merge protects.

The test cases below are for modParserTest.php. They pass on 3.x and fail on this branch. A possible fix satisfies these tests and this PR's own tests: replace the uksort + str_replace with strtr($content, $tagMap). strtr matches the longest key first at each position. It does not scan replaced substrings again. This is the necessary behavior. One difference from 3.x: str_replace can also substitute a later key inside output merged earlier in the same pass, while strtr defers that tag to the next iteration. This only changes the result if $depth runs out before the tag is processed. I verified the fix against the two scenarios above and against the expected output of testMergeTagOutputOverlappingNestedTags. The full test suite must confirm the result.

/**
 * A cacheable element's output may contain uncacheable tags, deferred to a later
 * iteration when parser_recurse_uncacheable is enabled (the default). Merge order
 * must not rewrite those emitted tags with values captured before the element executed.
 */
public function testMergedOutputKeepsDeferredUncacheableTags()
{
    $name = 'sg16991xx' . bin2hex(random_bytes(4)); // snippet tag must be longer than the placeholder tag
    $snippet = $this->modx->newObject(modSnippet::class);
    $snippet->set('name', $name);
    $snippet->set(
        'snippet',
        '$modx->setPlaceholder(\'greeting16991\', \'AFTER\'); return \'tpl says [[!+greeting16991]]\';'
    );
    $this->assertTrue($snippet->save());

    $this->modx->setPlaceholder('greeting16991', 'BEFORE');

    /* cacheable snippet call; same signature as the uncached pass in modResource::process() */
    $content = "Header shows [[!+greeting16991]] ... [[{$name}]]";
    try {
        $this->modx->parser->processElementTags('', $content, true, false, '[[', ']]', [], 10);
        $this->assertStringContainsString('Header shows BEFORE', $content);
        $this->assertStringContainsString('tpl says AFTER', $content);
    } finally {
        $snippet->remove();
        $this->modx->unsetPlaceholder('greeting16991');
    }
}

/**
 * An uncacheable snippet called standalone and also emitted by a cacheable chunk
 * must execute once per occurrence, not have the standalone output duplicated.
 */
public function testMergedOutputKeepsDeferredUncacheableSnippetExecutions()
{
    $suffix = bin2hex(random_bytes(4));
    $snipName = 'uid16991' . $suffix;
    $chunkName = 'wrap16991xx' . $suffix; // chunk tag must be longer than the snippet tag

    $snippet = $this->modx->newObject(modSnippet::class);
    $snippet->set('name', $snipName);
    $snippet->set(
        'snippet',
        '$n = (int) $modx->getPlaceholder(\'uidcount16991\') + 1;'
        . '$modx->setPlaceholder(\'uidcount16991\', $n);'
        . 'return \'UID\' . $n;'
    );
    $this->assertTrue($snippet->save());

    $chunk = $this->modx->newObject(modChunk::class);
    $chunk->set('name', $chunkName);
    $chunk->set('snippet', "<div>[[!{$snipName}]]</div>");
    $this->assertTrue($chunk->save());

    $content = "[[!{$snipName}]] ... [[\${$chunkName}]]";
    try {
        $this->modx->parser->processElementTags('', $content, true, false, '[[', ']]', [], 10);
        $this->assertStringContainsString('UID1', $content);
        $this->assertStringContainsString('UID2', $content);
        $this->assertSame(2, (int) $this->modx->getPlaceholder('uidcount16991'));
    } finally {
        $snippet->remove();
        $chunk->remove();
        $this->modx->unsetPlaceholder('uidcount16991');
    }
}

[Ibochkarev]

@mkschell Thanks for the review! I'll be back with a fix soon.

[Ibochkarev]

@mkschell Good catch on the sequential str_replace re-scanning merged output.

I switched mergeTagOutput to strtr($content, $tagMap) and added both test cases to modParserTest.php. strtr matches the longest key at each position in one pass without re-scanning inserted output. All 89 parser tests pass.

[rthrash]

Test Pilot result: Pass

• Pull request: #16991
• Revision tested: 9a67a01e1cc7cbc0ecb99666250f22f051834e49
• MODX: 3.2.3-pl
• PHP: 8.3.13
• Site type: Staging

Testing notes

I have not tested more extensively, but this seems solid to me with the test case supplied. Here's my setup content on the page with a variety of Snippets, custom code/plugins, etc.

Before this PR applied, definitely broken output (it really broke the page, badly).

After, this worked as expected. I did not notice any other parts of the site not working as expected.

Given that this is changing how the parser does it thing, it might make more sense for others with larger sites and more Snippet calls to also test.

Generated by Test Pilot

[opengeek]

Thanks for picking this one up, @Ibochkarev — #13043 has been sitting a long time, and the three-level repro is a real bug. The one-line change in modParser::mergeTagOutput() is the right fix. I'm requesting changes on the description and two test rows, not on the approach.

I checked this out locally and ran it against DDEV (PHP 8.1 / MariaDB 10.11). Details below.

---

1. The description doesn't match the code — and the described version is broken

The body says:

I sort the map longest-key-first in mergeTagOutput before replace.

The diff does strtr($content, $tagMap). Those aren't equivalent, and the difference isn't cosmetic. I implemented what the description says — uksort($tagMap, fn($a, $b) => strlen($b) <=> strlen($a)) followed by the original str_replace — and ran the suite:

1) modParserTest::testMergedOutputKeepsDeferredUncacheableTags
   Failed asserting that 'Header shows BEFORE ... tpl says BEFORE' contains "tpl says AFTER".

2) modParserTest::testMergedOutputKeepsDeferredUncacheableSnippetExecutions
   Failed asserting that 'UID1 ... <div>UID1</div>' contains "UID2".

Sorting longest-first makes the shorter key replace last, so it rewrites content the longer key just emitted — stale placeholder values and duplicated snippet output. Your two MergedOutputKeepsDeferred* tests are exactly what catches that, and they're the strongest thing in this PR.

So: the code is right, the write-up describes a change that would be a regression. Please rewrite "What changed and why" to describe strtr and what it actually does. That section is what the rest of the team reviews from and what ends up in the changelog.

2. Two of the removed provider rows should come back

Four rows came out of providerProcessElementTags. I ran each against today's 3.x, the sorted variant, and this PR:

Removed row	Old expectation	Under this PR
multiline is2, depth 0	processed=1, unresolved inner tag	Obsolete — it pinned the bug. Correctly replaced.
single-line is2, depth 0	processed=1, [[+is2:is=2…]]	Obsolete — same.
multiline is2, depth 2	processed=2, "\n 2\n"	Still valid. Produces processed=1, identical content.
single-line is2, depth 2	processed=2, "2"	Still valid. Produces processed=1, identical content.

The first two had to go — no argument there. The last two only needed 'processed' => 2 changed to 1, which is exactly the treatment the is3 depth 2 row got and kept in this same diff.

"Redundant is2 depth-2 rows removed" undersells what a depth > 0 row is for. It isn't a duplicate of the depth 0 row now that both produce the same content — it's the assertion that a second recursive pass leaves already-final content alone. That's precisely the property this change perturbs, so it's the coverage I least want to lose here. After this PR the only depth > 0 row left in that family is the single-line is3 case; nothing exercises a multiline nested filter through recursion.

I restored both rows with 'processed' => 1 and the suite is green at 91 tests / 138 assertions. They also fail against unmodified 3.x, so they pin the new behavior rather than just riding along.

3. The real semantic change is broader than the PR states

strtr differs from str_replace in two ways. The description covers one:

1. Longest match wins at each position — this is the #13043 fix.
2. It never re-scans replaced output. str_replace applies each key across the whole current subject, so key i can rewrite text that key j<i just inserted.

(2) is unmentioned and it's the one with reach. It touches the deferred-uncacheable path — modParser.php:433, where a cacheable element processed during an uncacheable pass has _processingUncacheable flipped off, so [[!…]] tags in its output survive to the parent merge. With a chunk emitting [[!+ph]] and the same tag standalone later in the content:

depth 0    3.x → '<div>VAL</div> | VAL'      this PR → '<div>[[!+ph]]</div> | VAL'
depth 10   3.x → '<div>VAL</div> | VAL'      this PR → '<div>VAL</div> | VAL'

They converge at depth ≥ 1, so the resource pipeline is fine — it uses parser_max_iterations (default 10). The exposure is the depth 0 callers:

• modParsedManagerController.php:62 — processElementTags('', $html, true, true) on manager page HTML. Note removeUnprocessed only strips tags that fail during collection, not ones inserted at merge time, so a raw [[!…]] could render.
• modElement.php:696 — property_preprocess property values
• modStaticResource.php:48,65 and modElement.php:587,628 — filename / source path
• Sources/modMediaSource.php:1435,2048

I'm not treating this as a blocker. It needs the same uncacheable tag both emitted by a cacheable element and standalone later in one string, at a depth-0 site — and the old behavior was already order-dependent, since reversing the two leaves the tag raw on 3.x today too. But it belongs in "Breaking change assessment", which currently only mentions leftover markup.

Also worth stating: processed drops from 2 to 1 for these cases. Only modElement::getProperties() consumes the return value, as a boolean, so there's no functional impact — but the two removed depth 2 rows were what pinned it.

4. One thing the PR undersells

strtr makes a single pass; str_replace with N keys makes N full passes over the content. On 87KB of content with a 200-key tag map, 200 iterations:

str_replace: 2.852s
strtr      : 0.022s

Same output. This is a hot path in every page render, and it's a better argument for strtr over the sorted variant than the one currently being made. Please add it.

Verification

• Full suite on this branch: 759 tests, 1038 assertions, green (6 skipped / 3 incomplete, all pre-existing on 3.x).
• New tests against unmodified mergeTagOutput: 5 failures, so the fix is genuinely pinned. testNestedChunkNameContainsTag "two levels" passes on 3.x — only "three levels" fails, which matches the issue report.
• phpcs on both changed files: 15 errors / 20 warnings vs 16 / 20 on base. No new violations.
• Non-string tag outputs (array / int / float / bool / object) behave identically under strtr and str_replace, so there's no new fatal-error edge for a badly-behaved snippet.

Test nits (non-blocking)

• Both new element-creating tests name fixtures with bin2hex(random_bytes(…)). Non-deterministic fixtures make failures hard to reproduce and depart from this file's setUpFixturesBeforeClass convention. Prefer fixed names with cleanup in @before/@after.
• testMergedOutputKeepsDeferredUncacheableTags only pins what it's meant to because [[!+greeting16991]] appears before the snippet call. Reorder them and it silently stops testing anything. Worth a comment saying the ordering is the point.
• $chunk->setCacheable(false) in testNestedChunkNameContainsTag sets xPDO object cacheability, not element cacheability — harmless, but it reads as if it's doing something it isn't.
• providerNestedChunkNameContainsTag sits after its tests, between other test methods. The rest of the file keeps providers next to what they feed.

---

To summarize what I need before merge:

1. Rewrite "What changed and why" and "Breaking change assessment" to describe strtr and the no-re-scan semantic.
2. Restore the two is2 depth 2 provider rows with 'processed' => 1.

The nits and the benchmark are welcome but won't hold this up. Nice work tracking down the actual merge-order cause — that's the part previous attempts at #13043 missed.

[Ibochkarev]

@opengeek Thanks for the detailed review — I applied the two requested blocking changes.

1. I rewrote the PR description to match the actual implementation (strtr) and documented the no-re-scan behavior in the breaking-change section.
2. I restored the two is2 depth=2 rows in providerProcessElementTags with processed => 1.

Verification on this branch:

• php -l _build/test/Tests/Model/modParserTest.php
• core/vendor/bin/phpunit --enforce-time-limit -c _build/test/phpunit.xml --filter modParserTest
• Result: OK (91 tests, 138 assertions)

If you want, I can follow up with a small cleanup pass for the non-blocking test nits in a separate commit.