fix: layout snippet inside of snippet (#7567)

[JojOatXGME]

Description

Fixes #7567. Implement handling of layout snippets within snippets.

Extract implementation from Template::render into Snippet::load. Snippet::load overrides Tpl::load. The calls to static::load inside Snippet::render and Snippet::factory are now referencing Snippet::load instead of Tpl::load. As a result, these two methods are now correctly handling layout snippets.

Changelog

🐛 Bug fixes

• Fix layout snippet used within another snippet (Layout snippets do not work from within other layout snippets #7567)

Docs

• https://getkirby.com/docs/guide/templates/snippets#passing-slots-to-snippets__layout-snippets

For review team

• Add lab and/or sandbox examples (wherever helpful)
• Add changes & docs to release notes draft in Notion

[JojOatXGME]

Note that any output from before opening the layout snippet gets discarded. This was already the case before. I think ideally, we should call Snippet::endlayout before Tpl::load calls ob_get_contents(). However, I avoided to call Snippet::endlayout from within Tpl::load because I assumed dependencies from \Kirby\Toolkit to \Kirby\Template are to be avoided. Alternatively, it might make sense to trigger an exception if there is any output before opening a layout snippet.

[distantnative]

@JojOatXGME Could you please provide the setup that this PR tries to solve. I have to admit I couldn't quite follow the description in the issue and therefore aren't sure if this is fixing an unwanted behavior or enabling an unwanted usage 😅

[JojOatXGME]

Could you please provide the setup that this PR tries to solve.

@distantnative I just created a fork of plainkit and made some adjustment to reproduce the issue. Does this help?

JojOatXGME/plainkit@5798fca

If you check out commit 5798fca5aa919f5160ed57964a22069f852542d0 and run the project, the resulting page will be empty. However, I would expect to see the content in the default template.

[afbora]

@JojOatXGME I've been trying to understand the PR for 15 minutes, but I don't get it, sorry 🙂 I just see that the Template::render method has been moved to Snippet::load. Am I missing something? Does this PR actually solve the problem?

[distantnative]

I think this might be the better/right direction, having the handling in Snippet::load() after every time a snippet file was actually loaded. I'll try to give it a test myself tomorrow, we might have to adapt some of the comments to make sense now.

The Template render call to snippet load reads a little weird - but might be full functional. Just semantically made me stumble.

[JojOatXGME]

I just see that the Template::render method has been moved to Snippet::load.

@afbora Snippet::render and Snippet::factory use static::load. They have previously used Tpl::load, but are now using Snippet::load. As a result, these two methods are now correctly handling layout snippets.

EDIT: I also now updated the description in the PR accordingly.

[distantnative]

We will need very thorough testing. These nested snippets and slots pose a lot of edge cases that I am afraid our unit tests do not fully cover yet. Before we merge, we should be very certain this doesn't add a regression for other use cases.

We also need to update the DocBlock and comments of the Snippet::load() method. It's misleading that we talk about templates win the context of snippets. I would also love if we could remove the term layout snippet (I know, not introduced but this PR but let's take the chance). Especially in this use case of nesting, the idea of layout doesn't make a lot of sense to me. I think in the end it's about snippets with slots where the closing endsnippet() call has been omitted and Kirby should end it implicitly at the end of a snippet file.

[distantnative]

Question: Where do you see the need to split this up into two parameters? Instead just having one $data parameter.

[JojOatXGME]

Currently, Template::render sets $layoutData to the same value as $data, but Snippet::render keeps the parameter empty. I only set $layoutData to the same value as $data inside Template::render for backward compatibility, but it looks to me like it actually might have been a bug. (At least the resulting deviation from normal snippets seems not to be documented.)

Within Snippet::render, I cannot set $layoutData to the same value as $data because this would lead to a runtime exception. The array for $layoutData currently must not contain 'slot' nor 'slots'. However, the $data given by Snippet::render does contain 'slot' and 'slots'.

I feel like the currently intended behavior is to always keep $layoutData empty. In other words, I think forwarding $data from the template to the snippet was actually another minor bug in the existing code, which I just preserved. If my understanding is correct, I could remove this parameter and always use an empty array instead.

However, if I look at the behavior not from the perspective of the bug fix, I would actually prefer if layout snippets would forward the data from the parent. I actually considered creating a feature request for that one or two weeks ago. This would match the behavior of “Template Inheritance” in other popular template engines like Twig, Laravel's Blade Templates, Jinja, and Django's template language. However, I would consider this a new feature and therefore did not implement this behavior. To make this feature work, I would always set $layoutData to the same value as $data, implement proper merging of slot data, and tweak the precedence of data coming from this parameter vs the data explicitly provided from within the template. (The explicit content from the template should maybe take precedence for data, but not for slots.) While I would be happy to implement this behavior, I did not consider it part of this ticket. Also note that full “Template Inheritance” usually also includes a feature to call super() or @@parent to preserve the content from the extended template. This functionality would still not be covered by the implementation I have just described.