Closed silkentrance closed 3 years ago
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.
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.
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.
Great! thanks @ultraq :)
@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.
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 😁
Version 2.5.2-SNAPSHOT, which contains the linked PR and merged into the latest code, is now available to try.
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?
Awesome, I'll make a release some time this weekend and post in this issue when it's available
Version 2.5.2 is out now 😁 It's making its way through maven central now so should show up there soon
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
The above will fail with the following output
A remedy for this is to change the FragmentProcessor to use the last fragment instead of the first, e.g.
Doing so will make the tests for infinite loops fail, so this might be more involved.