Redeclaring a layout fragment in a decoration template that is part of a hierarchy of nested templates yields the wrong results #200

silkentrance · 2020-03-07T20:50:12Z

Having a template in a hierarchy of nested templates, that redeclares a layout fragment that was already declared in the decorated base template at the root of the hierarchy, and with the content template once again declaring the same layout fragment, the fragment declared by the decorated template will be used instead of that from the content template.

Example


# Test a deep layout hierarchy (3 levels) with redeclaration of layout fragments.

%TEMPLATE_MODE HTML


%INPUT
<!DOCTYPE html>
<html xmlns:layout="http://www.ultraq.net.nz/thymeleaf/layout"
	layout:decorate="~{Parent}">
<head>
	<title>Page title</title>
	<script src="child-script.js"></script>
</head>
<body>
	<div layout:fragment="content">
		<p>Page content</p>
	</div>
	<footer layout:fragment="footer">
	  <p>Page footer</p>
	</footer>
</body>
</html>


%INPUT[Parent]
<!DOCTYPE html>
<html xmlns:layout="http://www.ultraq.net.nz/thymeleaf/layout"
	layout:decorate="~{Grandparent}">
<head>
	<script src="parent-script.js"></script>
</head>
<body>
	<section layout:fragment="section">
		<header>
			<h1>My website</h1>
		</header>
		<div layout:fragment="content">
			<p>Parent content</p>
		</div>
	</section>
	<!-- parent redeclares footer -->
	<footer layout:fragment="footer">
		<p>Parent footer</p>
	</footer>
</body>
</html>


%INPUT[Grandparent]
<!DOCTYPE html>
<html xmlns:layout="http://www.ultraq.net.nz/thymeleaf/layout">
<head>
	<script src="grandparent-script.js"></script>
</head>
<body>
	<section layout:fragment="section">
		<p>Grandparent section</p>
	</section>
	<footer layout:fragment="footer">
		<p>Grandparent footer</p>
	</footer>
</body>
</html>


%OUTPUT
<!DOCTYPE html>
<html>
<head>
	<title>Page title</title>
	<script src="grandparent-script.js"></script>
	<script src="parent-script.js"></script>
	<script src="child-script.js"></script>
</head>
<body>
	<section>
		<header>
			<h1>My website</h1>
		</header>
		<div>
			<p>Page content</p>
		</div>
	</section>
	<footer>
		<p>Page footer</p>
	</footer>
</body>
</html>

The above will fail with the following output

Obtained:
[ion>
	<footer>
		<p>Parent footer</p>
	</footer>
</bo]
at line 19 col 6, but expected:
[ion>
	<footer>
		<p>Page footer</p>
	</footer>

A remedy for this is to change the FragmentProcessor to use the last fragment instead of the first, e.g.

def fragment = fragments.size() == 1 ? fragments.first() : fragements.last()

Doing so will make the tests for infinite loops fail, so this might be more involved.

The text was updated successfully, but these errors were encountered:

silkentrance · 2020-03-07T22:14:57Z

Fixing the include/replace/insert problem without just documenting the fact that one must not redeclare a given layout:fragment, requires changes to the FragmentMap, e.g.


	/**
	 * Set the fragment collection to contain whatever it initially had, plus the
	 * given fragments, just for the scope of the current node.
	 *
	 * This must be used when processing non include/insert/replace related directives.
	 * 
	 * @param context
	 * @param structureHandler
	 * @param fragments The new fragments to add to the map.
	 */
	static void setForNode(IContext context, IElementModelStructureHandler structureHandler,
		Map<String,List<IModel>> fragments) {
		setForNodeInternal(context, structureHandler, fragments, false);
	}

	/**
	 * Set the fragment collection to contain whatever it initially had, plus the
	 * given fragments, just for the scope of the current node.
	 *
	 * This must be used when processing include/insert/replace related directives.
	 *
	 * @param context
	 * @param structureHandler
	 * @param fragments The new fragments to add to the map.
	 */
	static void setForNodeIncludeProcessing(IContext context, IElementModelStructureHandler structureHandler,
		Map<String,List<IModel>> fragments) {
		setForNodeInternal(context, structureHandler, fragments, true);
	}

	private static void setForNodeInternal(IContext context, IElementModelStructureHandler structureHandler,
		Map<String,List<IModel>> fragments, boolean isIncludeProcessing) {

		structureHandler.setLocalVariable(FRAGMENT_COLLECTION_KEY,
			get(context).inject(fragments.clone()) { accumulator, fragmentName, fragmentList ->
				if (accumulator[fragmentName]) {
					accumulator[fragmentName] += fragmentList
					if (isIncludeProcessing) {
						accumulator[fragmentName] = ((List) accumulator[fragmentName]).reverse()
					}
				} else {
					accumulator[fragmentName] = fragmentList
				}
				return accumulator
			}
		)
	}

Also, IncludeProcessor, InsertProcessor and ReplaceProcessor must now call setForNodeIncludeProcessing instead of just setForNode.

By making this change, everything seems to be working just fine.

…n hierarchies must produce the expected result

didiez · 2020-12-21T17:20:50Z

Any progress on this? We got bitten by this issue upgrading from 2.3.0 to 2.5.1.
We have downgraded again to 2.3.0 as it is a blocking issue for us.

ultraq · 2020-12-22T07:47:56Z

There's a linked PR to fix this issue, and I had some changes I wanted to see made to it in my review, but I haven't heard back from the OP about updates 😕 I'll apply the PR + changes and release a 2.5.2-SNAPSHOT in the coming days.

didiez · 2020-12-22T10:40:27Z

Great! thanks @ultraq :)

silkentrance · 2020-12-22T13:10:17Z

@ultraq Oh I am sorry. I completely forgot about this 😄 . And what is worse is that my MB Pro broke and I am no longer able to access the worktree where I already implemented some of the suggested changes.

ultraq · 2020-12-23T05:07:32Z

That's OK @silkentrance - it turns out I wrote some pretty thorough review notes, so I was planning to merge your PR as is, and then apply exactly what I wrote in that PR on top of your changes 😁

fix #200: redeclaration of layout fragments in nested decoration hierarchies must produce the expected result

ultraq · 2020-12-28T01:35:41Z

Version 2.5.2-SNAPSHOT, which contains the linked PR and merged into the latest code, is now available to try.

didiez · 2020-12-29T13:15:03Z

Version 2.5.2-SNAPSHOT, which contains the linked PR and merged into the latest code, is now available to try.

It works like a charm. Thanks for the quick fix!
Is there any ETA for the next release?

ultraq · 2020-12-30T21:01:05Z

Awesome, I'll make a release some time this weekend and post in this issue when it's available

ultraq · 2021-01-03T02:15:08Z

Version 2.5.2 is out now 😁 It's making its way through maven central now so should show up there soon

silkentrance added a commit to coldrye-collaboration/thymeleaf-layout-dialect that referenced this issue Mar 7, 2020

fix ultraq#200: redeclaration of layout fragments in nested decoratio…

2a94d0b

…n hierarchies must produce the expected result

coveralls mentioned this issue Mar 7, 2020

fix #200: redeclaration of layout fragments in nested decoration hierarchies must produce the expected result #201

Merged

ultraq added the enhancement label Mar 9, 2020

ultraq added this to the Thymeleaf Layout Dialect 2.5.0 milestone Mar 11, 2020

ultraq added a commit that referenced this issue Dec 24, 2020

Merge pull request #201 from coldrye-collaboration/gh-200

e87da3c

fix #200: redeclaration of layout fragments in nested decoration hierarchies must produce the expected result

ultraq closed this as completed in 9db661e Dec 24, 2020

ultraq reopened this Dec 24, 2020

ultraq self-assigned this Dec 30, 2020

ultraq added bug and removed enhancement labels Dec 30, 2020

ultraq closed this as completed Jan 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redeclaring a layout fragment in a decoration template that is part of a hierarchy of nested templates yields the wrong results #200

Redeclaring a layout fragment in a decoration template that is part of a hierarchy of nested templates yields the wrong results #200

silkentrance commented Mar 7, 2020 •

edited

silkentrance commented Mar 7, 2020

didiez commented Dec 21, 2020

ultraq commented Dec 22, 2020

didiez commented Dec 22, 2020

silkentrance commented Dec 22, 2020

ultraq commented Dec 23, 2020

ultraq commented Dec 28, 2020

didiez commented Dec 29, 2020

ultraq commented Dec 30, 2020

ultraq commented Jan 3, 2021

Redeclaring a layout fragment in a decoration template that is part of a hierarchy of nested templates yields the wrong results #200

Redeclaring a layout fragment in a decoration template that is part of a hierarchy of nested templates yields the wrong results #200

Comments

silkentrance commented Mar 7, 2020 • edited

silkentrance commented Mar 7, 2020

didiez commented Dec 21, 2020

ultraq commented Dec 22, 2020

didiez commented Dec 22, 2020

silkentrance commented Dec 22, 2020

ultraq commented Dec 23, 2020

ultraq commented Dec 28, 2020

didiez commented Dec 29, 2020

ultraq commented Dec 30, 2020

ultraq commented Jan 3, 2021

silkentrance commented Mar 7, 2020 •

edited