Allow inline templates to use layouts.

[mkende]

Summary

Allow inline templates to use layouts. This requires the layout to be specified directly in the call to render or in the template itself, same as when using the render_to_string method.

Note: this is edited from the initial version where a layout set in the stash was also applied. The current version of this PR starts at this comment.

Motivation

There is no reason to disallow inline templates from using layouts if they can work and if doing so won’t break existing users.

In my particular case I am building a site where users can provide custom themes that are mostly implemented as templates. These templates can’ be guaranteed to have unique names so i can’t add there parents directory to the templates directories of the application. Using inline template is a workaround but currently prevent using layouts. This PR is attempting to fix this.

This is my first contributing to Mojolicious, so I may be missing something and the fix may not be correct (although all current tests pass, except those that specifically tested that inline template didn’t work with layouts and that are now fixed).

References

None.

[CandyAngel]

Done a quick bit of digging and it looks like inline templates using layouts was experimental in the past and was removed in 677fe63 due to causing issues.

[mkende]

Do you know what were or are these issues? I have tested a couple of scenarios in my code and this seems to work correctly.

[kraih]

If i remember correctly the problem was users setting app->defaults(layout => 'default') and then plugins that used inline rendering for various things misbehaving because their results suddenly had a layout wrapper.

[mkende]

Edit: note that following this discussion the PR was edited so that only a layout explicitly specified in the render() call is applied (following the same approach as in calls to render_to_string()). The current status of this PR starts at this comment.

That situation seems like a reasonable semantics and such a plugin is already broken if it uses a non-inline layout. That is, I would argue that the problem is either in setting a global layout or in the plugin not protecting against that (I understand that this is a change in semantics but the new one seems clearer and more general).

If it helps, I added a test that setting "layout => undef" when rendering an inline template correctly disable a default layout.

[mkende]

Another option would be to also delete the layout and extends stash value in Mojolicious::Controller::render() when rendering an inline template and not just when rendering to string (I guess that part has been implemented to fix that same problem with plugins — it seems improbable by the way that a plugin would not be rendering to a string if it wants to ignore the layout).

I think that this would solve my issue (being able to use layouts with inline template), but feels like adding a hack around the fact that using a global layout is anyway a dangerous thing (reading the documentation I already wondered why rendering to string behaved differently from the rest).

[kraih]

You misunderstand, the plugins use inline without a layout, and they are accidentally getting one because it's configured globally.

[mkende]

I understand that. What I meant is that having the global default layout applies to all template rendering, independently of whether the template is rendered to a string or not, or whether the template comes from a string or not seems a reasonable API, even if it can break existing behavior. And I was claiming that possibly code relying on the special casing of inline template needs to be fixed (either by not using a global default layout or by explicitly setting "layout => undef" when rendering a template in a plugin and you want to be sure to not be impacted by a global default template).

But as this might be difficult to do for all the existing code an alternative option might be to extend the current special handling of template rendered to string, to inline template and ignore the default layout in that case

To summarize the situation and possible options :

	normal template	rendered to string	inline template
current behavior	use layout	ignore global	never use
alternative 0 (initial version of this PR)	use layout	ignore global	use layout
alternative 1	use layout	use layout	use layout
alternative 2 (current version of this PR)	use layout	ignore global	ignore global

The current behavior is the most complex because each type of template has different rules. This PR and the alternative 1 are simpler and, subjectively, I think that they offer a more consistent API, but may be difficult to roll-out due to the breaking change to existing code. Alternative 2 might be offer a good balance.

Let me know what you think and I’m happy to implement alternative 1 or 2 if you would prefer one of them to this PR or the current situation.

[mkende]

Following the discussion on IRC two days ago which seems to indicate that PR #1782 is blocked for now, I would be interested in feedback on this PR here, in particular on what I called "option 2" in my previous comment (and which is now the version that is implemented here).

The PR does not introduce new keywords or new API, it should not break any existing use*, it makes the existing API more consistent, and it allows new features. So, I would hope that this is not a controversial change.

*The only cases that are changed are calls that are using inline template and explicitly specifying a layout in the call torender (or in the template itself), as the layout will no longer be ignored.

[kraih]

Kinda related, mojo.js is testing inline layouts. https://github.com/mojolicious/mojo.js/blob/main/docs/Introduction.md#layouts

[mkende]

Well I saw your message the other day about mojo.js being 15 times faster than Mojolicious and I’m sure there are plenty of other similar reasons to pick up Node these days, but I just like Perl too much to switch.

[kraih]

No, you seem to be misunderstanding what i meant. All the features we are testing with mojo.js can be backported to Mojolicious if they are a success.

[mkende]

Can a template declare its own layout in mojo.js as in Mojolicious? Because having a different stash keyword for the layout of inline template is reasonable but the template itself should not have to know whether it is an inline or non inline template (so, in Mojolicious, my expectation would be that the same layout helper should work if used from inside a layout executed inline).

[mkende]

Hi, any chance this PR could be reviewed? Or let me know if there is a time when we can sync on IRC if needed.

[mkende]

Kiwiroy, do you have other concerns with this PR? If not, do you feel like approving it if you have write access?

[mkende]

Any chance this can be reviewed? I understand that Kraih does not have the time currently for that, but maybe there are other maintainer of the project who could review it? If not, what should be my expectation here about having this PR eventually reviewed?

[kraih]

Because of the side effects the added complexity in the documentation and better alternative implemented by mojo.js, i have to give this proposal a 👎 .

[mkende]

What side effect? (and what alternative is mojo.js using ? can this be implemented in Mojolicious?)

[marcusramberg]

I think this can be closed, if this functionality is to be added #1887 seems a more likely candidate.